Fix client#removeGroupMemberships parameter order #51
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
I wasn't able to see the following Azure error in the audit logs
`warnings` field. It looks as though even though the request to Azure
fails, when the Azure app is removed the group memberships are still
cleaned up.
I was only able to see the error below by adding a log line to print the
error when revoke is called.
```
1 error occurred:
* error removing group membership: graphrbac.GroupsClient#RemoveMember: Failure responding to request: StatusCode=404 -- Original Error: autorest/azure: Service returned an error. Status=404 Code="Unknown" Message="Unknown service error" Details=[{"odata.error":{"code":"Request_ResourceNotFound","date":"2020-10-23T20:32:14","message":{"lang":"en","value":"Resource 'c1004989-2986-48e8-9e2e-aa03b7cacbc1' does not exist or one of its queried reference-property objects are not present."},"requestId":"a2d4d4c6-4e0a-4ea2-a807-16486c307d03"}}]
: timestamp=2020-10-23T14:32:14.604-0600
```
mdgreenfield
commented
Oct 23, 2020
client.go
Outdated
| @@ -302,7 +302,7 @@ func (c *client) removeGroupMemberships(ctx context.Context, servicePrincipalObj | |||
| var merr *multierror.Error | |||
|
|
|||
| for _, id := range groupIDs { | |||
| if _, err := c.provider.RemoveGroupMember(ctx, servicePrincipalObjectID, id); err != nil { | |||
| if _, err := c.provider.RemoveGroupMember(ctx, id, servicePrincipalObjectID); err != nil { | |||
There was a problem hiding this comment.
ref to RemoveGroupMember -
vault-plugin-secrets-azure/provider.go
Line 41 in 6544951
|
I came across this working on an implementation of #50 with our internal fork. |
|
|
austingebauer
approved these changes
Feb 2, 2023
There was a problem hiding this comment.
I realize it's been a while on this one, but it makes sense to fix the parameter order. Thanks, @mdgreenfield!
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I wasn't able to see the following Azure error in the audit logs
warningsfield. It looks as though even though the request to Azurefails, when the Azure app is removed the group memberships are still
cleaned up.
I was only able to see the error below by adding a log line to print the
error when revoke is called.
With that said, I think it begs the question whether or not calling
removeGroupMembershipsshould even be called on revoke.