Skip to content

Commit

Permalink
Fix client#removeGroupMemberships parameter order (#51)
Browse files Browse the repository at this point in the history
* Fix client#removeGroupMemberships parameter order

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
```

* Add test assertion to ensure deleting group member works

---------

Co-authored-by: Austin Gebauer <[email protected]>
  • Loading branch information
mdgreenfield and austingebauer authored Feb 2, 2023
1 parent cb6c2c9 commit e2de23d
Show file tree
Hide file tree
Showing 2 changed files with 5 additions and 1 deletion.
2 changes: 1 addition & 1 deletion client.go
Original file line number Diff line number Diff line change
Expand Up @@ -252,7 +252,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 {

// If a membership was deleted manually then Azure returns a error with a Status=404
if strings.Contains(err.Error(), "Status=404") {
Expand Down
4 changes: 4 additions & 0 deletions path_service_principal_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -578,6 +578,10 @@ func TestSPRevoke(t *testing.T) {
t.Fatalf("receive response error: %v", resp.Error())
}

if len(resp.Warnings) > 0 {
t.Fatalf("response contains warnings but should not have")
}

if client.provider.(*mockProvider).appExists(appObjID) {
t.Fatalf("application present but should have been deleted")
}
Expand Down

0 comments on commit e2de23d

Please sign in to comment.