From 726e7b1813f8881cb7ae23ec76d836d2c89620a0 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Wed, 6 Dec 2023 10:15:14 -0600 Subject: [PATCH 01/11] chore: use httpError to allow better error elevation --- coderd/userauth.go | 45 ++++++++++++++++++++++----------------------- 1 file changed, 22 insertions(+), 23 deletions(-) diff --git a/coderd/userauth.go b/coderd/userauth.go index 0ffa4ca271567..10398e02334ad 100644 --- a/coderd/userauth.go +++ b/coderd/userauth.go @@ -925,18 +925,15 @@ func (api *API) userOIDC(rw http.ResponseWriter, r *http.Request) { picture, _ = pictureRaw.(string) } - usingGroups, groups, err := api.oidcGroups(ctx, mergedClaims) - if err != nil { - httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{ - Message: "Failed to sync groups from OIDC claims", - Detail: err.Error(), - }) + usingGroups, groups, groupErr := api.oidcGroups(ctx, mergedClaims) + if groupErr != nil { + groupErr.Write(rw, r) return } - roles, ok := api.oidcRoles(ctx, rw, r, mergedClaims) - if !ok { - // oidcRoles writes the error to the response writer for us. + roles, roleErr := api.oidcRoles(ctx, mergedClaims) + if roleErr != nil { + roleErr.Write(rw, r) return } @@ -1009,7 +1006,7 @@ func (api *API) userOIDC(rw http.ResponseWriter, r *http.Request) { } // oidcGroups returns the groups for the user from the OIDC claims. -func (api *API) oidcGroups(ctx context.Context, mergedClaims map[string]interface{}) (bool, []string, error) { +func (api *API) oidcGroups(ctx context.Context, mergedClaims map[string]interface{}) (bool, []string, *httpError) { logger := api.Logger.Named(userAuthLoggerName) usingGroups := false var groups []string @@ -1026,7 +1023,12 @@ func (api *API) oidcGroups(ctx context.Context, mergedClaims map[string]interfac slog.F("type", fmt.Sprintf("%T", groupsRaw)), slog.Error(err), ) - return false, nil, err + return false, nil, &httpError{ + code: http.StatusBadRequest, + msg: "Failed to sync groups from OIDC claims", + detail: err.Error(), + renderStaticPage: false, + } } api.Logger.Debug(ctx, "groups returned in oidc claims", @@ -1058,10 +1060,10 @@ func (api *API) oidcGroups(ctx context.Context, mergedClaims map[string]interfac // It would be preferred to just return an error, however this function // decorates returned errors with the appropriate HTTP status codes and details // that are hard to carry in a standard `error` without more work. -func (api *API) oidcRoles(ctx context.Context, rw http.ResponseWriter, r *http.Request, mergedClaims map[string]interface{}) ([]string, bool) { +func (api *API) oidcRoles(ctx context.Context, mergedClaims map[string]interface{}) ([]string, *httpError) { roles := api.OIDCConfig.UserRolesDefault if !api.OIDCConfig.RoleSyncEnabled() { - return roles, true + return roles, nil } rolesRow, ok := mergedClaims[api.OIDCConfig.UserRoleField] @@ -1080,15 +1082,12 @@ func (api *API) oidcRoles(ctx context.Context, rw http.ResponseWriter, r *http.R slog.F("type", fmt.Sprintf("%T", rolesRow)), slog.Error(err), ) - site.RenderStaticErrorPage(rw, r, site.ErrorPageData{ - Status: http.StatusInternalServerError, - HideStatus: true, - Title: "Login disabled until OIDC config is fixed", - Description: fmt.Sprintf("Roles claim must be an array of strings, type found: %T. Disabling role sync will allow login to proceed.", rolesRow), - RetryEnabled: false, - DashboardURL: "/login", - }) - return nil, false + return nil, &httpError{ + code: http.StatusInternalServerError, + msg: "Login disabled until OIDC config is fixed", + detail: fmt.Sprintf("Roles claim must be an array of strings, type found: %T. Disabling role sync will allow login to proceed.", rolesRow), + renderStaticPage: false, + } } api.Logger.Debug(ctx, "roles returned in oidc claims", @@ -1107,7 +1106,7 @@ func (api *API) oidcRoles(ctx context.Context, rw http.ResponseWriter, r *http.R roles = append(roles, role) } - return roles, true + return roles, nil } // claimFields returns the sorted list of fields in the claims map. From db5a8aa78b43fc998adc9ec039ec70779b37f0f6 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Wed, 6 Dec 2023 11:38:11 -0600 Subject: [PATCH 02/11] feat: group allow list in OIDC settings Users not in the group allowlist cannot authenticate with Coder. --- cli/server.go | 1 + coderd/userauth.go | 25 +++++++++++++++++++++++++ codersdk/deployment.go | 11 +++++++++++ 3 files changed, 37 insertions(+) diff --git a/cli/server.go b/cli/server.go index 99dccc1088646..93989f16010e0 100644 --- a/cli/server.go +++ b/cli/server.go @@ -161,6 +161,7 @@ func createOIDCConfig(ctx context.Context, vals *codersdk.DeploymentValues) (*co IgnoreUserInfo: vals.OIDC.IgnoreUserInfo.Value(), GroupField: vals.OIDC.GroupField.String(), GroupFilter: vals.OIDC.GroupRegexFilter.Value(), + GroupAllowList: vals.OIDC.GroupAllowList.Value(), CreateMissingGroups: vals.OIDC.GroupAutoCreate.Value(), GroupMapping: vals.OIDC.GroupMapping.Value, UserRoleField: vals.OIDC.UserRoleField.String(), diff --git a/coderd/userauth.go b/coderd/userauth.go index 10398e02334ad..405e7cb580c8d 100644 --- a/coderd/userauth.go +++ b/coderd/userauth.go @@ -701,6 +701,10 @@ type OIDCConfig struct { // the OIDC provider. Any group not matched by this regex will be ignored. // If the group filter is nil, then no group filtering will occur. GroupFilter *regexp.Regexp + // GroupAllowList is a list of groups that are allowed to log in. + // If the list length is 0, then the allow list will not be applied and + // this feature is disabled. + GroupAllowList []string // GroupMapping controls how groups returned by the OIDC provider get mapped // to groups within Coder. // map[oidcGroupName]coderGroupName @@ -1014,6 +1018,15 @@ func (api *API) oidcGroups(ctx context.Context, mergedClaims map[string]interfac // If the GroupField is the empty string, then groups from OIDC are not used. // This is so we can support manual group assignment. if api.OIDCConfig.GroupField != "" { + // allow list is a map of groups that are allowed to log in. + allowed := make(map[string]bool) + for _, group := range api.OIDCConfig.GroupAllowList { + allowed[group] = true + } + // If the allow list is empty, then the user is allowed to log in. + // Otherwise, they must belong to at least 1 group in the allow list. + inAllowList := len(allowed) == 0 + usingGroups = true groupsRaw, ok := mergedClaims[api.OIDCConfig.GroupField] if ok { @@ -1040,9 +1053,21 @@ func (api *API) oidcGroups(ctx context.Context, mergedClaims map[string]interfac if mappedGroup, ok := api.OIDCConfig.GroupMapping[group]; ok { group = mappedGroup } + if _, ok := allowed[group]; ok { + inAllowList = true + } groups = append(groups, group) } } + + if !inAllowList { + return usingGroups, groups, &httpError{ + code: http.StatusForbidden, + msg: "You aren't a member of an authorized group!", + detail: fmt.Sprintf("You must be a member of one of the following groups: %v", api.OIDCConfig.GroupAllowList), + renderStaticPage: false, + } + } } // This conditional is purely to warn the user they might have misconfigured their OIDC diff --git a/codersdk/deployment.go b/codersdk/deployment.go index 56dd5b38f0c8d..45d1d7601ef2f 100644 --- a/codersdk/deployment.go +++ b/codersdk/deployment.go @@ -291,6 +291,7 @@ type OIDCConfig struct { IgnoreUserInfo clibase.Bool `json:"ignore_user_info" typescript:",notnull"` GroupAutoCreate clibase.Bool `json:"group_auto_create" typescript:",notnull"` GroupRegexFilter clibase.Regexp `json:"group_regex_filter" typescript:",notnull"` + GroupAllowList clibase.StringArray `json:"group_allow_list" typescript:",notnull"` GroupField clibase.String `json:"groups_field" typescript:",notnull"` GroupMapping clibase.Struct[map[string]string] `json:"group_mapping" typescript:",notnull"` UserRoleField clibase.String `json:"user_role_field" typescript:",notnull"` @@ -1187,6 +1188,16 @@ when required by your organization's security policy.`, Group: &deploymentGroupOIDC, YAML: "groupRegexFilter", }, + { + Name: "OIDC Allowed Groups", + Description: "If provided any group name not in the list will not be allowed to authenticate. This allows for restricting access to a specific set of groups. This filter is applied after the group mapping and before the regex filter.", + Flag: "oidc-allowed-groups", + Env: "CODER_OIDC_ALLOWED_GROUPS", + Default: "", + Value: &c.OIDC.GroupAllowList, + Group: &deploymentGroupOIDC, + YAML: "groupAllowed", + }, { Name: "OIDC User Role Field", Description: "This field must be set if using the user roles sync feature. Set this to the name of the claim used to store the user's role. The roles should be sent as an array of strings.", From 524aced3d87dabb715b7fb55263066ceab3b0677 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Wed, 6 Dec 2023 12:57:41 -0600 Subject: [PATCH 03/11] add unit test --- coderd/userauth.go | 10 +- coderd/userauth_test.go | 371 +++++++++++++++++---------------- site/src/api/typesGenerated.ts | 1 + 3 files changed, 194 insertions(+), 188 deletions(-) diff --git a/coderd/userauth.go b/coderd/userauth.go index 405e7cb580c8d..cf384939ea6e5 100644 --- a/coderd/userauth.go +++ b/coderd/userauth.go @@ -1061,11 +1061,15 @@ func (api *API) oidcGroups(ctx context.Context, mergedClaims map[string]interfac } if !inAllowList { + detail := fmt.Sprintf("Ask an administrator to add one of your groups to the whitelist: %s", strings.Join(groups, ", ")) + if len(groups) == 0 { + detail = "You are currently not a member of any groups!" + } return usingGroups, groups, &httpError{ code: http.StatusForbidden, - msg: "You aren't a member of an authorized group!", - detail: fmt.Sprintf("You must be a member of one of the following groups: %v", api.OIDCConfig.GroupAllowList), - renderStaticPage: false, + msg: "Not a member of an allowed group", + detail: detail, + renderStaticPage: true, } } } diff --git a/coderd/userauth_test.go b/coderd/userauth_test.go index fe6ded1e901b1..e553108ed661b 100644 --- a/coderd/userauth_test.go +++ b/coderd/userauth_test.go @@ -608,191 +608,192 @@ func TestUserOIDC(t *testing.T) { StatusCode int IgnoreEmailVerified bool IgnoreUserInfo bool - }{{ - Name: "EmailOnly", - IDTokenClaims: jwt.MapClaims{ - "email": "kyle@kwc.io", - }, - AllowSignups: true, - StatusCode: http.StatusOK, - Username: "kyle", - }, { - Name: "EmailNotVerified", - IDTokenClaims: jwt.MapClaims{ - "email": "kyle@kwc.io", - "email_verified": false, - }, - AllowSignups: true, - StatusCode: http.StatusForbidden, - }, { - Name: "EmailNotAString", - IDTokenClaims: jwt.MapClaims{ - "email": 3.14159, - "email_verified": false, - }, - AllowSignups: true, - StatusCode: http.StatusBadRequest, - }, { - Name: "EmailNotVerifiedIgnored", - IDTokenClaims: jwt.MapClaims{ - "email": "kyle@kwc.io", - "email_verified": false, - }, - AllowSignups: true, - StatusCode: http.StatusOK, - Username: "kyle", - IgnoreEmailVerified: true, - }, { - Name: "NotInRequiredEmailDomain", - IDTokenClaims: jwt.MapClaims{ - "email": "kyle@kwc.io", - "email_verified": true, - }, - AllowSignups: true, - EmailDomain: []string{ - "coder.com", - }, - StatusCode: http.StatusForbidden, - }, { - Name: "EmailDomainCaseInsensitive", - IDTokenClaims: jwt.MapClaims{ - "email": "kyle@KWC.io", - "email_verified": true, - }, - AllowSignups: true, - EmailDomain: []string{ - "kwc.io", - }, - StatusCode: http.StatusOK, - }, { - Name: "EmptyClaims", - IDTokenClaims: jwt.MapClaims{}, - AllowSignups: true, - StatusCode: http.StatusBadRequest, - }, { - Name: "NoSignups", - IDTokenClaims: jwt.MapClaims{ - "email": "kyle@kwc.io", - "email_verified": true, - }, - StatusCode: http.StatusForbidden, - }, { - Name: "UsernameFromEmail", - IDTokenClaims: jwt.MapClaims{ - "email": "kyle@kwc.io", - "email_verified": true, - }, - Username: "kyle", - AllowSignups: true, - StatusCode: http.StatusOK, - }, { - Name: "UsernameFromClaims", - IDTokenClaims: jwt.MapClaims{ - "email": "kyle@kwc.io", - "email_verified": true, - "preferred_username": "hotdog", - }, - Username: "hotdog", - AllowSignups: true, - StatusCode: http.StatusOK, - }, { - // Services like Okta return the email as the username: - // https://developer.okta.com/docs/reference/api/oidc/#base-claims-always-present - Name: "UsernameAsEmail", - IDTokenClaims: jwt.MapClaims{ - "email": "kyle@kwc.io", - "email_verified": true, - "preferred_username": "kyle@kwc.io", - }, - Username: "kyle", - AllowSignups: true, - StatusCode: http.StatusOK, - }, { - // See: https://github.com/coder/coder/issues/4472 - Name: "UsernameIsEmail", - IDTokenClaims: jwt.MapClaims{ - "preferred_username": "kyle@kwc.io", - }, - Username: "kyle", - AllowSignups: true, - StatusCode: http.StatusOK, - }, { - Name: "WithPicture", - IDTokenClaims: jwt.MapClaims{ - "email": "kyle@kwc.io", - "email_verified": true, - "preferred_username": "kyle", - "picture": "/example.png", - }, - Username: "kyle", - AllowSignups: true, - AvatarURL: "/example.png", - StatusCode: http.StatusOK, - }, { - Name: "WithUserInfoClaims", - IDTokenClaims: jwt.MapClaims{ - "email": "kyle@kwc.io", - "email_verified": true, - }, - UserInfoClaims: jwt.MapClaims{ - "preferred_username": "potato", - "picture": "/example.png", - }, - Username: "potato", - AllowSignups: true, - AvatarURL: "/example.png", - StatusCode: http.StatusOK, - }, { - Name: "GroupsDoesNothing", - IDTokenClaims: jwt.MapClaims{ - "email": "coolin@coder.com", - "groups": []string{"pingpong"}, - }, - AllowSignups: true, - StatusCode: http.StatusOK, - }, { - Name: "UserInfoOverridesIDTokenClaims", - IDTokenClaims: jwt.MapClaims{ - "email": "internaluser@internal.domain", - "email_verified": false, - }, - UserInfoClaims: jwt.MapClaims{ - "email": "externaluser@external.domain", - "email_verified": true, - "preferred_username": "user", - }, - Username: "user", - AllowSignups: true, - IgnoreEmailVerified: false, - StatusCode: http.StatusOK, - }, { - Name: "InvalidUserInfo", - IDTokenClaims: jwt.MapClaims{ - "email": "internaluser@internal.domain", - "email_verified": false, - }, - UserInfoClaims: jwt.MapClaims{ - "email": 1, - }, - AllowSignups: true, - IgnoreEmailVerified: false, - StatusCode: http.StatusInternalServerError, - }, { - Name: "IgnoreUserInfo", - IDTokenClaims: jwt.MapClaims{ - "email": "user@internal.domain", - "email_verified": true, - "preferred_username": "user", - }, - UserInfoClaims: jwt.MapClaims{ - "email": "user.mcname@external.domain", - "preferred_username": "Mr. User McName", - }, - Username: "user", - IgnoreUserInfo: true, - AllowSignups: true, - StatusCode: http.StatusOK, - }} { + }{ + { + Name: "EmailOnly", + IDTokenClaims: jwt.MapClaims{ + "email": "kyle@kwc.io", + }, + AllowSignups: true, + StatusCode: http.StatusOK, + Username: "kyle", + }, { + Name: "EmailNotVerified", + IDTokenClaims: jwt.MapClaims{ + "email": "kyle@kwc.io", + "email_verified": false, + }, + AllowSignups: true, + StatusCode: http.StatusForbidden, + }, { + Name: "EmailNotAString", + IDTokenClaims: jwt.MapClaims{ + "email": 3.14159, + "email_verified": false, + }, + AllowSignups: true, + StatusCode: http.StatusBadRequest, + }, { + Name: "EmailNotVerifiedIgnored", + IDTokenClaims: jwt.MapClaims{ + "email": "kyle@kwc.io", + "email_verified": false, + }, + AllowSignups: true, + StatusCode: http.StatusOK, + Username: "kyle", + IgnoreEmailVerified: true, + }, { + Name: "NotInRequiredEmailDomain", + IDTokenClaims: jwt.MapClaims{ + "email": "kyle@kwc.io", + "email_verified": true, + }, + AllowSignups: true, + EmailDomain: []string{ + "coder.com", + }, + StatusCode: http.StatusForbidden, + }, { + Name: "EmailDomainCaseInsensitive", + IDTokenClaims: jwt.MapClaims{ + "email": "kyle@KWC.io", + "email_verified": true, + }, + AllowSignups: true, + EmailDomain: []string{ + "kwc.io", + }, + StatusCode: http.StatusOK, + }, { + Name: "EmptyClaims", + IDTokenClaims: jwt.MapClaims{}, + AllowSignups: true, + StatusCode: http.StatusBadRequest, + }, { + Name: "NoSignups", + IDTokenClaims: jwt.MapClaims{ + "email": "kyle@kwc.io", + "email_verified": true, + }, + StatusCode: http.StatusForbidden, + }, { + Name: "UsernameFromEmail", + IDTokenClaims: jwt.MapClaims{ + "email": "kyle@kwc.io", + "email_verified": true, + }, + Username: "kyle", + AllowSignups: true, + StatusCode: http.StatusOK, + }, { + Name: "UsernameFromClaims", + IDTokenClaims: jwt.MapClaims{ + "email": "kyle@kwc.io", + "email_verified": true, + "preferred_username": "hotdog", + }, + Username: "hotdog", + AllowSignups: true, + StatusCode: http.StatusOK, + }, { + // Services like Okta return the email as the username: + // https://developer.okta.com/docs/reference/api/oidc/#base-claims-always-present + Name: "UsernameAsEmail", + IDTokenClaims: jwt.MapClaims{ + "email": "kyle@kwc.io", + "email_verified": true, + "preferred_username": "kyle@kwc.io", + }, + Username: "kyle", + AllowSignups: true, + StatusCode: http.StatusOK, + }, { + // See: https://github.com/coder/coder/issues/4472 + Name: "UsernameIsEmail", + IDTokenClaims: jwt.MapClaims{ + "preferred_username": "kyle@kwc.io", + }, + Username: "kyle", + AllowSignups: true, + StatusCode: http.StatusOK, + }, { + Name: "WithPicture", + IDTokenClaims: jwt.MapClaims{ + "email": "kyle@kwc.io", + "email_verified": true, + "preferred_username": "kyle", + "picture": "/example.png", + }, + Username: "kyle", + AllowSignups: true, + AvatarURL: "/example.png", + StatusCode: http.StatusOK, + }, { + Name: "WithUserInfoClaims", + IDTokenClaims: jwt.MapClaims{ + "email": "kyle@kwc.io", + "email_verified": true, + }, + UserInfoClaims: jwt.MapClaims{ + "preferred_username": "potato", + "picture": "/example.png", + }, + Username: "potato", + AllowSignups: true, + AvatarURL: "/example.png", + StatusCode: http.StatusOK, + }, { + Name: "GroupsDoesNothing", + IDTokenClaims: jwt.MapClaims{ + "email": "coolin@coder.com", + "groups": []string{"pingpong"}, + }, + AllowSignups: true, + StatusCode: http.StatusOK, + }, { + Name: "UserInfoOverridesIDTokenClaims", + IDTokenClaims: jwt.MapClaims{ + "email": "internaluser@internal.domain", + "email_verified": false, + }, + UserInfoClaims: jwt.MapClaims{ + "email": "externaluser@external.domain", + "email_verified": true, + "preferred_username": "user", + }, + Username: "user", + AllowSignups: true, + IgnoreEmailVerified: false, + StatusCode: http.StatusOK, + }, { + Name: "InvalidUserInfo", + IDTokenClaims: jwt.MapClaims{ + "email": "internaluser@internal.domain", + "email_verified": false, + }, + UserInfoClaims: jwt.MapClaims{ + "email": 1, + }, + AllowSignups: true, + IgnoreEmailVerified: false, + StatusCode: http.StatusInternalServerError, + }, { + Name: "IgnoreUserInfo", + IDTokenClaims: jwt.MapClaims{ + "email": "user@internal.domain", + "email_verified": true, + "preferred_username": "user", + }, + UserInfoClaims: jwt.MapClaims{ + "email": "user.mcname@external.domain", + "preferred_username": "Mr. User McName", + }, + Username: "user", + IgnoreUserInfo: true, + AllowSignups: true, + StatusCode: http.StatusOK, + }} { tc := tc t.Run(tc.Name, func(t *testing.T) { t.Parallel() diff --git a/site/src/api/typesGenerated.ts b/site/src/api/typesGenerated.ts index af34c7d602e4e..dedb421dab568 100644 --- a/site/src/api/typesGenerated.ts +++ b/site/src/api/typesGenerated.ts @@ -689,6 +689,7 @@ export interface OIDCConfig { readonly ignore_user_info: boolean; readonly group_auto_create: boolean; readonly group_regex_filter: string; + readonly group_allow_list: string[]; readonly groups_field: string; readonly group_mapping: Record; readonly user_role_field: string; From 75d120277550883ef393aed9ec2bcfb9056a0431 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Wed, 6 Dec 2023 15:47:25 -0600 Subject: [PATCH 04/11] revert test --- coderd/userauth_test.go | 371 ++++++++++++++++++++-------------------- 1 file changed, 185 insertions(+), 186 deletions(-) diff --git a/coderd/userauth_test.go b/coderd/userauth_test.go index e553108ed661b..fe6ded1e901b1 100644 --- a/coderd/userauth_test.go +++ b/coderd/userauth_test.go @@ -608,192 +608,191 @@ func TestUserOIDC(t *testing.T) { StatusCode int IgnoreEmailVerified bool IgnoreUserInfo bool - }{ - { - Name: "EmailOnly", - IDTokenClaims: jwt.MapClaims{ - "email": "kyle@kwc.io", - }, - AllowSignups: true, - StatusCode: http.StatusOK, - Username: "kyle", - }, { - Name: "EmailNotVerified", - IDTokenClaims: jwt.MapClaims{ - "email": "kyle@kwc.io", - "email_verified": false, - }, - AllowSignups: true, - StatusCode: http.StatusForbidden, - }, { - Name: "EmailNotAString", - IDTokenClaims: jwt.MapClaims{ - "email": 3.14159, - "email_verified": false, - }, - AllowSignups: true, - StatusCode: http.StatusBadRequest, - }, { - Name: "EmailNotVerifiedIgnored", - IDTokenClaims: jwt.MapClaims{ - "email": "kyle@kwc.io", - "email_verified": false, - }, - AllowSignups: true, - StatusCode: http.StatusOK, - Username: "kyle", - IgnoreEmailVerified: true, - }, { - Name: "NotInRequiredEmailDomain", - IDTokenClaims: jwt.MapClaims{ - "email": "kyle@kwc.io", - "email_verified": true, - }, - AllowSignups: true, - EmailDomain: []string{ - "coder.com", - }, - StatusCode: http.StatusForbidden, - }, { - Name: "EmailDomainCaseInsensitive", - IDTokenClaims: jwt.MapClaims{ - "email": "kyle@KWC.io", - "email_verified": true, - }, - AllowSignups: true, - EmailDomain: []string{ - "kwc.io", - }, - StatusCode: http.StatusOK, - }, { - Name: "EmptyClaims", - IDTokenClaims: jwt.MapClaims{}, - AllowSignups: true, - StatusCode: http.StatusBadRequest, - }, { - Name: "NoSignups", - IDTokenClaims: jwt.MapClaims{ - "email": "kyle@kwc.io", - "email_verified": true, - }, - StatusCode: http.StatusForbidden, - }, { - Name: "UsernameFromEmail", - IDTokenClaims: jwt.MapClaims{ - "email": "kyle@kwc.io", - "email_verified": true, - }, - Username: "kyle", - AllowSignups: true, - StatusCode: http.StatusOK, - }, { - Name: "UsernameFromClaims", - IDTokenClaims: jwt.MapClaims{ - "email": "kyle@kwc.io", - "email_verified": true, - "preferred_username": "hotdog", - }, - Username: "hotdog", - AllowSignups: true, - StatusCode: http.StatusOK, - }, { - // Services like Okta return the email as the username: - // https://developer.okta.com/docs/reference/api/oidc/#base-claims-always-present - Name: "UsernameAsEmail", - IDTokenClaims: jwt.MapClaims{ - "email": "kyle@kwc.io", - "email_verified": true, - "preferred_username": "kyle@kwc.io", - }, - Username: "kyle", - AllowSignups: true, - StatusCode: http.StatusOK, - }, { - // See: https://github.com/coder/coder/issues/4472 - Name: "UsernameIsEmail", - IDTokenClaims: jwt.MapClaims{ - "preferred_username": "kyle@kwc.io", - }, - Username: "kyle", - AllowSignups: true, - StatusCode: http.StatusOK, - }, { - Name: "WithPicture", - IDTokenClaims: jwt.MapClaims{ - "email": "kyle@kwc.io", - "email_verified": true, - "preferred_username": "kyle", - "picture": "/example.png", - }, - Username: "kyle", - AllowSignups: true, - AvatarURL: "/example.png", - StatusCode: http.StatusOK, - }, { - Name: "WithUserInfoClaims", - IDTokenClaims: jwt.MapClaims{ - "email": "kyle@kwc.io", - "email_verified": true, - }, - UserInfoClaims: jwt.MapClaims{ - "preferred_username": "potato", - "picture": "/example.png", - }, - Username: "potato", - AllowSignups: true, - AvatarURL: "/example.png", - StatusCode: http.StatusOK, - }, { - Name: "GroupsDoesNothing", - IDTokenClaims: jwt.MapClaims{ - "email": "coolin@coder.com", - "groups": []string{"pingpong"}, - }, - AllowSignups: true, - StatusCode: http.StatusOK, - }, { - Name: "UserInfoOverridesIDTokenClaims", - IDTokenClaims: jwt.MapClaims{ - "email": "internaluser@internal.domain", - "email_verified": false, - }, - UserInfoClaims: jwt.MapClaims{ - "email": "externaluser@external.domain", - "email_verified": true, - "preferred_username": "user", - }, - Username: "user", - AllowSignups: true, - IgnoreEmailVerified: false, - StatusCode: http.StatusOK, - }, { - Name: "InvalidUserInfo", - IDTokenClaims: jwt.MapClaims{ - "email": "internaluser@internal.domain", - "email_verified": false, - }, - UserInfoClaims: jwt.MapClaims{ - "email": 1, - }, - AllowSignups: true, - IgnoreEmailVerified: false, - StatusCode: http.StatusInternalServerError, - }, { - Name: "IgnoreUserInfo", - IDTokenClaims: jwt.MapClaims{ - "email": "user@internal.domain", - "email_verified": true, - "preferred_username": "user", - }, - UserInfoClaims: jwt.MapClaims{ - "email": "user.mcname@external.domain", - "preferred_username": "Mr. User McName", - }, - Username: "user", - IgnoreUserInfo: true, - AllowSignups: true, - StatusCode: http.StatusOK, - }} { + }{{ + Name: "EmailOnly", + IDTokenClaims: jwt.MapClaims{ + "email": "kyle@kwc.io", + }, + AllowSignups: true, + StatusCode: http.StatusOK, + Username: "kyle", + }, { + Name: "EmailNotVerified", + IDTokenClaims: jwt.MapClaims{ + "email": "kyle@kwc.io", + "email_verified": false, + }, + AllowSignups: true, + StatusCode: http.StatusForbidden, + }, { + Name: "EmailNotAString", + IDTokenClaims: jwt.MapClaims{ + "email": 3.14159, + "email_verified": false, + }, + AllowSignups: true, + StatusCode: http.StatusBadRequest, + }, { + Name: "EmailNotVerifiedIgnored", + IDTokenClaims: jwt.MapClaims{ + "email": "kyle@kwc.io", + "email_verified": false, + }, + AllowSignups: true, + StatusCode: http.StatusOK, + Username: "kyle", + IgnoreEmailVerified: true, + }, { + Name: "NotInRequiredEmailDomain", + IDTokenClaims: jwt.MapClaims{ + "email": "kyle@kwc.io", + "email_verified": true, + }, + AllowSignups: true, + EmailDomain: []string{ + "coder.com", + }, + StatusCode: http.StatusForbidden, + }, { + Name: "EmailDomainCaseInsensitive", + IDTokenClaims: jwt.MapClaims{ + "email": "kyle@KWC.io", + "email_verified": true, + }, + AllowSignups: true, + EmailDomain: []string{ + "kwc.io", + }, + StatusCode: http.StatusOK, + }, { + Name: "EmptyClaims", + IDTokenClaims: jwt.MapClaims{}, + AllowSignups: true, + StatusCode: http.StatusBadRequest, + }, { + Name: "NoSignups", + IDTokenClaims: jwt.MapClaims{ + "email": "kyle@kwc.io", + "email_verified": true, + }, + StatusCode: http.StatusForbidden, + }, { + Name: "UsernameFromEmail", + IDTokenClaims: jwt.MapClaims{ + "email": "kyle@kwc.io", + "email_verified": true, + }, + Username: "kyle", + AllowSignups: true, + StatusCode: http.StatusOK, + }, { + Name: "UsernameFromClaims", + IDTokenClaims: jwt.MapClaims{ + "email": "kyle@kwc.io", + "email_verified": true, + "preferred_username": "hotdog", + }, + Username: "hotdog", + AllowSignups: true, + StatusCode: http.StatusOK, + }, { + // Services like Okta return the email as the username: + // https://developer.okta.com/docs/reference/api/oidc/#base-claims-always-present + Name: "UsernameAsEmail", + IDTokenClaims: jwt.MapClaims{ + "email": "kyle@kwc.io", + "email_verified": true, + "preferred_username": "kyle@kwc.io", + }, + Username: "kyle", + AllowSignups: true, + StatusCode: http.StatusOK, + }, { + // See: https://github.com/coder/coder/issues/4472 + Name: "UsernameIsEmail", + IDTokenClaims: jwt.MapClaims{ + "preferred_username": "kyle@kwc.io", + }, + Username: "kyle", + AllowSignups: true, + StatusCode: http.StatusOK, + }, { + Name: "WithPicture", + IDTokenClaims: jwt.MapClaims{ + "email": "kyle@kwc.io", + "email_verified": true, + "preferred_username": "kyle", + "picture": "/example.png", + }, + Username: "kyle", + AllowSignups: true, + AvatarURL: "/example.png", + StatusCode: http.StatusOK, + }, { + Name: "WithUserInfoClaims", + IDTokenClaims: jwt.MapClaims{ + "email": "kyle@kwc.io", + "email_verified": true, + }, + UserInfoClaims: jwt.MapClaims{ + "preferred_username": "potato", + "picture": "/example.png", + }, + Username: "potato", + AllowSignups: true, + AvatarURL: "/example.png", + StatusCode: http.StatusOK, + }, { + Name: "GroupsDoesNothing", + IDTokenClaims: jwt.MapClaims{ + "email": "coolin@coder.com", + "groups": []string{"pingpong"}, + }, + AllowSignups: true, + StatusCode: http.StatusOK, + }, { + Name: "UserInfoOverridesIDTokenClaims", + IDTokenClaims: jwt.MapClaims{ + "email": "internaluser@internal.domain", + "email_verified": false, + }, + UserInfoClaims: jwt.MapClaims{ + "email": "externaluser@external.domain", + "email_verified": true, + "preferred_username": "user", + }, + Username: "user", + AllowSignups: true, + IgnoreEmailVerified: false, + StatusCode: http.StatusOK, + }, { + Name: "InvalidUserInfo", + IDTokenClaims: jwt.MapClaims{ + "email": "internaluser@internal.domain", + "email_verified": false, + }, + UserInfoClaims: jwt.MapClaims{ + "email": 1, + }, + AllowSignups: true, + IgnoreEmailVerified: false, + StatusCode: http.StatusInternalServerError, + }, { + Name: "IgnoreUserInfo", + IDTokenClaims: jwt.MapClaims{ + "email": "user@internal.domain", + "email_verified": true, + "preferred_username": "user", + }, + UserInfoClaims: jwt.MapClaims{ + "email": "user.mcname@external.domain", + "preferred_username": "Mr. User McName", + }, + Username: "user", + IgnoreUserInfo: true, + AllowSignups: true, + StatusCode: http.StatusOK, + }} { tc := tc t.Run(tc.Name, func(t *testing.T) { t.Parallel() From 217aa5468b6e8ea28a42901868f8e55972a32ffe Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Wed, 6 Dec 2023 15:54:54 -0600 Subject: [PATCH 05/11] Add unit test --- cli/server.go | 4 +++ coderd/coderdtest/oidctest/helper.go | 8 ++++++ enterprise/coderd/userauth_test.go | 43 ++++++++++++++++++++++++---- 3 files changed, 50 insertions(+), 5 deletions(-) diff --git a/cli/server.go b/cli/server.go index 93989f16010e0..26f8f46056762 100644 --- a/cli/server.go +++ b/cli/server.go @@ -147,6 +147,10 @@ func createOIDCConfig(ctx context.Context, vals *codersdk.DeploymentValues) (*co } useCfg = pkiCfg } + if len(vals.OIDC.GroupAllowList) > 0 && vals.OIDC.GroupField == "" { + return nil, xerrors.Errorf("'oidc-group-field' must be set if 'oidc-allowed-groups' is set. Either unset 'oidc-allowed-groups' or set 'oidc-group-field'") + } + return &coderd.OIDCConfig{ OAuth2Config: useCfg, Provider: oidcProvider, diff --git a/coderd/coderdtest/oidctest/helper.go b/coderd/coderdtest/oidctest/helper.go index 1c434f1fcdf34..abf29d4fa2b46 100644 --- a/coderd/coderdtest/oidctest/helper.go +++ b/coderd/coderdtest/oidctest/helper.go @@ -48,6 +48,14 @@ func (h *LoginHelper) Login(t *testing.T, idTokenClaims jwt.MapClaims) (*codersd return h.fake.Login(t, unauthenticatedClient, idTokenClaims) } +// AttemptLogin does not assert a successful login. +func (h *LoginHelper) AttemptLogin(t *testing.T, idTokenClaims jwt.MapClaims) (*codersdk.Client, *http.Response) { + t.Helper() + unauthenticatedClient := codersdk.New(h.client.URL) + + return h.fake.AttemptLogin(t, unauthenticatedClient, idTokenClaims) +} + // ExpireOauthToken expires the oauth token for the given user. func (*LoginHelper) ExpireOauthToken(t *testing.T, db database.Store, user *codersdk.Client) database.UserLink { t.Helper() diff --git a/enterprise/coderd/userauth_test.go b/enterprise/coderd/userauth_test.go index 70e63f6a1e3b6..fed52b9f4dd24 100644 --- a/enterprise/coderd/userauth_test.go +++ b/enterprise/coderd/userauth_test.go @@ -387,6 +387,37 @@ func TestUserOIDC(t *testing.T) { require.Equal(t, http.StatusOK, resp.StatusCode) runner.AssertGroups(t, "alice", []string{groupName}) }) + + t.Run("GroupAllowList", func(t *testing.T) { + t.Parallel() + + const groupClaim = "custom-groups" + const allowedGroup = "foo" + runner := setupOIDCTest(t, oidcTestConfig{ + Config: func(cfg *coderd.OIDCConfig) { + cfg.AllowSignups = true + cfg.GroupField = groupClaim + cfg.GroupAllowList = []string{allowedGroup} + }, + }) + + // Test forbidden + _, resp := runner.AttemptLogin(t, jwt.MapClaims{ + "email": "alice@coder.com", + groupClaim: []string{"not-allowed"}, + }) + require.Equal(t, http.StatusForbidden, resp.StatusCode) + + // Test allowed + client, _ := runner.Login(t, jwt.MapClaims{ + "email": "alice@coder.com", + groupClaim: []string{allowedGroup}, + }) + + ctx := testutil.Context(t, testutil.WaitShort) + _, err := client.User(ctx, codersdk.Me) + require.NoError(t, err) + }) }) t.Run("Refresh", func(t *testing.T) { @@ -661,7 +692,8 @@ type oidcTestRunner struct { // Login will call the OIDC flow with an unauthenticated client. // The IDP will return the idToken claims. - Login func(t *testing.T, idToken jwt.MapClaims) (*codersdk.Client, *http.Response) + Login func(t *testing.T, idToken jwt.MapClaims) (*codersdk.Client, *http.Response) + AttemptLogin func(t *testing.T, idToken jwt.MapClaims) (*codersdk.Client, *http.Response) // ForceRefresh will use an authenticated codersdk.Client, and force their // OIDC token to be expired and require a refresh. The refresh will use the claims provided. // It just calls the /users/me endpoint to trigger the refresh. @@ -751,10 +783,11 @@ func setupOIDCTest(t *testing.T, settings oidcTestConfig) *oidcTestRunner { helper := oidctest.NewLoginHelper(owner, fake) return &oidcTestRunner{ - AdminClient: owner, - AdminUser: admin, - API: api, - Login: helper.Login, + AdminClient: owner, + AdminUser: admin, + API: api, + Login: helper.Login, + AttemptLogin: helper.AttemptLogin, ForceRefresh: func(t *testing.T, client *codersdk.Client, idToken jwt.MapClaims) { helper.ForceRefresh(t, api.Database, client, idToken) }, From 8a871d737fd091002fb4f8774e1acdef73a4fecc Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Wed, 6 Dec 2023 16:20:07 -0600 Subject: [PATCH 06/11] update golden files --- cli/testdata/coder_server_--help.golden | 6 ++++++ cli/testdata/server-config.yaml.golden | 5 +++++ coderd/userauth.go | 4 ++-- 3 files changed, 13 insertions(+), 2 deletions(-) diff --git a/cli/testdata/coder_server_--help.golden b/cli/testdata/coder_server_--help.golden index 4d1b9609aa6f4..72c1c2bee7334 100644 --- a/cli/testdata/coder_server_--help.golden +++ b/cli/testdata/coder_server_--help.golden @@ -333,6 +333,12 @@ OIDC OPTIONS: --oidc-allow-signups bool, $CODER_OIDC_ALLOW_SIGNUPS (default: true) Whether new users can sign up with OIDC. + --oidc-allowed-groups string-array, $CODER_OIDC_ALLOWED_GROUPS + If provided any group name not in the list will not be allowed to + authenticate. This allows for restricting access to a specific set of + groups. This filter is applied after the group mapping and before the + regex filter. + --oidc-auth-url-params struct[map[string]string], $CODER_OIDC_AUTH_URL_PARAMS (default: {"access_type": "offline"}) OIDC auth URL parameters to pass to the upstream provider. diff --git a/cli/testdata/server-config.yaml.golden b/cli/testdata/server-config.yaml.golden index 3163a6e2cab33..2346f73d112d1 100644 --- a/cli/testdata/server-config.yaml.golden +++ b/cli/testdata/server-config.yaml.golden @@ -323,6 +323,11 @@ oidc: # mapping. # (default: .*, type: regexp) groupRegexFilter: .* + # If provided any group name not in the list will not be allowed to authenticate. + # This allows for restricting access to a specific set of groups. This filter is + # applied after the group mapping and before the regex filter. + # (default: , type: string-array) + groupAllowed: [] # This field must be set if using the user roles sync feature. Set this to the # name of the claim used to store the user's role. The roles should be sent as an # array of strings. diff --git a/coderd/userauth.go b/coderd/userauth.go index cf384939ea6e5..322ad382eb65d 100644 --- a/coderd/userauth.go +++ b/coderd/userauth.go @@ -1061,9 +1061,9 @@ func (api *API) oidcGroups(ctx context.Context, mergedClaims map[string]interfac } if !inAllowList { - detail := fmt.Sprintf("Ask an administrator to add one of your groups to the whitelist: %s", strings.Join(groups, ", ")) + detail := fmt.Sprintf("Ask an administrator to add one of your groups (%s) to the whitelist", strings.Join(groups, ", ")) if len(groups) == 0 { - detail = "You are currently not a member of any groups!" + detail = "You are currently not a member of any groups! Ask an administrator to add you to an authorized group to login." } return usingGroups, groups, &httpError{ code: http.StatusForbidden, From a9a740af32350bc0e13fadd2b852e7e2576f4871 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Wed, 6 Dec 2023 16:22:42 -0600 Subject: [PATCH 07/11] Make gen --- coderd/apidoc/docs.go | 6 ++++++ coderd/apidoc/swagger.json | 6 ++++++ docs/api/general.md | 1 + docs/api/schemas.md | 4 ++++ docs/cli/server.md | 10 ++++++++++ 5 files changed, 27 insertions(+) diff --git a/coderd/apidoc/docs.go b/coderd/apidoc/docs.go index 8b9a47c224549..40d48562d47c6 100644 --- a/coderd/apidoc/docs.go +++ b/coderd/apidoc/docs.go @@ -9408,6 +9408,12 @@ const docTemplate = `{ "email_field": { "type": "string" }, + "group_allow_list": { + "type": "array", + "items": { + "type": "string" + } + }, "group_auto_create": { "type": "boolean" }, diff --git a/coderd/apidoc/swagger.json b/coderd/apidoc/swagger.json index 85fe382918ec4..dc6f847e5b46a 100644 --- a/coderd/apidoc/swagger.json +++ b/coderd/apidoc/swagger.json @@ -8445,6 +8445,12 @@ "email_field": { "type": "string" }, + "group_allow_list": { + "type": "array", + "items": { + "type": "string" + } + }, "group_auto_create": { "type": "boolean" }, diff --git a/docs/api/general.md b/docs/api/general.md index de89e07e558c5..8aa124a18368f 100644 --- a/docs/api/general.md +++ b/docs/api/general.md @@ -271,6 +271,7 @@ curl -X GET http://coder-server:8080/api/v2/deployment/config \ "client_secret": "string", "email_domain": ["string"], "email_field": "string", + "group_allow_list": ["string"], "group_auto_create": true, "group_mapping": {}, "group_regex_filter": {}, diff --git a/docs/api/schemas.md b/docs/api/schemas.md index bcf54626d4952..0ccafbea41872 100644 --- a/docs/api/schemas.md +++ b/docs/api/schemas.md @@ -2196,6 +2196,7 @@ AuthorizationObject can represent a "set" of objects, such as: all workspaces in "client_secret": "string", "email_domain": ["string"], "email_field": "string", + "group_allow_list": ["string"], "group_auto_create": true, "group_mapping": {}, "group_regex_filter": {}, @@ -2571,6 +2572,7 @@ AuthorizationObject can represent a "set" of objects, such as: all workspaces in "client_secret": "string", "email_domain": ["string"], "email_field": "string", + "group_allow_list": ["string"], "group_auto_create": true, "group_mapping": {}, "group_regex_filter": {}, @@ -3577,6 +3579,7 @@ AuthorizationObject can represent a "set" of objects, such as: all workspaces in "client_secret": "string", "email_domain": ["string"], "email_field": "string", + "group_allow_list": ["string"], "group_auto_create": true, "group_mapping": {}, "group_regex_filter": {}, @@ -3618,6 +3621,7 @@ AuthorizationObject can represent a "set" of objects, such as: all workspaces in | `client_secret` | string | false | | | | `email_domain` | array of string | false | | | | `email_field` | string | false | | | +| `group_allow_list` | array of string | false | | | | `group_auto_create` | boolean | false | | | | `group_mapping` | object | false | | | | `group_regex_filter` | [clibase.Regexp](#clibaseregexp) | false | | | diff --git a/docs/cli/server.md b/docs/cli/server.md index 3fb0c57b3ee48..2b700e09568f8 100644 --- a/docs/cli/server.md +++ b/docs/cli/server.md @@ -449,6 +449,16 @@ Base URL of a GitHub Enterprise deployment to use for Login with GitHub. Whether new users can sign up with OIDC. +### --oidc-allowed-groups + +| | | +| ----------- | --------------------------------------- | +| Type | string-array | +| Environment | $CODER_OIDC_ALLOWED_GROUPS | +| YAML | oidc.groupAllowed | + +If provided any group name not in the list will not be allowed to authenticate. This allows for restricting access to a specific set of groups. This filter is applied after the group mapping and before the regex filter. + ### --oidc-auth-url-params | | | From 80208a74e97d88a370b4d57e1a3864676124ee5a Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Thu, 7 Dec 2023 08:32:10 -0600 Subject: [PATCH 08/11] make group allowlist a map in the outright --- cli/server.go | 7 ++++++- coderd/userauth.go | 11 +++-------- 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/cli/server.go b/cli/server.go index 26f8f46056762..8b325e409078d 100644 --- a/cli/server.go +++ b/cli/server.go @@ -151,6 +151,11 @@ func createOIDCConfig(ctx context.Context, vals *codersdk.DeploymentValues) (*co return nil, xerrors.Errorf("'oidc-group-field' must be set if 'oidc-allowed-groups' is set. Either unset 'oidc-allowed-groups' or set 'oidc-group-field'") } + groupAllowList := make(map[string]bool) + for _, group := range vals.OIDC.GroupAllowList.Value() { + groupAllowList[group] = true + } + return &coderd.OIDCConfig{ OAuth2Config: useCfg, Provider: oidcProvider, @@ -165,7 +170,7 @@ func createOIDCConfig(ctx context.Context, vals *codersdk.DeploymentValues) (*co IgnoreUserInfo: vals.OIDC.IgnoreUserInfo.Value(), GroupField: vals.OIDC.GroupField.String(), GroupFilter: vals.OIDC.GroupRegexFilter.Value(), - GroupAllowList: vals.OIDC.GroupAllowList.Value(), + GroupAllowList: groupAllowList, CreateMissingGroups: vals.OIDC.GroupAutoCreate.Value(), GroupMapping: vals.OIDC.GroupMapping.Value, UserRoleField: vals.OIDC.UserRoleField.String(), diff --git a/coderd/userauth.go b/coderd/userauth.go index 322ad382eb65d..5f584a416f5bd 100644 --- a/coderd/userauth.go +++ b/coderd/userauth.go @@ -704,7 +704,7 @@ type OIDCConfig struct { // GroupAllowList is a list of groups that are allowed to log in. // If the list length is 0, then the allow list will not be applied and // this feature is disabled. - GroupAllowList []string + GroupAllowList map[string]bool // GroupMapping controls how groups returned by the OIDC provider get mapped // to groups within Coder. // map[oidcGroupName]coderGroupName @@ -1018,14 +1018,9 @@ func (api *API) oidcGroups(ctx context.Context, mergedClaims map[string]interfac // If the GroupField is the empty string, then groups from OIDC are not used. // This is so we can support manual group assignment. if api.OIDCConfig.GroupField != "" { - // allow list is a map of groups that are allowed to log in. - allowed := make(map[string]bool) - for _, group := range api.OIDCConfig.GroupAllowList { - allowed[group] = true - } // If the allow list is empty, then the user is allowed to log in. // Otherwise, they must belong to at least 1 group in the allow list. - inAllowList := len(allowed) == 0 + inAllowList := len(api.OIDCConfig.GroupAllowList) == 0 usingGroups = true groupsRaw, ok := mergedClaims[api.OIDCConfig.GroupField] @@ -1053,7 +1048,7 @@ func (api *API) oidcGroups(ctx context.Context, mergedClaims map[string]interfac if mappedGroup, ok := api.OIDCConfig.GroupMapping[group]; ok { group = mappedGroup } - if _, ok := allowed[group]; ok { + if _, ok := api.OIDCConfig.GroupAllowList[group]; ok { inAllowList = true } groups = append(groups, group) From 58551efeb38015229d4bd0054cc6a640a923a794 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Thu, 7 Dec 2023 08:35:49 -0600 Subject: [PATCH 09/11] Drop log line on failed login due to group allowlist --- coderd/userauth.go | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/coderd/userauth.go b/coderd/userauth.go index 5f584a416f5bd..ba8f69a68d1a8 100644 --- a/coderd/userauth.go +++ b/coderd/userauth.go @@ -929,6 +929,7 @@ func (api *API) userOIDC(rw http.ResponseWriter, r *http.Request) { picture, _ = pictureRaw.(string) } + ctx = slog.With(ctx, slog.F("email", email), slog.F("username", username)) usingGroups, groups, groupErr := api.oidcGroups(ctx, mergedClaims) if groupErr != nil { groupErr.Write(rw, r) @@ -1056,7 +1057,11 @@ func (api *API) oidcGroups(ctx context.Context, mergedClaims map[string]interfac } if !inAllowList { - detail := fmt.Sprintf("Ask an administrator to add one of your groups (%s) to the whitelist", strings.Join(groups, ", ")) + logger.Debug(ctx, "oidc group claim not in allow list, rejecting login", + slog.F("allow_list_count", len(api.OIDCConfig.GroupAllowList)), + slog.F("user_group_count", len(groups)), + ) + detail := "Ask an administrator to add one of your groups to the whitelist" if len(groups) == 0 { detail = "You are currently not a member of any groups! Ask an administrator to add you to an authorized group to login." } From 586a67ca404c5194a569bc261456357b165564e5 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Thu, 7 Dec 2023 12:53:38 -0600 Subject: [PATCH 10/11] Forgot to update and fix the unit test --- enterprise/coderd/userauth_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/enterprise/coderd/userauth_test.go b/enterprise/coderd/userauth_test.go index fed52b9f4dd24..bb3b37e943101 100644 --- a/enterprise/coderd/userauth_test.go +++ b/enterprise/coderd/userauth_test.go @@ -397,7 +397,7 @@ func TestUserOIDC(t *testing.T) { Config: func(cfg *coderd.OIDCConfig) { cfg.AllowSignups = true cfg.GroupField = groupClaim - cfg.GroupAllowList = []string{allowedGroup} + cfg.GroupAllowList = map[string]bool{allowedGroup: true} }, }) From 025b48d8a36d718fe3bb78d94f80295976495585 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Thu, 7 Dec 2023 16:41:01 -0600 Subject: [PATCH 11/11] Update golden files --- enterprise/cli/testdata/coder_server_--help.golden | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/enterprise/cli/testdata/coder_server_--help.golden b/enterprise/cli/testdata/coder_server_--help.golden index 85c924474c206..6997e74260317 100644 --- a/enterprise/cli/testdata/coder_server_--help.golden +++ b/enterprise/cli/testdata/coder_server_--help.golden @@ -334,6 +334,12 @@ OIDC OPTIONS: --oidc-allow-signups bool, $CODER_OIDC_ALLOW_SIGNUPS (default: true) Whether new users can sign up with OIDC. + --oidc-allowed-groups string-array, $CODER_OIDC_ALLOWED_GROUPS + If provided any group name not in the list will not be allowed to + authenticate. This allows for restricting access to a specific set of + groups. This filter is applied after the group mapping and before the + regex filter. + --oidc-auth-url-params struct[map[string]string], $CODER_OIDC_AUTH_URL_PARAMS (default: {"access_type": "offline"}) OIDC auth URL parameters to pass to the upstream provider.