From 125b50b4621aa40d77a07c5052c78a1b62bd7fa1 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Wed, 16 Apr 2025 14:32:16 +0000 Subject: [PATCH] test: add unit test to excercise bug when idp sync hits deleted orgs (#17405) Deleted organizations are still attempting to sync members. This causes an error on inserting the member, and would likely cause issues later in the sync process even if that member is inserted. Deleted orgs should be skipped. --- coderd/database/dbauthz/dbauthz_test.go | 5 +- coderd/database/dbfake/builder.go | 18 +++ coderd/database/dbmem/dbmem.go | 18 ++- coderd/database/queries.sql.go | 13 +- coderd/database/queries/organizations.sql | 9 +- coderd/idpsync/organization.go | 57 ++++++++- coderd/idpsync/organizations_test.go | 111 ++++++++++++++++++ coderd/users.go | 2 +- .../coderd/enidpsync/organizations_test.go | 42 ++++--- 9 files changed, 242 insertions(+), 33 deletions(-) diff --git a/coderd/database/dbauthz/dbauthz_test.go b/coderd/database/dbauthz/dbauthz_test.go index 259fd941d2921..2d4dac4e2902e 100644 --- a/coderd/database/dbauthz/dbauthz_test.go +++ b/coderd/database/dbauthz/dbauthz_test.go @@ -839,7 +839,7 @@ func (s *MethodTestSuite) TestOrganization() { _ = dbgen.OrganizationMember(s.T(), db, database.OrganizationMember{UserID: u.ID, OrganizationID: a.ID}) b := dbgen.Organization(s.T(), db, database.Organization{}) _ = dbgen.OrganizationMember(s.T(), db, database.OrganizationMember{UserID: u.ID, OrganizationID: b.ID}) - check.Args(database.GetOrganizationsByUserIDParams{UserID: u.ID, Deleted: false}).Asserts(a, policy.ActionRead, b, policy.ActionRead).Returns(slice.New(a, b)) + check.Args(database.GetOrganizationsByUserIDParams{UserID: u.ID, Deleted: sql.NullBool{Valid: true, Bool: false}}).Asserts(a, policy.ActionRead, b, policy.ActionRead).Returns(slice.New(a, b)) })) s.Run("InsertOrganization", s.Subtest(func(db database.Store, check *expects) { check.Args(database.InsertOrganizationParams{ @@ -948,8 +948,7 @@ func (s *MethodTestSuite) TestOrganization() { member, policy.ActionRead, member, policy.ActionDelete). WithNotAuthorized("no rows"). - WithCancelled(cancelledErr). - ErrorsWithInMemDB(sql.ErrNoRows) + WithCancelled(cancelledErr) })) s.Run("UpdateOrganization", s.Subtest(func(db database.Store, check *expects) { o := dbgen.Organization(s.T(), db, database.Organization{ diff --git a/coderd/database/dbfake/builder.go b/coderd/database/dbfake/builder.go index 6803374e72445..c39b00ac79054 100644 --- a/coderd/database/dbfake/builder.go +++ b/coderd/database/dbfake/builder.go @@ -17,6 +17,7 @@ type OrganizationBuilder struct { t *testing.T db database.Store seed database.Organization + delete bool allUsersAllowance int32 members []uuid.UUID groups map[database.Group][]uuid.UUID @@ -44,6 +45,12 @@ func (b OrganizationBuilder) EveryoneAllowance(allowance int) OrganizationBuilde return b } +func (b OrganizationBuilder) Deleted(deleted bool) OrganizationBuilder { + //nolint: revive // returns modified struct + b.delete = deleted + return b +} + func (b OrganizationBuilder) Seed(seed database.Organization) OrganizationBuilder { //nolint: revive // returns modified struct b.seed = seed @@ -118,6 +125,17 @@ func (b OrganizationBuilder) Do() OrganizationResponse { } } + if b.delete { + now := dbtime.Now() + err = b.db.UpdateOrganizationDeletedByID(ctx, database.UpdateOrganizationDeletedByIDParams{ + UpdatedAt: now, + ID: org.ID, + }) + require.NoError(b.t, err) + org.Deleted = true + org.UpdatedAt = now + } + return OrganizationResponse{ Org: org, AllUsersGroup: everyone, diff --git a/coderd/database/dbmem/dbmem.go b/coderd/database/dbmem/dbmem.go index e4956958e0073..9a7533a5894d5 100644 --- a/coderd/database/dbmem/dbmem.go +++ b/coderd/database/dbmem/dbmem.go @@ -2167,10 +2167,13 @@ func (q *FakeQuerier) DeleteOrganizationMember(ctx context.Context, arg database q.mutex.Lock() defer q.mutex.Unlock() - deleted := slices.DeleteFunc(q.data.organizationMembers, func(member database.OrganizationMember) bool { - return member.OrganizationID == arg.OrganizationID && member.UserID == arg.UserID + deleted := false + q.data.organizationMembers = slices.DeleteFunc(q.data.organizationMembers, func(member database.OrganizationMember) bool { + match := member.OrganizationID == arg.OrganizationID && member.UserID == arg.UserID + deleted = deleted || match + return match }) - if len(deleted) == 0 { + if !deleted { return sql.ErrNoRows } @@ -3758,6 +3761,9 @@ func (q *FakeQuerier) GetOrganizations(_ context.Context, args database.GetOrgan if args.Name != "" && !strings.EqualFold(org.Name, args.Name) { continue } + if args.Deleted != org.Deleted { + continue + } tmp = append(tmp, org) } @@ -3774,7 +3780,11 @@ func (q *FakeQuerier) GetOrganizationsByUserID(_ context.Context, arg database.G continue } for _, organization := range q.organizations { - if organization.ID != organizationMember.OrganizationID || organization.Deleted != arg.Deleted { + if organization.ID != organizationMember.OrganizationID { + continue + } + + if arg.Deleted.Valid && organization.Deleted != arg.Deleted.Bool { continue } organizations = append(organizations, organization) diff --git a/coderd/database/queries.sql.go b/coderd/database/queries.sql.go index 37c79537c622f..0d50b08f0bc69 100644 --- a/coderd/database/queries.sql.go +++ b/coderd/database/queries.sql.go @@ -5221,8 +5221,13 @@ SELECT FROM organizations WHERE - -- Optionally include deleted organizations - deleted = $2 AND + -- Optionally provide a filter for deleted organizations. + CASE WHEN + $2 :: boolean IS NULL THEN + true + ELSE + deleted = $2 + END AND id = ANY( SELECT organization_id @@ -5234,8 +5239,8 @@ WHERE ` type GetOrganizationsByUserIDParams struct { - UserID uuid.UUID `db:"user_id" json:"user_id"` - Deleted bool `db:"deleted" json:"deleted"` + UserID uuid.UUID `db:"user_id" json:"user_id"` + Deleted sql.NullBool `db:"deleted" json:"deleted"` } func (q *sqlQuerier) GetOrganizationsByUserID(ctx context.Context, arg GetOrganizationsByUserIDParams) ([]Organization, error) { diff --git a/coderd/database/queries/organizations.sql b/coderd/database/queries/organizations.sql index 822b51c0aa8ba..988efb535152d 100644 --- a/coderd/database/queries/organizations.sql +++ b/coderd/database/queries/organizations.sql @@ -55,8 +55,13 @@ SELECT FROM organizations WHERE - -- Optionally include deleted organizations - deleted = @deleted AND + -- Optionally provide a filter for deleted organizations. + CASE WHEN + sqlc.narg('deleted') :: boolean IS NULL THEN + true + ELSE + deleted = sqlc.narg('deleted') + END AND id = ANY( SELECT organization_id diff --git a/coderd/idpsync/organization.go b/coderd/idpsync/organization.go index 87fd9af5e935d..be65daba369df 100644 --- a/coderd/idpsync/organization.go +++ b/coderd/idpsync/organization.go @@ -92,14 +92,16 @@ func (s AGPLIDPSync) SyncOrganizations(ctx context.Context, tx database.Store, u return nil // No sync configured, nothing to do } - expectedOrgs, err := orgSettings.ParseClaims(ctx, tx, params.MergedClaims) + expectedOrgIDs, err := orgSettings.ParseClaims(ctx, tx, params.MergedClaims) if err != nil { return xerrors.Errorf("organization claims: %w", err) } + // Fetch all organizations, even deleted ones. This is to remove a user + // from any deleted organizations they may be in. existingOrgs, err := tx.GetOrganizationsByUserID(ctx, database.GetOrganizationsByUserIDParams{ UserID: user.ID, - Deleted: false, + Deleted: sql.NullBool{}, }) if err != nil { return xerrors.Errorf("failed to get user organizations: %w", err) @@ -109,10 +111,35 @@ func (s AGPLIDPSync) SyncOrganizations(ctx context.Context, tx database.Store, u return org.ID }) + // finalExpected is the final set of org ids the user is expected to be in. + // Deleted orgs are omitted from this set. + finalExpected := expectedOrgIDs + if len(expectedOrgIDs) > 0 { + // If you pass in an empty slice to the db arg, you get all orgs. So the slice + // has to be non-empty to get the expected set. Logically it also does not make + // sense to fetch an empty set from the db. + expectedOrganizations, err := tx.GetOrganizations(ctx, database.GetOrganizationsParams{ + IDs: expectedOrgIDs, + // Do not include deleted organizations. Omitting deleted orgs will remove the + // user from any deleted organizations they are a member of. + Deleted: false, + }) + if err != nil { + return xerrors.Errorf("failed to get expected organizations: %w", err) + } + finalExpected = db2sdk.List(expectedOrganizations, func(org database.Organization) uuid.UUID { + return org.ID + }) + } + // Find the difference in the expected and the existing orgs, and // correct the set of orgs the user is a member of. - add, remove := slice.SymmetricDifference(existingOrgIDs, expectedOrgs) - notExists := make([]uuid.UUID, 0) + add, remove := slice.SymmetricDifference(existingOrgIDs, finalExpected) + // notExists is purely for debugging. It logs when the settings want + // a user in an organization, but the organization does not exist. + notExists := slice.DifferenceFunc(expectedOrgIDs, finalExpected, func(a, b uuid.UUID) bool { + return a == b + }) for _, orgID := range add { _, err := tx.InsertOrganizationMember(ctx, database.InsertOrganizationMemberParams{ OrganizationID: orgID, @@ -123,9 +150,30 @@ func (s AGPLIDPSync) SyncOrganizations(ctx context.Context, tx database.Store, u }) if err != nil { if xerrors.Is(err, sql.ErrNoRows) { + // This should not happen because we check the org existence + // beforehand. notExists = append(notExists, orgID) continue } + + if database.IsUniqueViolation(err, database.UniqueOrganizationMembersPkey) { + // If we hit this error we have a bug. The user already exists in the + // organization, but was not detected to be at the start of this function. + // Instead of failing the function, an error will be logged. This is to not bring + // down the entire syncing behavior from a single failed org. Failing this can + // prevent user logins, so only fatal non-recoverable errors should be returned. + // + // Inserting a user is privilege escalation. So skipping this instead of failing + // leaves the user with fewer permissions. So this is safe from a security + // perspective to continue. + s.Logger.Error(ctx, "syncing user to organization failed as they are already a member, please report this failure to Coder", + slog.F("user_id", user.ID), + slog.F("username", user.Username), + slog.F("organization_id", orgID), + slog.Error(err), + ) + continue + } return xerrors.Errorf("add user to organization: %w", err) } } @@ -141,6 +189,7 @@ func (s AGPLIDPSync) SyncOrganizations(ctx context.Context, tx database.Store, u } if len(notExists) > 0 { + notExists = slice.Unique(notExists) // Remove duplicates s.Logger.Debug(ctx, "organizations do not exist but attempted to use in org sync", slog.F("not_found", notExists), slog.F("user_id", user.ID), diff --git a/coderd/idpsync/organizations_test.go b/coderd/idpsync/organizations_test.go index 51c8a7365d22b..3a00499bdbced 100644 --- a/coderd/idpsync/organizations_test.go +++ b/coderd/idpsync/organizations_test.go @@ -1,6 +1,7 @@ package idpsync_test import ( + "database/sql" "testing" "github.com/golang-jwt/jwt/v4" @@ -8,6 +9,11 @@ import ( "github.com/stretchr/testify/require" "cdr.dev/slog/sloggers/slogtest" + "github.com/coder/coder/v2/coderd/database" + "github.com/coder/coder/v2/coderd/database/db2sdk" + "github.com/coder/coder/v2/coderd/database/dbfake" + "github.com/coder/coder/v2/coderd/database/dbgen" + "github.com/coder/coder/v2/coderd/database/dbtestutil" "github.com/coder/coder/v2/coderd/idpsync" "github.com/coder/coder/v2/coderd/runtimeconfig" "github.com/coder/coder/v2/testutil" @@ -38,3 +44,108 @@ func TestParseOrganizationClaims(t *testing.T) { require.False(t, params.SyncEntitled) }) } + +func TestSyncOrganizations(t *testing.T) { + t.Parallel() + + // This test creates some deleted organizations and checks the behavior is + // correct. + t.Run("SyncUserToDeletedOrg", func(t *testing.T) { + t.Parallel() + + ctx := testutil.Context(t, testutil.WaitMedium) + db, _ := dbtestutil.NewDB(t) + user := dbgen.User(t, db, database.User{}) + + // Create orgs for: + // - stays = User is a member, and stays + // - leaves = User is a member, and leaves + // - joins = User is not a member, and joins + // For deleted orgs, the user **should not** be a member of afterwards. + // - deletedStays = User is a member of deleted org, and wants to stay + // - deletedLeaves = User is a member of deleted org, and wants to leave + // - deletedJoins = User is not a member of deleted org, and wants to join + stays := dbfake.Organization(t, db).Members(user).Do() + leaves := dbfake.Organization(t, db).Members(user).Do() + joins := dbfake.Organization(t, db).Do() + + deletedStays := dbfake.Organization(t, db).Members(user).Deleted(true).Do() + deletedLeaves := dbfake.Organization(t, db).Members(user).Deleted(true).Do() + deletedJoins := dbfake.Organization(t, db).Deleted(true).Do() + + // Now sync the user to the deleted organization + s := idpsync.NewAGPLSync( + slogtest.Make(t, &slogtest.Options{}), + runtimeconfig.NewManager(), + idpsync.DeploymentSyncSettings{ + OrganizationField: "orgs", + OrganizationMapping: map[string][]uuid.UUID{ + "stay": {stays.Org.ID, deletedStays.Org.ID}, + "leave": {leaves.Org.ID, deletedLeaves.Org.ID}, + "join": {joins.Org.ID, deletedJoins.Org.ID}, + }, + OrganizationAssignDefault: false, + }, + ) + + err := s.SyncOrganizations(ctx, db, user, idpsync.OrganizationParams{ + SyncEntitled: true, + MergedClaims: map[string]interface{}{ + "orgs": []string{"stay", "join"}, + }, + }) + require.NoError(t, err) + + orgs, err := db.GetOrganizationsByUserID(ctx, database.GetOrganizationsByUserIDParams{ + UserID: user.ID, + Deleted: sql.NullBool{}, + }) + require.NoError(t, err) + require.Len(t, orgs, 2) + + // Verify the user only exists in 2 orgs. The one they stayed, and the one they + // joined. + inIDs := db2sdk.List(orgs, func(org database.Organization) uuid.UUID { + return org.ID + }) + require.ElementsMatch(t, []uuid.UUID{stays.Org.ID, joins.Org.ID}, inIDs) + }) + + t.Run("UserToZeroOrgs", func(t *testing.T) { + t.Parallel() + + ctx := testutil.Context(t, testutil.WaitMedium) + db, _ := dbtestutil.NewDB(t) + user := dbgen.User(t, db, database.User{}) + + deletedLeaves := dbfake.Organization(t, db).Members(user).Deleted(true).Do() + + // Now sync the user to the deleted organization + s := idpsync.NewAGPLSync( + slogtest.Make(t, &slogtest.Options{}), + runtimeconfig.NewManager(), + idpsync.DeploymentSyncSettings{ + OrganizationField: "orgs", + OrganizationMapping: map[string][]uuid.UUID{ + "leave": {deletedLeaves.Org.ID}, + }, + OrganizationAssignDefault: false, + }, + ) + + err := s.SyncOrganizations(ctx, db, user, idpsync.OrganizationParams{ + SyncEntitled: true, + MergedClaims: map[string]interface{}{ + "orgs": []string{}, + }, + }) + require.NoError(t, err) + + orgs, err := db.GetOrganizationsByUserID(ctx, database.GetOrganizationsByUserIDParams{ + UserID: user.ID, + Deleted: sql.NullBool{}, + }) + require.NoError(t, err) + require.Len(t, orgs, 0) + }) +} diff --git a/coderd/users.go b/coderd/users.go index bf5b1db763fe9..4c3d1ceaa7b58 100644 --- a/coderd/users.go +++ b/coderd/users.go @@ -1274,7 +1274,7 @@ func (api *API) organizationsByUser(rw http.ResponseWriter, r *http.Request) { organizations, err := api.Database.GetOrganizationsByUserID(ctx, database.GetOrganizationsByUserIDParams{ UserID: user.ID, - Deleted: false, + Deleted: sql.NullBool{Bool: false, Valid: true}, }) if errors.Is(err, sql.ErrNoRows) { err = nil diff --git a/enterprise/coderd/enidpsync/organizations_test.go b/enterprise/coderd/enidpsync/organizations_test.go index 391535c9478d7..b2e120592b582 100644 --- a/enterprise/coderd/enidpsync/organizations_test.go +++ b/enterprise/coderd/enidpsync/organizations_test.go @@ -14,6 +14,7 @@ import ( "github.com/coder/coder/v2/coderd/database" "github.com/coder/coder/v2/coderd/database/db2sdk" "github.com/coder/coder/v2/coderd/database/dbauthz" + "github.com/coder/coder/v2/coderd/database/dbfake" "github.com/coder/coder/v2/coderd/database/dbgen" "github.com/coder/coder/v2/coderd/database/dbtestutil" "github.com/coder/coder/v2/coderd/entitlements" @@ -89,7 +90,8 @@ func TestOrganizationSync(t *testing.T) { Name: "SingleOrgDeployment", Case: func(t *testing.T, db database.Store) OrganizationSyncTestCase { def, _ := db.GetDefaultOrganization(context.Background()) - other := dbgen.Organization(t, db, database.Organization{}) + other := dbfake.Organization(t, db).Do() + deleted := dbfake.Organization(t, db).Deleted(true).Do() return OrganizationSyncTestCase{ Entitlements: entitled, Settings: idpsync.DeploymentSyncSettings{ @@ -123,11 +125,19 @@ func TestOrganizationSync(t *testing.T) { }) dbgen.OrganizationMember(t, db, database.OrganizationMember{ UserID: user.ID, - OrganizationID: other.ID, + OrganizationID: other.Org.ID, + }) + dbgen.OrganizationMember(t, db, database.OrganizationMember{ + UserID: user.ID, + OrganizationID: deleted.Org.ID, }) }, Sync: ExpectedUser{ - Organizations: []uuid.UUID{def.ID, other.ID}, + Organizations: []uuid.UUID{ + def.ID, other.Org.ID, + // The user remains in the deleted org because no idp sync happens. + deleted.Org.ID, + }, }, }, }, @@ -138,17 +148,19 @@ func TestOrganizationSync(t *testing.T) { Name: "MultiOrgWithDefault", Case: func(t *testing.T, db database.Store) OrganizationSyncTestCase { def, _ := db.GetDefaultOrganization(context.Background()) - one := dbgen.Organization(t, db, database.Organization{}) - two := dbgen.Organization(t, db, database.Organization{}) - three := dbgen.Organization(t, db, database.Organization{}) + one := dbfake.Organization(t, db).Do() + two := dbfake.Organization(t, db).Do() + three := dbfake.Organization(t, db).Do() + deleted := dbfake.Organization(t, db).Deleted(true).Do() return OrganizationSyncTestCase{ Entitlements: entitled, Settings: idpsync.DeploymentSyncSettings{ OrganizationField: "organizations", OrganizationMapping: map[string][]uuid.UUID{ - "first": {one.ID}, - "second": {two.ID}, - "third": {three.ID}, + "first": {one.Org.ID}, + "second": {two.Org.ID}, + "third": {three.Org.ID}, + "deleted": {deleted.Org.ID}, }, OrganizationAssignDefault: true, }, @@ -167,7 +179,7 @@ func TestOrganizationSync(t *testing.T) { { Name: "AlreadyInOrgs", Claims: jwt.MapClaims{ - "organizations": []string{"second", "extra"}, + "organizations": []string{"second", "extra", "deleted"}, }, ExpectedParams: idpsync.OrganizationParams{ SyncEntitled: true, @@ -180,18 +192,18 @@ func TestOrganizationSync(t *testing.T) { }) dbgen.OrganizationMember(t, db, database.OrganizationMember{ UserID: user.ID, - OrganizationID: one.ID, + OrganizationID: one.Org.ID, }) }, Sync: ExpectedUser{ - Organizations: []uuid.UUID{def.ID, two.ID}, + Organizations: []uuid.UUID{def.ID, two.Org.ID}, }, }, { Name: "ManyClaims", Claims: jwt.MapClaims{ // Add some repeats - "organizations": []string{"second", "extra", "first", "third", "second", "second"}, + "organizations": []string{"second", "extra", "first", "third", "second", "second", "deleted"}, }, ExpectedParams: idpsync.OrganizationParams{ SyncEntitled: true, @@ -204,11 +216,11 @@ func TestOrganizationSync(t *testing.T) { }) dbgen.OrganizationMember(t, db, database.OrganizationMember{ UserID: user.ID, - OrganizationID: one.ID, + OrganizationID: one.Org.ID, }) }, Sync: ExpectedUser{ - Organizations: []uuid.UUID{def.ID, one.ID, two.ID, three.ID}, + Organizations: []uuid.UUID{def.ID, one.Org.ID, two.Org.ID, three.Org.ID}, }, }, },