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.

Sign up for GitHub

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

Jump to bottom

fix: delete users from specific identity provider #3398

Merged
merged 5 commits into from
Oct 11, 2022
Merged

fix: delete users from specific identity provider #3398

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
31697f5
fix: delete users from specific identity provider
BruceMacD Oct 5, 2022
6b1f50b
fix: delete users from specific identity provider
BruceMacD Oct 6, 2022
3b511df
fix: delete users from specific identity provider
BruceMacD Oct 7, 2022
7e59778
fix: delete users from specific identity provider
BruceMacD Oct 7, 2022
7adacb9
fix: delete users from specific identity provider
BruceMacD Oct 11, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 1 addition & 34 deletions internal/access/identity.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,6 @@ func CreateIdentity(c *gin.Context, identity *models.Identity) error {
return data.CreateIdentity(db, identity)
}

// TODO (https://github.com/infrahq/infra/issues/2318) remove provider user, not user.
func DeleteIdentity(c *gin.Context, id uid.ID) error {
rCtx := GetRequestContext(c)
self, err := isIdentitySelf(c, id)
Expand All @@ -60,39 +59,7 @@ func DeleteIdentity(c *gin.Context, id uid.ID) error {
return HandleAuthErr(err, "user", "delete", models.InfraAdminRole)
}

if err := data.DeleteAccessKeys(db, data.DeleteAccessKeysOptions{ByIssuedForID: id}); err != nil {
return fmt.Errorf("delete identity access keys: %w", err)
}

groups, err := data.ListGroups(db, nil, data.ByGroupMember(id))
if err != nil {
return fmt.Errorf("list groups for identity: %w", err)
}
for _, group := range groups {
err = data.RemoveUsersFromGroup(db, group.ID, []uid.ID{id})
if err != nil {
return fmt.Errorf("delete group membership for identity: %w", err)
}
}
// if an identity does not have credentials in the Infra provider this won't be found, but we can proceed
credential, err := data.GetCredential(db, data.ByIdentityID(id))
if err != nil && !errors.Is(err, internal.ErrNotFound) {
return fmt.Errorf("get delete identity creds: %w", err)
}

if credential != nil {
err := data.DeleteCredential(db, credential.ID)
if err != nil {
return fmt.Errorf("delete identity creds: %w", err)
}
}

err = data.DeleteGrants(db, data.DeleteGrantsOptions{BySubject: uid.NewIdentityPolymorphicID(id)})
if err != nil {
return fmt.Errorf("delete identity creds: %w", err)
}

return data.DeleteIdentity(db, id)
return data.DeleteIdentities(db, data.InfraProvider(db).ID, data.ByID(id))
BruceMacD marked this conversation as resolved.
Show resolved Hide resolved
}

func ListIdentities(c *gin.Context, name string, groupID uid.ID, ids []uid.ID, showSystem bool, p *data.Pagination) ([]models.Identity, error) {
Expand Down
6 changes: 6 additions & 0 deletions internal/access/identity_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,9 @@ func TestDeleteIdentityCleansUpResources(t *testing.T) {
err := data.CreateIdentity(db, identity)
assert.NilError(t, err)

_, err = data.CreateProviderUser(db, infraProvider, identity)
assert.NilError(t, err)
BruceMacD marked this conversation as resolved.
Show resolved Hide resolved

// create some resources for this identity

keyID := generate.MathRandom(models.AccessKeyKeyLength, generate.CharsetAlphaNumeric)
Expand Down Expand Up @@ -95,6 +98,9 @@ func TestDeleteIdentityCleansUpResources(t *testing.T) {
_, err = data.GetIdentity(db, data.ByID(identity.ID))
assert.ErrorIs(t, err, internal.ErrNotFound)

_, err = data.GetProviderUser(db, infraProvider.ID, identity.ID)
assert.ErrorIs(t, err, internal.ErrNotFound)

_, err = data.GetAccessKeyByKeyID(db, keyID)
assert.ErrorIs(t, err, internal.ErrNotFound)

Expand Down
4 changes: 2 additions & 2 deletions internal/cmd/users.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ $ infra users add johndoe@example.com`,
return err
}

cli.Output("Added user %q", args[0])
cli.Output("Added user %q to Infra", args[0])

if createResp.OneTimePassword != "" {
cli.Output("Password: %s", createResp.OneTimePassword)
Expand Down Expand Up @@ -241,7 +241,7 @@ $ infra users remove janedoe@example.com`,
return err
}

cli.Output("Removed user %q", user.Name)
cli.Output("Removed user %q from Infra", user.Name)
}

return nil
Expand Down
8 changes: 7 additions & 1 deletion internal/server/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -873,7 +873,7 @@ func (s Server) loadUsers(db data.GormTxn, users []User) error {
}

// remove any users previously defined by config
if err := data.DeleteIdentities(db, data.NotIDs(keep), data.CreatedBy(models.CreatedBySystem)); err != nil {
if err := data.DeleteIdentities(db, data.InfraProvider(db).ID, data.NotIDs(keep), data.CreatedBy(models.CreatedBySystem)); err != nil {
return err
}

Expand Down Expand Up @@ -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)
Copy link
Contributor

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.

Copy link
Collaborator Author

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"

if err != nil {
return nil, err
}

}

if err := s.loadCredential(db, identity, input.Password); err != nil {
Expand Down
2 changes: 1 addition & 1 deletion internal/server/config_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -702,7 +702,7 @@ func TestLoadConfigPruneConfig(t *testing.T) {

err = s.db.Raw("SELECT COUNT(*) FROM provider_users").Scan(&providerUsers).Error
assert.NilError(t, err)
assert.Equal(t, int64(0), providerUsers)
assert.Equal(t, int64(1), providerUsers)

// previous config is cleared on new config application
newConfig := Config{
Expand Down
74 changes: 54 additions & 20 deletions internal/server/data/identity.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,14 @@
package data

import (
"errors"
"fmt"
"time"

"github.com/ssoroka/slice"
"gorm.io/gorm"

"github.com/infrahq/infra/internal"
"github.com/infrahq/infra/internal/server/data/querybuilder"
"github.com/infrahq/infra/internal/server/models"
"github.com/infrahq/infra/uid"
Expand Down Expand Up @@ -147,11 +149,11 @@ func ListIdentities(db GormTxn, p *Pagination, selectors ...SelectorFunc) ([]mod
return list[models.Identity](db, p, selectors...)
}

func DeleteIdentity(db GormTxn, id uid.ID) error {
return delete[models.Identity](db, id)
func SaveIdentity(db GormTxn, identity *models.Identity) error {
return save(db, identity)
}

func DeleteIdentities(tx GormTxn, selectors ...SelectorFunc) error {
func DeleteIdentities(tx GormTxn, providerID uid.ID, selectors ...SelectorFunc) error {
selectID := func(db *gorm.DB) *gorm.DB {
return db.Select("id")
}
Expand All @@ -161,31 +163,63 @@ func DeleteIdentities(tx GormTxn, selectors ...SelectorFunc) error {
return err
}

ids := make([]uid.ID, 0)
for _, i := range toDelete {
ids = append(ids, i.ID)
ids, err := deleteReferencesToIdentities(tx, providerID, toDelete)
if err != nil {
return fmt.Errorf("remove identities: %w", err)
}

err := DeleteGrants(tx, DeleteGrantsOptions{BySubject: i.PolyID()})
if err != nil {
return err
if len(ids) > 0 {
return deleteAll[models.Identity](tx, ByIDs(ids))
}

return nil
}

func deleteReferencesToIdentities(tx GormTxn, providerID uid.ID, toDelete []models.Identity) (unreferencedIdentityIDs []uid.ID, err error) {
for _, i := range toDelete {
if err := DeleteAccessKeys(tx, DeleteAccessKeysOptions{ByIssuedForID: i.ID, ByProviderID: providerID}); err != nil {
return []uid.ID{}, fmt.Errorf("delete identity access keys: %w", err)
}
if providerID == InfraProvider(tx).ID {
// if an identity does not have credentials in the Infra provider this won't be found, but we can proceed
credential, err := GetCredential(tx, ByIdentityID(i.ID))
if err != nil && !errors.Is(err, internal.ErrNotFound) {
return []uid.ID{}, fmt.Errorf("get delete identity creds: %w", err)
}
if credential != nil {
err := DeleteCredential(tx, credential.ID)
if err != nil {
return []uid.ID{}, fmt.Errorf("delete identity creds: %w", err)
}
}
}
if err := DeleteProviderUsers(tx, DeleteProviderUsersOptions{ByIdentityID: i.ID, ByProviderID: providerID}); err != nil {
return []uid.ID{}, fmt.Errorf("remove provider user: %w", err)
}

groups, err := ListGroups(tx, nil, ByGroupMember(i.ID))
// if this identity no longer exists in any identity providers then remove all their references
user, err := GetIdentity(tx, Preload("Providers"), ByID(i.ID))
if err != nil {
return err
return []uid.ID{}, fmt.Errorf("check user providers: %w", err)
}

for _, group := range groups {
err = RemoveUsersFromGroup(tx, group.ID, []uid.ID{i.ID})
if len(user.Providers) == 0 {
groups, err := ListGroups(tx, nil, ByGroupMember(i.ID))
if err != nil {
return err
return []uid.ID{}, fmt.Errorf("list groups for identity: %w", err)
}
for _, group := range groups {
err = RemoveUsersFromGroup(tx, group.ID, []uid.ID{i.ID})
if err != nil {
return []uid.ID{}, fmt.Errorf("delete group membership for identity: %w", err)
}
}
err = DeleteGrants(tx, DeleteGrantsOptions{BySubject: uid.NewIdentityPolymorphicID(i.ID)})
if err != nil {
return []uid.ID{}, fmt.Errorf("delete identity creds: %w", err)
}
unreferencedIdentityIDs = append(unreferencedIdentityIDs, user.ID)
}
}

return deleteAll[models.Identity](tx, ByIDs(ids))
}

func SaveIdentity(db GormTxn, identity *models.Identity) error {
return save(db, identity)
return unreferencedIdentityIDs, nil
}
Loading