cleanup comments

Emyrk · Emyrk · commit 71d82f74bbe4 · 2024-09-09T10:34:16.000-05:00
diff --git a/coderd/idpsync/group.go b/coderd/idpsync/group.go
@@ -206,7 +206,7 @@ func (s AGPLIDPSync) ApplyGroupDifference(ctx context.Context, tx database.Store
 			return xerrors.Errorf("remove user from %d groups: %w", len(removeIDs), err)
 		}
 		if len(removedGroupIDs) != len(removeIDs) {
-			s.Logger.Debug(ctx, "failed to remove user from all groups",
+			s.Logger.Debug(ctx, "user not removed from expected number of groups",
 				slog.F("user_id", user.ID),
 				slog.F("groups_removed_count", len(removedGroupIDs)),
 				slog.F("expected_count", len(removeIDs)),
@@ -225,7 +225,7 @@ func (s AGPLIDPSync) ApplyGroupDifference(ctx context.Context, tx database.Store
 			return xerrors.Errorf("insert user into %d groups: %w", len(add), err)
 		}
 		if len(assignedGroupIDs) != len(add) {
-			s.Logger.Debug(ctx, "failed to assign all groups to user",
+			s.Logger.Debug(ctx, "user not assigned to expected number of groups",
 				slog.F("user_id", user.ID),
 				slog.F("groups_assigned_count", len(assignedGroupIDs)),
 				slog.F("expected_count", len(add)),
@@ -355,8 +355,7 @@ func (s GroupSyncSettings) ParseClaims(orgID uuid.UUID, mergedClaims jwt.MapClai
 // TODO: Batching this would be better, as this is 1 or 2 db calls per organization.
 func (s GroupSyncSettings) HandleMissingGroups(ctx context.Context, tx database.Store, orgID uuid.UUID, add []ExpectedGroup) ([]uuid.UUID, error) {
 	// All expected that are missing IDs means the group does not exist
-	// in the database. Either remove them, or create them if auto create is
-	// turned on.
+	// in the database, or it is a legacy mapping, and we need to do a lookup.
 	var missingGroups []string
 	addIDs := make([]uuid.UUID, 0)
 
diff --git a/coderd/idpsync/idpsync.go b/coderd/idpsync/idpsync.go
@@ -90,7 +90,9 @@ func FromDeploymentValues(dv *codersdk.DeploymentValues) DeploymentSyncSettings
 		OrganizationMapping:       dv.OIDC.OrganizationMapping.Value,
 		OrganizationAssignDefault: dv.OIDC.OrganizationAssignDefault.Value(),
 
-		// TODO: Separate group field for allow list from default org
+		// TODO: Separate group field for allow list from default org.
+		// Right now you cannot disable group sync from the default org and
+		// configure an allow list.
 		GroupField:     dv.OIDC.GroupField.Value(),
 		GroupAllowList: ConvertAllowList(dv.OIDC.GroupAllowList.Value()),
 		Legacy: DefaultOrgLegacySettings{
diff --git a/coderd/userauth.go b/coderd/userauth.go
@@ -1389,6 +1389,8 @@ func (api *API) oauthLogin(r *http.Request, params *oauthLoginParams) ([]*http.C
 			return xerrors.Errorf("sync organizations: %w", err)
 		}
 
+		// Group sync needs to occur after org sync, since a user can join an org,
+		// then have their groups sync to said org.
 		err = api.IDPSync.SyncGroups(ctx, tx, user, params.GroupSync)
 		if err != nil {
 			return xerrors.Errorf("sync groups: %w", err)

Original file line number	Diff line number	Diff line change
`@@ -1389,6 +1389,8 @@ func (api API) oauthLogin(r http.Request, params oauthLoginParams) ([]http.C`
`1389`	`1389`	`return xerrors.Errorf("sync organizations: %w", err)`
`1390`	`1390`	`}`
`1391`	`1391`
	`1392`	`+ // Group sync needs to occur after org sync, since a user can join an org,`
	`1393`	`+ // then have their groups sync to said org.`
`1392`	`1394`	`err = api.IDPSync.SyncGroups(ctx, tx, user, params.GroupSync)`
`1393`	`1395`	`if err != nil {`
`1394`	`1396`	`return xerrors.Errorf("sync groups: %w", err)`