@@ -115,9 +115,13 @@ func (s AGPLIDPSync) SyncOrganizations(ctx context.Context, tx database.Store, u
115115// Deleted orgs are omitted from this set.
116116finalExpected := expectedOrgIDs
117117if len (expectedOrgIDs )> 0 {
118+ // If you pass in an empty slice to the db arg, you get all orgs. So the slice
119+ // has to be non-empty to get the expected set. Logically it also does not make
120+ // sense to fetch an empty set from the db.
118121expectedOrganizations ,err := tx .GetOrganizations (ctx , database.GetOrganizationsParams {
119122IDs :expectedOrgIDs ,
120- // Do not include deleted organizations
123+ // Do not include deleted organizations. Omitting deleted orgs will remove the
124+ // user from any deleted organizations they are a member of.
121125Deleted :false ,
122126})
123127if err != nil {
@@ -158,6 +162,10 @@ func (s AGPLIDPSync) SyncOrganizations(ctx context.Context, tx database.Store, u
158162// Instead of failing the function, an error will be logged. This is to not bring
159163// down the entire syncing behavior from a single failed org. Failing this can
160164// prevent user logins, so only fatal non-recoverable errors should be returned.
165+ //
166+ // Inserting a user is privilege escalation. So skipping this instead of failing
167+ // leaves the user with fewer permissions. So this is safe from a security
168+ // perspective to continue.
161169s .Logger .Error (ctx ,"syncing user to organization failed as they are already a member, please report this failure to Coder" ,
162170slog .F ("user_id" ,user .ID ),
163171slog .F ("username" ,user .Username ),