Skip to content

Commit 2a1e2d0

Browse files
EmyrkEmyrk
committed
chore: remove old role sync, insert new idpsync package
1 parent 601652c commit 2a1e2d0

File tree

13 files changed

+160
-195
lines changed

13 files changed

+160
-195
lines changed

‎cli/server.go

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -187,9 +187,6 @@ func createOIDCConfig(ctx context.Context, logger slog.Logger, vals *codersdk.De
187187
EmailField: vals.OIDC.EmailField.String(),
188188
AuthURLParams: vals.OIDC.AuthURLParams.Value,
189189
IgnoreUserInfo: vals.OIDC.IgnoreUserInfo.Value(),
190-
UserRoleField: vals.OIDC.UserRoleField.String(),
191-
UserRoleMapping: vals.OIDC.UserRoleMapping.Value,
192-
UserRolesDefault: vals.OIDC.UserRolesDefault.GetSlice(),
193190
SignInText: vals.OIDC.SignInText.String(),
194191
SignupsDisabledText: vals.OIDC.SignupsDisabledText.String(),
195192
IconURL: vals.OIDC.IconURL.String(),

‎coderd/coderd.go

Lines changed: 0 additions & 9 deletions
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-
SetUserSiteRoles func(ctx context.Context, logger slog.Logger, tx database.Store, userID uuid.UUID, roles []string) error
185184
TemplateScheduleStore *atomic.Pointer[schedule.TemplateScheduleStore]
186185
UserQuietHoursScheduleStore *atomic.Pointer[schedule.UserQuietHoursScheduleStore]
187186
AccessControlStore *atomic.Pointer[dbauthz.AccessControlStore]
@@ -373,14 +372,6 @@ func New(options *Options) *API {
373372
if options.TracerProvider == nil {
374373
options.TracerProvider = trace.NewNoopTracerProvider()
375374
}
376-
if options.SetUserSiteRoles == nil {
377-
options.SetUserSiteRoles = func(ctx context.Context, logger slog.Logger, _ database.Store, userID uuid.UUID, roles []string) error {
378-
logger.Warn(ctx, "attempted to assign OIDC user roles without enterprise license",
379-
slog.F("user_id", userID), slog.F("roles", roles),
380-
)
381-
return nil
382-
}
383-
}
384375
if options.TemplateScheduleStore == nil {
385376
options.TemplateScheduleStore = &atomic.Pointer[schedule.TemplateScheduleStore]{}
386377
}

‎coderd/idpsync/idpsync.go

Lines changed: 32 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -43,6 +43,24 @@ type IDPSync interface {
4343
// accessed concurrently. The settings are stored in the database.
4444
GroupSyncSettings(ctx context.Context, orgID uuid.UUID, db database.Store) (*GroupSyncSettings, error)
4545
UpdateGroupSettings(ctx context.Context, orgID uuid.UUID, db database.Store, settings GroupSyncSettings) error
46+
47+
// RoleSyncEntitled returns true if the deployment is entitled to role syncing.
48+
RoleSyncEntitled() bool
49+
// OrganizationRoleSyncEnabled returns true if the organization has role sync
50+
// enabled.
51+
OrganizationRoleSyncEnabled(ctx context.Context, db database.Store, org uuid.UUID) (bool, error)
52+
// SiteRoleSyncEnabled returns true if the deployment has role sync enabled
53+
// at the site level.
54+
SiteRoleSyncEnabled() bool
55+
// RoleSyncSettings is similar to GroupSyncSettings. See GroupSyncSettings for
56+
// rational.
57+
RoleSyncSettings() runtimeconfig.RuntimeEntry[*RoleSyncSettings]
58+
// ParseRoleClaims takes claims from an OIDC provider, and returns the params
59+
// for role syncing. Most of the logic happens in SyncRoles.
60+
ParseRoleClaims(ctx context.Context, mergedClaims jwt.MapClaims) (RoleParams, *HTTPError)
61+
// SyncRoles assigns and removes users from roles based on the provided params.
62+
// Site & org roles are handled in this method.
63+
SyncRoles(ctx context.Context, db database.Store, user database.User, params RoleParams) error
4664
}
4765

4866
// AGPLIDPSync is the configuration for syncing user information from an external
@@ -77,9 +95,16 @@ type DeploymentSyncSettings struct {
7795
// Legacy deployment settings that only apply to the default org.
7896
Legacy DefaultOrgLegacySettings
7997

80-
// SiteRoleField syncs a user's site wide roles from an IDP.
81-
SiteRoleField string
82-
SiteRoleMapping map[string][]string
98+
// SiteRoleField selects the claim field to be used as the created user's
99+
// roles. If the field is the empty string, then no site role updates
100+
// will ever come from the OIDC provider.
101+
SiteRoleField string
102+
// SiteRoleMapping controls how groups returned by the OIDC provider get mapped
103+
// to site roles within Coder.
104+
// map[oidcRoleName][]coderRoleName
105+
SiteRoleMapping map[string][]string
106+
// SiteDefaultRoles is the default set of site roles to assign to a user if role sync
107+
// is enabled.
83108
SiteDefaultRoles []string
84109
}
85110

@@ -99,6 +124,10 @@ func FromDeploymentValues(dv *codersdk.DeploymentValues) DeploymentSyncSettings
99124
OrganizationMapping: dv.OIDC.OrganizationMapping.Value,
100125
OrganizationAssignDefault: dv.OIDC.OrganizationAssignDefault.Value(),
101126

127+
SiteRoleField: dv.OIDC.UserRoleField.Value(),
128+
SiteRoleMapping: dv.OIDC.UserRoleMapping.Value,
129+
SiteDefaultRoles: dv.OIDC.UserRolesDefault.Value(),
130+
102131
// TODO: Separate group field for allow list from default org.
103132
// Right now you cannot disable group sync from the default org and
104133
// configure an allow list.

‎coderd/idpsync/role.go

Lines changed: 16 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -19,32 +19,42 @@ import (
1919
)
2020

2121
type RoleParams struct {
22-
// SyncEnabled if false will skip syncing the user's roles
23-
SyncEnabled bool
22+
// SyncEntitled if false will skip syncing the user's roles at
23+
// all levels.
24+
SyncEntitled bool
2425
SyncSiteWide bool
2526
SiteWideRoles []string
2627
// MergedClaims are passed to the organization level for syncing
2728
MergedClaims jwt.MapClaims
2829
}
2930

30-
func (AGPLIDPSync) RoleSyncEnabled() bool {
31+
func (AGPLIDPSync) RoleSyncEntitled() bool {
3132
// AGPL does not support syncing groups.
3233
return false
3334
}
35+
36+
func (AGPLIDPSync) OrganizationRoleSyncEnabled(_ context.Context, _ database.Store, _ uuid.UUID) (bool, error) {
37+
return false, nil
38+
}
39+
40+
func (AGPLIDPSync) SiteRoleSyncEnabled() bool {
41+
return false
42+
}
43+
3444
func (s AGPLIDPSync) RoleSyncSettings() runtimeconfig.RuntimeEntry[*RoleSyncSettings] {
3545
return s.Role
3646
}
3747

3848
func (s AGPLIDPSync) ParseRoleClaims(_ context.Context, _ jwt.MapClaims) (RoleParams, *HTTPError) {
3949
return RoleParams{
40-
SyncEnabled: s.RoleSyncEnabled(),
41-
SyncSiteWide: false,
50+
SyncEntitled: s.RoleSyncEntitled(),
51+
SyncSiteWide: s.SiteRoleSyncEnabled(),
4252
}, nil
4353
}
4454

4555
func (s AGPLIDPSync) SyncRoles(ctx context.Context, db database.Store, user database.User, params RoleParams) error {
4656
// Nothing happens if sync is not enabled
47-
if !params.SyncEnabled {
57+
if !params.SyncEntitled {
4858
return nil
4959
}
5060

‎coderd/idpsync/role_test.go

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -212,7 +212,7 @@ func TestRoleSyncTable(t *testing.T) {
212212

213213
// Do the role sync!
214214
err := s.SyncRoles(ctx, db, user, idpsync.RoleParams{
215-
SyncEnabled: true,
215+
SyncEntitled: true,
216216
SyncSiteWide: false,
217217
MergedClaims: userClaims,
218218
})
@@ -264,7 +264,7 @@ func TestRoleSyncTable(t *testing.T) {
264264
}
265265

266266
err := s.SyncRoles(ctx, db, user, idpsync.RoleParams{
267-
SyncEnabled: true,
267+
SyncEntitled: true,
268268
SyncSiteWide: true,
269269
SiteWideRoles: []string{
270270
rbac.RoleTemplateAdmin().Name, // Duplicate this value to test deduplication
@@ -353,7 +353,7 @@ func TestNoopNoDiff(t *testing.T) {
353353
RBACRoles: siteRoles,
354354
LoginType: database.LoginTypePassword,
355355
}, idpsync.RoleParams{
356-
SyncEnabled: true,
356+
SyncEntitled: true,
357357
SyncSiteWide: true,
358358
SiteWideRoles: siteRoles,
359359
MergedClaims: jwt.MapClaims{

‎coderd/members.go

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ import (
1111
"github.com/coder/coder/v2/coderd/audit"
1212
"github.com/coder/coder/v2/coderd/database"
1313
"github.com/coder/coder/v2/coderd/database/db2sdk"
14+
"github.com/coder/coder/v2/coderd/database/dbauthz"
1415
"github.com/coder/coder/v2/coderd/database/dbtime"
1516
"github.com/coder/coder/v2/coderd/httpapi"
1617
"github.com/coder/coder/v2/coderd/httpmw"
@@ -215,6 +216,33 @@ func (api *API) putMemberRoles(rw http.ResponseWriter, r *http.Request) {
215216
aReq.Old = member.OrganizationMember.Auditable(member.Username)
216217
defer commitAudit()
217218

219+
// Keep this block scoping to prevent accidental use of the user variable.
220+
{
221+
// nolint:gocritic // The caller could be an org admin without this perm.
222+
// We need to disable manual role assignment if role sync is enabled for
223+
// the given organization.
224+
user, err := api.Database.GetUserByID(dbauthz.AsSystemRestricted(ctx), member.UserID)
225+
if err != nil {
226+
httpapi.InternalServerError(rw, err)
227+
return
228+
}
229+
if user.LoginType == database.LoginTypeOIDC {
230+
// nolint:gocritic // fetching settings
231+
orgSync, err := api.IDPSync.OrganizationRoleSyncEnabled(dbauthz.AsSystemRestricted(ctx), api.Database, organization.ID)
232+
if err != nil {
233+
httpapi.InternalServerError(rw, err)
234+
return
235+
}
236+
if orgSync {
237+
httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{
238+
Message: "Cannot modify roles for OIDC users when role sync is enabled. This organization member's roles are managed by the identity provider.",
239+
Detail: "'User Role Field' is set in the organization settings. Ask an administrator to adjust or disable these settings.",
240+
})
241+
return
242+
}
243+
}
244+
}
245+
218246
if apiKey.UserID == member.OrganizationMember.UserID {
219247
httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{
220248
Message: "You cannot change your own organization roles.",

‎coderd/userauth.go

Lines changed: 12 additions & 113 deletions
Original file line numberDiff line numberDiff line change
@@ -740,17 +740,6 @@ type OIDCConfig struct {
740740
// support the userinfo endpoint, or if the userinfo endpoint causes
741741
// undesirable behavior.
742742
IgnoreUserInfo bool
743-
// UserRoleField selects the claim field to be used as the created user's
744-
// roles. If the field is the empty string, then no role updates
745-
// will ever come from the OIDC provider.
746-
UserRoleField string
747-
// UserRoleMapping controls how groups returned by the OIDC provider get mapped
748-
// to roles within Coder.
749-
// map[oidcRoleName][]coderRoleName
750-
UserRoleMapping map[string][]string
751-
// UserRolesDefault is the default set of roles to assign to a user if role sync
752-
// is enabled.
753-
UserRolesDefault []string
754743
// SignInText is the text to display on the OIDC login button
755744
SignInText string
756745
// IconURL points to the URL of an icon to display on the OIDC login button
@@ -759,10 +748,6 @@ type OIDCConfig struct {
759748
SignupsDisabledText string
760749
}
761750

762-
func (cfg OIDCConfig) RoleSyncEnabled() bool {
763-
return cfg.UserRoleField != ""
764-
}
765-
766751
// @Summary OpenID Connect Callback
767752
// @ID openid-connect-callback
768753
// @Security CoderSessionToken
@@ -983,12 +968,6 @@ func (api *API) userOIDC(rw http.ResponseWriter, r *http.Request) {
983968

984969
ctx = slog.With(ctx, slog.F("email", email), slog.F("username", username), slog.F("name", name))
985970

986-
roles, roleErr := api.oidcRoles(ctx, mergedClaims)
987-
if roleErr != nil {
988-
roleErr.Write(rw, r)
989-
return
990-
}
991-
992971
user, link, err := findLinkedUser(ctx, api.Database, oidcLinkedID(idToken), email)
993972
if err != nil {
994973
logger.Error(ctx, "oauth2: unable to find linked user", slog.F("email", email), slog.Error(err))
@@ -1011,6 +990,12 @@ func (api *API) userOIDC(rw http.ResponseWriter, r *http.Request) {
1011990
return
1012991
}
1013992

993+
roleSync, roleSyncErr := api.IDPSync.ParseRoleClaims(ctx, mergedClaims)
994+
if roleSyncErr != nil {
995+
roleSyncErr.Write(rw, r)
996+
return
997+
}
998+
1014999
// If a new user is authenticating for the first time
10151000
// the audit action is 'register', not 'login'
10161001
if user.ID == uuid.Nil {
@@ -1028,10 +1013,9 @@ func (api *API) userOIDC(rw http.ResponseWriter, r *http.Request) {
10281013
Username: username,
10291014
Name: name,
10301015
AvatarURL: picture,
1031-
UsingRoles: api.OIDCConfig.RoleSyncEnabled(),
1032-
Roles: roles,
10331016
OrganizationSync: orgSync,
10341017
GroupSync: groupSync,
1018+
RoleSync: roleSync,
10351019
DebugContext: OauthDebugContext{
10361020
IDTokenClaims: idtokenClaims,
10371021
UserInfoClaims: userInfoClaims,
@@ -1067,61 +1051,6 @@ func (api *API) userOIDC(rw http.ResponseWriter, r *http.Request) {
10671051
http.Redirect(rw, r, redirect, http.StatusTemporaryRedirect)
10681052
}
10691053

1070-
// oidcRoles returns the roles for the user from the OIDC claims.
1071-
// If the function returns false, then the caller should return early.
1072-
// All writes to the response writer are handled by this function.
1073-
// It would be preferred to just return an error, however this function
1074-
// decorates returned errors with the appropriate HTTP status codes and details
1075-
// that are hard to carry in a standard `error` without more work.
1076-
func (api *API) oidcRoles(ctx context.Context, mergedClaims map[string]interface{}) ([]string, *idpsync.HTTPError) {
1077-
roles := api.OIDCConfig.UserRolesDefault
1078-
if !api.OIDCConfig.RoleSyncEnabled() {
1079-
return roles, nil
1080-
}
1081-
1082-
rolesRow, ok := mergedClaims[api.OIDCConfig.UserRoleField]
1083-
if !ok {
1084-
// If no claim is provided than we can assume the user is just
1085-
// a member. This is because there is no way to tell the difference
1086-
// between []string{} and nil for OIDC claims. IDPs omit claims
1087-
// if they are empty ([]string{}).
1088-
// Use []interface{}{} so the next typecast works.
1089-
rolesRow = []interface{}{}
1090-
}
1091-
1092-
parsedRoles, err := idpsync.ParseStringSliceClaim(rolesRow)
1093-
if err != nil {
1094-
api.Logger.Error(ctx, "oidc claims user roles field was an unknown type",
1095-
slog.F("type", fmt.Sprintf("%T", rolesRow)),
1096-
slog.Error(err),
1097-
)
1098-
return nil, &idpsync.HTTPError{
1099-
Code: http.StatusInternalServerError,
1100-
Msg: "Login disabled until OIDC config is fixed",
1101-
Detail: fmt.Sprintf("Roles claim must be an array of strings, type found: %T. Disabling role sync will allow login to proceed.", rolesRow),
1102-
RenderStaticPage: false,
1103-
}
1104-
}
1105-
1106-
api.Logger.Debug(ctx, "roles returned in oidc claims",
1107-
slog.F("len", len(parsedRoles)),
1108-
slog.F("roles", parsedRoles),
1109-
)
1110-
for _, role := range parsedRoles {
1111-
if mappedRoles, ok := api.OIDCConfig.UserRoleMapping[role]; ok {
1112-
if len(mappedRoles) == 0 {
1113-
continue
1114-
}
1115-
// Mapped roles are added to the list of roles
1116-
roles = append(roles, mappedRoles...)
1117-
continue
1118-
}
1119-
1120-
roles = append(roles, role)
1121-
}
1122-
return roles, nil
1123-
}
1124-
11251054
// claimFields returns the sorted list of fields in the claims map.
11261055
func claimFields(claims map[string]interface{}) []string {
11271056
fields := []string{}
@@ -1182,10 +1111,7 @@ type oauthLoginParams struct {
11821111
// OrganizationSync has the organizations that the user will be assigned to.
11831112
OrganizationSync idpsync.OrganizationParams
11841113
GroupSync idpsync.GroupParams
1185-
// Is UsingRoles is true, then the user will be assigned
1186-
// the roles provided.
1187-
UsingRoles bool
1188-
Roles []string
1114+
RoleSync idpsync.RoleParams
11891115

11901116
DebugContext OauthDebugContext
11911117

@@ -1394,37 +1320,10 @@ func (api *API) oauthLogin(r *http.Request, params *oauthLoginParams) ([]*http.C
13941320
return xerrors.Errorf("sync groups: %w", err)
13951321
}
13961322

1397-
// Ensure roles are correct.
1398-
if params.UsingRoles {
1399-
ignored := make([]string, 0)
1400-
filtered := make([]string, 0, len(params.Roles))
1401-
for _, role := range params.Roles {
1402-
// TODO: This only supports mapping deployment wide roles. Organization scoped roles
1403-
// are unsupported.
1404-
if _, err := rbac.RoleByName(rbac.RoleIdentifier{Name: role}); err == nil {
1405-
filtered = append(filtered, role)
1406-
} else {
1407-
ignored = append(ignored, role)
1408-
}
1409-
}
1410-
1411-
//nolint:gocritic
1412-
err := api.Options.SetUserSiteRoles(dbauthz.AsSystemRestricted(ctx), logger, tx, user.ID, filtered)
1413-
if err != nil {
1414-
return &idpsync.HTTPError{
1415-
Code: http.StatusBadRequest,
1416-
Msg: "Invalid roles through OIDC claims",
1417-
Detail: fmt.Sprintf("Error from role assignment attempt: %s", err.Error()),
1418-
RenderStaticPage: true,
1419-
}
1420-
}
1421-
if len(ignored) > 0 {
1422-
logger.Debug(ctx, "OIDC roles ignored in assignment",
1423-
slog.F("ignored", ignored),
1424-
slog.F("assigned", filtered),
1425-
slog.F("user_id", user.ID),
1426-
)
1427-
}
1323+
// Role sync needs to occur after org sync.
1324+
err = api.IDPSync.SyncRoles(ctx, tx, user, params.RoleSync)
1325+
if err != nil {
1326+
return xerrors.Errorf("sync roles: %w", err)
14281327
}
14291328

14301329
needsUpdate := false

‎coderd/users.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1162,7 +1162,7 @@ func (api *API) putUserRoles(rw http.ResponseWriter, r *http.Request) {
11621162
defer commitAudit()
11631163
aReq.Old = user
11641164

1165-
if user.LoginType == database.LoginTypeOIDC && api.OIDCConfig.RoleSyncEnabled() {
1165+
if user.LoginType == database.LoginTypeOIDC && api.IDPSync.SiteRoleSyncEnabled() {
11661166
httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{
11671167
Message: "Cannot modify roles for OIDC users when role sync is enabled.",
11681168
Detail: "'User Role Field' is set in the OIDC configuration. All role changes must come from the oidc identity provider.",

0 commit comments

Comments
 (0)