maintain: track provider groups separate from users #3211

BruceMacD · 2022-09-15T13:29:26Z

add provider information to trace identity group membership relations
migrate provider user groups to provider groups

Summary

In order to track where users had their groups assigned to them more accurately updating the database schema to track and resolve group membership. This means tracking the state of external identity provider groups with relations to their provider users, and adding provider information to identities_groups relations.

identities_groups relations now have the form (identity_id, group_id, provider_id, provider_group_name) to allow mapping external groups from identity providers to groups within Infra. This behavior will be a later change, this PR keeps the same behavior of automatically mapping groups from identity providers to groups within Infra that have the same name.

Checklist

Wrote appropriate unit tests
Considered security implications of the change
Updated associated docs where necessary
Updated associated configuration where necessary
Change is backwards compatible if it needs to be (user can upgrade without manual steps?)
Nothing sensitive logged
Considered data migrations for smooth upgrades

Related Issues

Resolves #3138
Resolves #2982

BruceMacD · 2022-09-15T13:30:22Z

internal/access/identity.go

@@ -170,10 +170,6 @@ func UpdateIdentityInfoFromProvider(c RequestContext, oidc providers.OIDCClient)
 			logging.Errorf("failed to revoke invalid user session: %s", nestedErr)
 		}

-		if nestedErr := data.DeleteProviderUsers(db, data.ByIdentityID(identity.ID), data.ByProviderID(provider.ID)); nestedErr != nil {


Removed this, on a failure the access is revoked, but we don't know that the user no longer exists. Its confusing when their group membership changes based on a failed login.

BruceMacD · 2022-09-15T13:30:53Z

internal/server/authn/oidc.go

@@ -45,7 +45,7 @@ func (a *oidcAuthn) Authenticate(ctx context.Context, db data.GormTxn, requested
 		return AuthenticatedIdentity{}, fmt.Errorf("exhange code for tokens: %w", err)
 	}

-	identity, err := data.GetIdentity(db, data.Preload("Groups"), data.ByName(email))


The identity groups loaded here weren't used

internal/server/data/schema.sql

dnephin

Nice! I haven't looked over all of it yet, but left a couple suggestions/questions on the model and diagram

docs/dev/identity-provider-tracking.md

internal/server/models/providergroup.go

docs/dev/identity-provider-tracking.md

internal/server/data/group.go

internal/server/models/provideruser.go

internal/server/data/migrations.go

internal/server/data/group.go

ssoroka

I think we've really complicated things here and the complication isn't worth it. I really hate that the queries now have to query provider tables to build the picture of what our memberships are.

internal/server/data/identity.go

internal/server/data/providergroup.go

internal/server/data/schema.sql

ssoroka

I think we can do more to simplify this. There's no reason to keep two active join tables for membership; the provider memberships should update our memberships which simplifies querying and joining a ton; it's a lot easier if our core application knows nothing about providers and provider groups

internal/server/data/migrations.go

internal/server/data/providergroup.go

dnephin

I think this is looking good! I was surprised that identities_groups was still around after these changes. I left a question about that below.

I'll continue to review tomorrow.

internal/server/data/providergroup_test.go

internal/server/data/providergroup.go

internal/server/models/providergroup.go

internal/server/data/providergroup.go

internal/server/data/migrations.go

internal/access/identity.go

internal/server/data/group.go

ssoroka · 2022-09-29T17:27:36Z

internal/server/data/group.go

@@ -101,7 +102,7 @@ func DeleteGroup(tx WriteTxn, id uid.ID) error {

 	_, err = tx.Exec(`DELETE from identities_groups WHERE group_id = ?`, id)


I think this has a similar problem to users; If you're deleting a group that was both created by infra and also the result of a mapping, what happens to that mapping? do you leave the group and only remove the identities_groups record for the infra provider?

Yeah, this would result in the group being deleted, and then just re-created next time a user in a provider with that group is synchronized. That's what would happen at the moment too, so I think the fix for this is in our group mapping UI which will come after this.

internal/server/data/group.go

internal/server/data/migrations.go

ssoroka · 2022-09-29T20:56:39Z

internal/server/data/providergroup.go

+	if opts.ByMemberIdentityID != 0 {
+		query.B(`
+			JOIN provider_groups_provider_users 
+			ON provider_groups.name = provider_groups_provider_users.provider_group_name 


should it join on id instead? names could be problematic if they're not indexed

- join group membership from provider group relation - unlink provider groups from groups on deletion - migrate provider user groups to provider groups

BruceMacD requested review from technovangelist, dnephin, jmorganca, pdevine and mchiang0610 as code owners September 15, 2022 13:29

BruceMacD commented Sep 15, 2022

View reviewed changes

BruceMacD force-pushed the brucemacd/provider_groups branch from 201f914 to 84ede6c Compare September 15, 2022 14:03

vercel bot temporarily deployed to Preview September 15, 2022 14:03 Inactive

dnephin reviewed Sep 15, 2022

View reviewed changes

docs/dev/identity-provider-tracking.md Outdated Show resolved Hide resolved

internal/server/models/providergroup.go Outdated Show resolved Hide resolved

docs/dev/identity-provider-tracking.md Show resolved Hide resolved

dnephin reviewed Sep 15, 2022

View reviewed changes

internal/server/data/group.go Outdated Show resolved Hide resolved