Skip to content

Commit a54b876

Browse files
EmyrkEmyrk
committed
chore: remove old group sync code
1 parent 9887057 commit a54b876

File tree

6 files changed

+36
-232
lines changed

6 files changed

+36
-232
lines changed

‎coderd/coderd.go

-11
Original file line numberDiff line numberDiff line change
@@ -181,7 +181,6 @@ type Options struct {
181181
NetworkTelemetryBatchFrequency time.Duration
182182
NetworkTelemetryBatchMaxSize int
183183
SwaggerEndpoint bool
184-
SetUserGroups func(ctx context.Context, logger slog.Logger, tx database.Store, userID uuid.UUID, orgGroupNames map[uuid.UUID][]string, createMissingGroups bool) error
185184
SetUserSiteRoles func(ctx context.Context, logger slog.Logger, tx database.Store, userID uuid.UUID, roles []string) error
186185
TemplateScheduleStore *atomic.Pointer[schedule.TemplateScheduleStore]
187186
UserQuietHoursScheduleStore *atomic.Pointer[schedule.UserQuietHoursScheduleStore]
@@ -374,16 +373,6 @@ func New(options *Options) *API {
374373
if options.TracerProvider == nil {
375374
options.TracerProvider = trace.NewNoopTracerProvider()
376375
}
377-
if options.SetUserGroups == nil {
378-
options.SetUserGroups = func(ctx context.Context, logger slog.Logger, _ database.Store, userID uuid.UUID, orgGroupNames map[uuid.UUID][]string, createMissingGroups bool) error {
379-
logger.Warn(ctx, "attempted to assign OIDC groups without enterprise license",
380-
slog.F("user_id", userID),
381-
slog.F("groups", orgGroupNames),
382-
slog.F("create_missing_groups", createMissingGroups),
383-
)
384-
return nil
385-
}
386-
}
387376
if options.SetUserSiteRoles == nil {
388377
options.SetUserSiteRoles = func(ctx context.Context, logger slog.Logger, _ database.Store, userID uuid.UUID, roles []string) error {
389378
logger.Warn(ctx, "attempted to assign OIDC user roles without enterprise license",

‎coderd/idpsync/group.go

+7-1
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@ import (
1414
"github.com/coder/coder/v2/coderd/database"
1515
"github.com/coder/coder/v2/coderd/database/db2sdk"
1616
"github.com/coder/coder/v2/coderd/database/dbauthz"
17+
"github.com/coder/coder/v2/coderd/runtimeconfig"
1718
"github.com/coder/coder/v2/coderd/util/slice"
1819
)
1920

@@ -76,7 +77,12 @@ func (s AGPLIDPSync) SyncGroups(ctx context.Context, db database.Store, user dat
7677
orgResolver := s.Manager.OrganizationResolver(tx, orgID)
7778
settings, err := s.SyncSettings.Group.Resolve(ctx, orgResolver)
7879
if err != nil {
79-
return xerrors.Errorf("resolve group sync settings: %w", err)
80+
if xerrors.Is(err, runtimeconfig.EntryNotFound) {
81+
// Default to not being configured
82+
settings = &GroupSyncSettings{}
83+
} else {
84+
return xerrors.Errorf("resolve group sync settings: %w", err)
85+
}
8086
}
8187

8288
// Legacy deployment settings will override empty settings.

‎coderd/idpsync/idpsync.go

+2-2
Original file line numberDiff line numberDiff line change
@@ -27,15 +27,15 @@ type IDPSync interface {
2727
OrganizationSyncEnabled() bool
2828
// ParseOrganizationClaims takes claims from an OIDC provider, and returns the
2929
// organization sync params for assigning users into organizations.
30-
ParseOrganizationClaims(ctx context.Context, _ jwt.MapClaims) (OrganizationParams, *HTTPError)
30+
ParseOrganizationClaims(ctx context.Context, mergedClaims jwt.MapClaims) (OrganizationParams, *HTTPError)
3131
// SyncOrganizations assigns and removed users from organizations based on the
3232
// provided params.
3333
SyncOrganizations(ctx context.Context, tx database.Store, user database.User, params OrganizationParams) error
3434

3535
GroupSyncEnabled() bool
3636
// ParseGroupClaims takes claims from an OIDC provider, and returns the params
3737
// for group syncing. Most of the logic happens in SyncGroups.
38-
ParseGroupClaims(ctx context.Context, _ jwt.MapClaims) (GroupParams, *HTTPError)
38+
ParseGroupClaims(ctx context.Context, mergedClaims jwt.MapClaims) (GroupParams, *HTTPError)
3939

4040
// SyncGroups assigns and removes users from groups based on the provided params.
4141
SyncGroups(ctx context.Context, db database.Store, user database.User, params GroupParams) error

‎coderd/userauth.go

+27-151
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,6 @@ import (
2020
"github.com/google/go-github/v43/github"
2121
"github.com/google/uuid"
2222
"github.com/moby/moby/pkg/namesgenerator"
23-
"golang.org/x/exp/slices"
2423
"golang.org/x/oauth2"
2524
"golang.org/x/xerrors"
2625

@@ -659,6 +658,9 @@ func (api *API) userOAuth2Github(rw http.ResponseWriter, r *http.Request) {
659658
AvatarURL: ghUser.GetAvatarURL(),
660659
Name: normName,
661660
DebugContext: OauthDebugContext{},
661+
GroupSync: idpsync.GroupParams{
662+
SyncEnabled: false,
663+
},
662664
OrganizationSync: idpsync.OrganizationParams{
663665
SyncEnabled: false,
664666
IncludeDefault: true,
@@ -1004,11 +1006,6 @@ func (api *API) userOIDC(rw http.ResponseWriter, r *http.Request) {
10041006
}
10051007

10061008
ctx = slog.With(ctx, slog.F("email", email), slog.F("username", username), slog.F("name", name))
1007-
usingGroups, groups, groupErr := api.oidcGroups(ctx, mergedClaims)
1008-
if groupErr != nil {
1009-
groupErr.Write(rw, r)
1010-
return
1011-
}
10121009

10131010
roles, roleErr := api.oidcRoles(ctx, mergedClaims)
10141011
if roleErr != nil {
@@ -1032,30 +1029,33 @@ func (api *API) userOIDC(rw http.ResponseWriter, r *http.Request) {
10321029
return
10331030
}
10341031

1032+
groupSync, groupSyncErr := api.IDPSync.ParseGroupClaims(ctx, mergedClaims)
1033+
if groupSyncErr != nil {
1034+
groupSyncErr.Write(rw, r)
1035+
return
1036+
}
1037+
10351038
// If a new user is authenticating for the first time
10361039
// the audit action is 'register', not 'login'
10371040
if user.ID == uuid.Nil {
10381041
aReq.Action = database.AuditActionRegister
10391042
}
10401043

10411044
params := (&oauthLoginParams{
1042-
User: user,
1043-
Link: link,
1044-
State: state,
1045-
LinkedID: oidcLinkedID(idToken),
1046-
LoginType: database.LoginTypeOIDC,
1047-
AllowSignups: api.OIDCConfig.AllowSignups,
1048-
Email: email,
1049-
Username: username,
1050-
Name: name,
1051-
AvatarURL: picture,
1052-
UsingRoles: api.OIDCConfig.RoleSyncEnabled(),
1053-
Roles: roles,
1054-
UsingGroups: usingGroups,
1055-
Groups: groups,
1056-
OrganizationSync: orgSync,
1057-
CreateMissingGroups: api.OIDCConfig.CreateMissingGroups,
1058-
GroupFilter: api.OIDCConfig.GroupFilter,
1045+
User: user,
1046+
Link: link,
1047+
State: state,
1048+
LinkedID: oidcLinkedID(idToken),
1049+
LoginType: database.LoginTypeOIDC,
1050+
AllowSignups: api.OIDCConfig.AllowSignups,
1051+
Email: email,
1052+
Username: username,
1053+
Name: name,
1054+
AvatarURL: picture,
1055+
UsingRoles: api.OIDCConfig.RoleSyncEnabled(),
1056+
Roles: roles,
1057+
OrganizationSync: orgSync,
1058+
GroupSync: groupSync,
10591059
DebugContext: OauthDebugContext{
10601060
IDTokenClaims: idtokenClaims,
10611061
UserInfoClaims: userInfoClaims,
@@ -1091,79 +1091,6 @@ func (api *API) userOIDC(rw http.ResponseWriter, r *http.Request) {
10911091
http.Redirect(rw, r, redirect, http.StatusTemporaryRedirect)
10921092
}
10931093

1094-
// oidcGroups returns the groups for the user from the OIDC claims.
1095-
func (api *API) oidcGroups(ctx context.Context, mergedClaims map[string]interface{}) (bool, []string, *idpsync.HTTPError) {
1096-
logger := api.Logger.Named(userAuthLoggerName)
1097-
usingGroups := false
1098-
var groups []string
1099-
1100-
// If the GroupField is the empty string, then groups from OIDC are not used.
1101-
// This is so we can support manual group assignment.
1102-
if api.OIDCConfig.GroupField != "" {
1103-
// If the allow list is empty, then the user is allowed to log in.
1104-
// Otherwise, they must belong to at least 1 group in the allow list.
1105-
inAllowList := len(api.OIDCConfig.GroupAllowList) == 0
1106-
1107-
usingGroups = true
1108-
groupsRaw, ok := mergedClaims[api.OIDCConfig.GroupField]
1109-
if ok {
1110-
parsedGroups, err := idpsync.ParseStringSliceClaim(groupsRaw)
1111-
if err != nil {
1112-
api.Logger.Debug(ctx, "groups field was an unknown type in oidc claims",
1113-
slog.F("type", fmt.Sprintf("%T", groupsRaw)),
1114-
slog.Error(err),
1115-
)
1116-
return false, nil, &idpsync.HTTPError{
1117-
Code: http.StatusBadRequest,
1118-
Msg: "Failed to sync groups from OIDC claims",
1119-
Detail: err.Error(),
1120-
RenderStaticPage: false,
1121-
}
1122-
}
1123-
1124-
api.Logger.Debug(ctx, "groups returned in oidc claims",
1125-
slog.F("len", len(parsedGroups)),
1126-
slog.F("groups", parsedGroups),
1127-
)
1128-
1129-
for _, group := range parsedGroups {
1130-
if mappedGroup, ok := api.OIDCConfig.GroupMapping[group]; ok {
1131-
group = mappedGroup
1132-
}
1133-
if _, ok := api.OIDCConfig.GroupAllowList[group]; ok {
1134-
inAllowList = true
1135-
}
1136-
groups = append(groups, group)
1137-
}
1138-
}
1139-
1140-
if !inAllowList {
1141-
logger.Debug(ctx, "oidc group claim not in allow list, rejecting login",
1142-
slog.F("allow_list_count", len(api.OIDCConfig.GroupAllowList)),
1143-
slog.F("user_group_count", len(groups)),
1144-
)
1145-
detail := "Ask an administrator to add one of your groups to the allow list"
1146-
if len(groups) == 0 {
1147-
detail = "You are currently not a member of any groups! Ask an administrator to add you to an authorized group to login."
1148-
}
1149-
return usingGroups, groups, &idpsync.HTTPError{
1150-
Code: http.StatusForbidden,
1151-
Msg: "Not a member of an allowed group",
1152-
Detail: detail,
1153-
RenderStaticPage: true,
1154-
}
1155-
}
1156-
}
1157-
1158-
// This conditional is purely to warn the user they might have misconfigured their OIDC
1159-
// configuration.
1160-
if _, groupClaimExists := mergedClaims["groups"]; !usingGroups && groupClaimExists {
1161-
logger.Debug(ctx, "claim 'groups' was returned, but 'oidc-group-field' is not set, check your coder oidc settings")
1162-
}
1163-
1164-
return usingGroups, groups, nil
1165-
}
1166-
11671094
// oidcRoles returns the roles for the user from the OIDC claims.
11681095
// If the function returns false, then the caller should return early.
11691096
// All writes to the response writer are handled by this function.
@@ -1278,14 +1205,7 @@ type oauthLoginParams struct {
12781205
AvatarURL string
12791206
// OrganizationSync has the organizations that the user will be assigned to.
12801207
OrganizationSync idpsync.OrganizationParams
1281-
// Is UsingGroups is true, then the user will be assigned
1282-
// to the Groups provided.
1283-
UsingGroups bool
1284-
CreateMissingGroups bool
1285-
// These are the group names from the IDP. Internally, they will map to
1286-
// some organization groups.
1287-
Groups []string
1288-
GroupFilter *regexp.Regexp
1208+
GroupSync idpsync.GroupParams
12891209
// Is UsingRoles is true, then the user will be assigned
12901210
// the roles provided.
12911211
UsingRoles bool
@@ -1491,53 +1411,9 @@ func (api *API) oauthLogin(r *http.Request, params *oauthLoginParams) ([]*http.C
14911411
return xerrors.Errorf("sync organizations: %w", err)
14921412
}
14931413

1494-
// Ensure groups are correct.
1495-
// This places all groups into the default organization.
1496-
// To go multi-org, we need to add a mapping feature here to know which
1497-
// groups go to which orgs.
1498-
if params.UsingGroups {
1499-
filtered := params.Groups
1500-
if params.GroupFilter != nil {
1501-
filtered = make([]string, 0, len(params.Groups))
1502-
for _, group := range params.Groups {
1503-
if params.GroupFilter.MatchString(group) {
1504-
filtered = append(filtered, group)
1505-
}
1506-
}
1507-
}
1508-
1509-
//nolint:gocritic // No user present in the context.
1510-
defaultOrganization, err := tx.GetDefaultOrganization(dbauthz.AsSystemRestricted(ctx))
1511-
if err != nil {
1512-
// If there is no default org, then we can't assign groups.
1513-
// By default, we assume all groups belong to the default org.
1514-
return xerrors.Errorf("get default organization: %w", err)
1515-
}
1516-
1517-
//nolint:gocritic // No user present in the context.
1518-
memberships, err := tx.OrganizationMembers(dbauthz.AsSystemRestricted(ctx), database.OrganizationMembersParams{
1519-
UserID: user.ID,
1520-
OrganizationID: uuid.Nil,
1521-
})
1522-
if err != nil {
1523-
return xerrors.Errorf("get organization memberships: %w", err)
1524-
}
1525-
1526-
// If the user is not in the default organization, then we can't assign groups.
1527-
// A user cannot be in groups to an org they are not a member of.
1528-
if !slices.ContainsFunc(memberships, func(member database.OrganizationMembersRow) bool {
1529-
return member.OrganizationMember.OrganizationID == defaultOrganization.ID
1530-
}) {
1531-
return xerrors.Errorf("user %s is not a member of the default organization, cannot assign to groups in the org", user.ID)
1532-
}
1533-
1534-
//nolint:gocritic
1535-
err = api.Options.SetUserGroups(dbauthz.AsSystemRestricted(ctx), logger, tx, user.ID, map[uuid.UUID][]string{
1536-
defaultOrganization.ID: filtered,
1537-
}, params.CreateMissingGroups)
1538-
if err != nil {
1539-
return xerrors.Errorf("set user groups: %w", err)
1540-
}
1414+
err = api.IDPSync.SyncGroups(ctx, tx, user, params.GroupSync)
1415+
if err != nil {
1416+
return xerrors.Errorf("sync groups: %w", err)
15411417
}
15421418

15431419
// Ensure roles are correct.

‎enterprise/coderd/coderd.go

-1
Original file line numberDiff line numberDiff line change
@@ -145,7 +145,6 @@ func New(ctx context.Context, options *Options) (_ *API, err error) {
145145
}
146146
return c.Subject, c.Trial, nil
147147
}
148-
api.AGPL.Options.SetUserGroups = api.setUserGroups
149148
api.AGPL.Options.SetUserSiteRoles = api.setUserSiteRoles
150149
api.AGPL.SiteHandler.RegionsFetcher = func(ctx context.Context) (any, error) {
151150
// If the user can read the workspace proxy resource, return that.

‎enterprise/coderd/userauth.go

-66
Original file line numberDiff line numberDiff line change
@@ -8,75 +8,9 @@ import (
88

99
"cdr.dev/slog"
1010
"github.com/coder/coder/v2/coderd/database"
11-
"github.com/coder/coder/v2/coderd/database/dbauthz"
1211
"github.com/coder/coder/v2/codersdk"
1312
)
1413

15-
// nolint: revive
16-
func (api *API) setUserGroups(ctx context.Context, logger slog.Logger, db database.Store, userID uuid.UUID, orgGroupNames map[uuid.UUID][]string, createMissingGroups bool) error {
17-
if !api.Entitlements.Enabled(codersdk.FeatureTemplateRBAC) {
18-
return nil
19-
}
20-
21-
return db.InTx(func(tx database.Store) error {
22-
// When setting the user's groups, it's easier to just clear their groups and re-add them.
23-
// This ensures that the user's groups are always in sync with the auth provider.
24-
orgs, err := tx.GetOrganizationsByUserID(ctx, userID)
25-
if err != nil {
26-
return xerrors.Errorf("get user orgs: %w", err)
27-
}
28-
if len(orgs) != 1 {
29-
return xerrors.Errorf("expected 1 org, got %d", len(orgs))
30-
}
31-
32-
// Delete all groups the user belongs to.
33-
// nolint:gocritic // Requires system context to remove user from all groups.
34-
err = tx.RemoveUserFromAllGroups(dbauthz.AsSystemRestricted(ctx), userID)
35-
if err != nil {
36-
return xerrors.Errorf("delete user groups: %w", err)
37-
}
38-
39-
// TODO: This could likely be improved by making these single queries.
40-
// Either by batching or some other means. This for loop could be really
41-
// inefficient if there are a lot of organizations. There was deployments
42-
// on v1 with >100 orgs.
43-
for orgID, groupNames := range orgGroupNames {
44-
// Create the missing groups for each organization.
45-
if createMissingGroups {
46-
// This is the system creating these additional groups, so we use the system restricted context.
47-
// nolint:gocritic
48-
created, err := tx.InsertMissingGroups(dbauthz.AsSystemRestricted(ctx), database.InsertMissingGroupsParams{
49-
OrganizationID: orgID,
50-
GroupNames: groupNames,
51-
Source: database.GroupSourceOidc,
52-
})
53-
if err != nil {
54-
return xerrors.Errorf("insert missing groups: %w", err)
55-
}
56-
if len(created) > 0 {
57-
logger.Debug(ctx, "auto created missing groups",
58-
slog.F("org_id", orgID.ID),
59-
slog.F("created", created),
60-
slog.F("num", len(created)),
61-
)
62-
}
63-
}
64-
65-
// Re-add the user to all groups returned by the auth provider.
66-
err = tx.InsertUserGroupsByName(ctx, database.InsertUserGroupsByNameParams{
67-
UserID: userID,
68-
OrganizationID: orgID,
69-
GroupNames: groupNames,
70-
})
71-
if err != nil {
72-
return xerrors.Errorf("insert user groups: %w", err)
73-
}
74-
}
75-
76-
return nil
77-
}, nil)
78-
}
79-
8014
func (api *API) setUserSiteRoles(ctx context.Context, logger slog.Logger, db database.Store, userID uuid.UUID, roles []string) error {
8115
if !api.Entitlements.Enabled(codersdk.FeatureUserRoleManagement) {
8216
logger.Warn(ctx, "attempted to assign OIDC user roles without enterprise entitlement, roles left unchanged",

0 commit comments

Comments
 (0)