Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add identity secrets engine: Entity, EntityAlias, Group and GroupAlias #77

Merged
merged 10 commits into from
Mar 16, 2024

Conversation

stormshield-gt
Copy link
Collaborator

@stormshield-gt stormshield-gt commented Mar 1, 2024

This PR is a follow-up of #64 and #38 that include the review feedback plus the missing methods.

It implements all the endpoint for Entity, EntityAlias, Group and GroupAlias

Most of the job is done, it's still a draft because I want to polish the entity::create and group::create with the same tricks I used for entity_alias::create and group_alias::create.

src/api/identity.rs Outdated Show resolved Hide resolved
@stormshield-gt stormshield-gt force-pushed the add_Identity_secrets_engine branch 3 times, most recently from 030f518 to 74bd058 Compare March 4, 2024 18:17
@stormshield-gt stormshield-gt changed the title Draft: Add identity secrets engine: Entity, EntityAlias, Group and GroupAlias Add identity secrets engine: Entity, EntityAlias, Group and GroupAlias Mar 4, 2024
@stormshield-gt
Copy link
Collaborator Author

I've just rebased on master to get the latest changes, this is ready for further review

Copy link
Collaborator

@Haennetz Haennetz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some cosmetic stuff and typo fixes functionality looks good. Nice work! 👍

src/api/identity/entity/responses.rs Outdated Show resolved Hide resolved
src/api/identity/entity/responses.rs Outdated Show resolved Hide resolved
src/api/identity/entity/responses.rs Outdated Show resolved Hide resolved
src/api/identity/entity/responses.rs Outdated Show resolved Hide resolved
src/api/identity/entity/responses.rs Outdated Show resolved Hide resolved
src/api/identity/entity/requests.rs Outdated Show resolved Hide resolved
src/api/identity/entity/requests.rs Outdated Show resolved Hide resolved
src/api/identity/entity/requests.rs Outdated Show resolved Hide resolved
src/api/identity/entity/responses.rs Outdated Show resolved Hide resolved
src/api/identity/entity_alias/requests.rs Show resolved Hide resolved
@Haennetz
Copy link
Collaborator

Haennetz commented Mar 9, 2024

Also, pleas remove the extra API function exec_with_result_or_empty.
I think the entity and group endpoints do the following.
When the request to the create function includes an id vault send it to the update function which then returns an HTTP response with the return code 204 No Content that's the reason it returns sometime an empty body.

I came up with 2 options to solve this:

  1. check if the create function is called with an opts that includes an id and return an error that mentions that the update_by_id should be used.
  2. Remove the ID from the create request and explain that the update request should be used.

Let me know what you are thinking about the solution.

Copy link
Collaborator

@Haennetz Haennetz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought about the issue with the empty response again, and I came up with that solution to solve this.
Let me know what you think about it.

src/api/identity/entity/requests.rs Outdated Show resolved Hide resolved
src/identity/entity.rs Outdated Show resolved Hide resolved
src/identity/group.rs Outdated Show resolved Hide resolved
Haennetz

This comment was marked as resolved.

Co-authored-by: Hännetz <34142036+Haennetz@users.noreply.github.com>
@stormshield-gt
Copy link
Collaborator Author

Thanks for taking time to review this! I think the suggested solution to the "maybe empty result" problem is fine, I would like to suggest another alternative before I go to implement it.

Maybe we can just remove the id field of CreateEntityRequest and CreateGroupRequest?

The main advantage of this is that we could then return directly a CreateEntityResponse without the Option.
I think the "create" use case of the endpoint is the one we should be favored, so removing the Option should make life easier for users most of the time. The 2 previous PR were going with this direction, so I suppose that's what they wanted.
It also prevents errors at compile time, making it impossible to use it on "update" mode and needing a special logic inside the calling function.
Besides, in the suggested changes, the endpoint reached can be different from the one from the doc, which can be surprising.

This does have the disadvantage to not be conformed to the create entity/group official doc, as we won't support updating through this method. But as long as we make it clear that the user should directly use the update method, I think it's fine.

Anyway, that's just a suggestion, and I'm totally fine going with the proposed solution. Please let me know what you think about this.

@Haennetz
Copy link
Collaborator

Thanks for your suggestion i think it is a beeter way.
Maybe you can mention the reason why we removed the id from the create requests and point the users to the Update request if they want to use the id.

@stormshield-gt stormshield-gt force-pushed the add_Identity_secrets_engine branch 3 times, most recently from 84a8ed2 to 39c416d Compare March 12, 2024 16:28
@Haennetz Haennetz merged commit 192ba2a into jmgilman:master Mar 16, 2024
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants