@@ -115,9 +115,13 @@ func (s AGPLIDPSync) SyncOrganizations(ctx context.Context, tx database.Store, u
115115 // Deleted orgs are omitted from this set.
116116 finalExpected := expectedOrgIDs
117117 if 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.
118121 expectedOrganizations , err := tx .GetOrganizations (ctx , database.GetOrganizationsParams {
119122 IDs : 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.
121125 Deleted : false ,
122126 })
123127 if 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.
161169 s .Logger .Error (ctx , "syncing user to organization failed as they are already a member, please report this failure to Coder" ,
162170 slog .F ("user_id" , user .ID ),
163171 slog .F ("username" , user .Username ),
0 commit comments