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}, }, }, },