coder · gcp-cherry-pick-bot · Apr 16, 2025
diff --git a/coderd/database/dbauthz/dbauthz_test.go b/coderd/database/dbauthz/dbauthz_test.go
@@ -878,7 +878,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{
@@ -987,8 +987,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
@@ -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
@@ -45,6 +46,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
@@ -119,6 +126,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
@@ -2350,10 +2350,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
 	}
 
@@ -4143,6 +4146,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)
 	}
 
@@ -4159,7 +4165,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
diff --git 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
@@ -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
@@ -1,13 +1,19 @@
 package idpsync_test
 
 import (
+	"database/sql"
 	"testing"
 
 	"github.com/golang-jwt/jwt/v4"
 	"github.com/google/uuid"
 	"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
@@ -1302,7 +1302,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