-
Notifications
You must be signed in to change notification settings - Fork 56
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
fix: delete users from specific identity provider #3398
Conversation
- infra users command adds/removes from the infra identity provider specifically - consolidate identity deletion logic - delete identity when it no longer exists in any identity provider
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! I think this will resolves a couple problems.
There are a lot of calls to DeleteIdentity
so I think I'll need another pass to think through all those scenarios and see how it matches up to our test coverage. Left a few questions from my first pass.
@@ -902,6 +902,12 @@ func (s Server) loadUser(db data.GormTxn, input User) (*models.Identity, error) | |||
if err := data.CreateIdentity(db, identity); err != nil { | |||
return nil, err | |||
} | |||
|
|||
_, err = data.CreateProviderUser(db, data.InfraProvider(db), identity) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at where we call CreateProviderUser
, it seems like the majority of the time it's either accompanied by CreateCredential
, or by CreateIdentity
.
I wonder if we should have CreateCredential
call this automatically for us (since a credential doesn't work without this record, and the call is idempontent), and add a CreateInfraUser
that creates both the identity and provider_user.
Probably not for this PR, but it's something I've been wondering for a bit now. Maybe that'll make it easier to ensure that tests correctly reflect production without having to remember to call both.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CreateCredential
creating the provider user would make a lot of sense, but the limitation there at the moment is that user can be declared in grants without having a credential linked to them.
Ex:
infra grants add dne@example.com infra --role admin --force
Created user "dne@example.com"
Created grant to "infra" for "dne@example.com"
- do not delete credential when identity is not deleted from infra provider - test credential not deleted in infra provider - assert provider user is deleted in test
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DeleteIdentities
does a lot of things, and I think we might be missing assertions for the conditional removal of grants and groups. With that test coverage I think this looks great!
Should access.UpdateIdentityInfoFromProvider
be calling data.DeleteIdentities
now to clean up? It's currently only deleting the access key and provider_user, but I would expect it to need to do more if that was the last provider.
- update tests
Summary
When a user is deleted using
infra users add
orinfra users rm
only delete the resources associated with the identity through the Infra provider. Users from other identity providers can be removed by removing the associated provider, or through SCIM (soon).Checklist
Related Issues
Resolves #3397
Resolves #2838