Skip to content

Commit 7f25d31

Browse files
EmyrkEmyrk
authored
feat: Allow changing the 'group' oidc claim field (coder#6546)
* feat: Allow changing the 'group' oidc claim field * Enable empty groups support * fix: Delete was wiping all groups, not just the single user's groups * Update docs * fix: Dbfake delete group member fixed
1 parent 11a930e commit 7f25d31

File tree

14 files changed

+169
-45
lines changed

14 files changed

+169
-45
lines changed

‎cli/server.go

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -75,6 +75,7 @@ import (
7575
"github.com/coder/coder/coderd/telemetry"
7676
"github.com/coder/coder/coderd/tracing"
7777
"github.com/coder/coder/coderd/updatecheck"
78+
"github.com/coder/coder/coderd/util/slice"
7879
"github.com/coder/coder/codersdk"
7980
"github.com/coder/coder/cryptorand"
8081
"github.com/coder/coder/provisioner/echo"
@@ -765,6 +766,11 @@ flags, and YAML configuration. The precedence is as follows:
765766
if err != nil {
766767
return xerrors.Errorf("parse oidc oauth callback url: %w", err)
767768
}
769+
// If the scopes contain 'groups', we enable group support.
770+
// Do not override any custom value set by the user.
771+
if slice.Contains(cfg.OIDC.Scopes, "groups") && cfg.OIDC.GroupField == "" {
772+
cfg.OIDC.GroupField = "groups"
773+
}
768774
options.OIDCConfig = &coderd.OIDCConfig{
769775
OAuth2Config: &oauth2.Config{
770776
ClientID: cfg.OIDC.ClientID.String(),
@@ -780,6 +786,7 @@ flags, and YAML configuration. The precedence is as follows:
780786
EmailDomain: cfg.OIDC.EmailDomain,
781787
AllowSignups: cfg.OIDC.AllowSignups.Value(),
782788
UsernameField: cfg.OIDC.UsernameField.String(),
789+
GroupField: cfg.OIDC.GroupField.String(),
783790
SignInText: cfg.OIDC.SignInText.String(),
784791
IconURL: cfg.OIDC.IconURL.String(),
785792
IgnoreEmailVerified: cfg.OIDC.IgnoreEmailVerified.Value(),

‎coderd/apidoc/docs.go

Lines changed: 3 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎coderd/apidoc/swagger.json

Lines changed: 3 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎coderd/coderdtest/coderdtest.go

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -939,7 +939,7 @@ func (o *OIDCConfig) EncodeClaims(t *testing.T, claims jwt.MapClaims) string {
939939
return base64.StdEncoding.EncodeToString([]byte(signed))
940940
}
941941

942-
func (o *OIDCConfig) OIDCConfig(t *testing.T, userInfoClaims jwt.MapClaims) *coderd.OIDCConfig {
942+
func (o *OIDCConfig) OIDCConfig(t *testing.T, userInfoClaims jwt.MapClaims, opts ...func(cfg *coderd.OIDCConfig)) *coderd.OIDCConfig {
943943
// By default, the provider can be empty.
944944
// This means it won't support any endpoints!
945945
provider := &oidc.Provider{}
@@ -956,7 +956,7 @@ func (o *OIDCConfig) OIDCConfig(t *testing.T, userInfoClaims jwt.MapClaims) *cod
956956
}
957957
provider = cfg.NewProvider(context.Background())
958958
}
959-
return &coderd.OIDCConfig{
959+
cfg := &coderd.OIDCConfig{
960960
OAuth2Config: o,
961961
Verifier: oidc.NewVerifier(o.issuer, &oidc.StaticKeySet{
962962
PublicKeys: []crypto.PublicKey{o.key.Public()},
@@ -965,7 +965,12 @@ func (o *OIDCConfig) OIDCConfig(t *testing.T, userInfoClaims jwt.MapClaims) *cod
965965
}),
966966
Provider: provider,
967967
UsernameField: "preferred_username",
968+
GroupField: "groups",
968969
}
970+
for _, opt := range opts {
971+
opt(cfg)
972+
}
973+
return cfg
969974
}
970975

971976
// NewAzureInstanceIdentity returns a metadata client and ID token validator for faking

‎coderd/database/dbfake/databasefake.go

Lines changed: 14 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -3905,13 +3905,22 @@ func (q *fakeQuerier) DeleteGroupMembersByOrgAndUser(_ context.Context, arg data
39053905

39063906
newMembers := q.groupMembers[:0]
39073907
for _, member := range q.groupMembers {
3908-
if member.UserID == arg.UserID {
3908+
if member.UserID != arg.UserID {
3909+
// Do not delete the other members
3910+
newMembers = append(newMembers, member)
3911+
} else if member.UserID == arg.UserID {
3912+
// We only want to delete from groups in the organization in the args.
39093913
for _, group := range q.groups {
3910-
if group.ID == member.GroupID && group.OrganizationID == arg.OrganizationID {
3911-
continue
3914+
// Find the group that the member is apartof.
3915+
if group.ID == member.GroupID {
3916+
// Only add back the member if the organization ID does not match
3917+
// the arg organization ID. Since the arg is saying which
3918+
// org to delete.
3919+
if group.OrganizationID != arg.OrganizationID {
3920+
newMembers = append(newMembers, member)
3921+
}
3922+
break
39123923
}
3913-
3914-
newMembers = append(newMembers, member)
39153924
}
39163925
}
39173926
}

‎coderd/database/queries.sql.go

Lines changed: 5 additions & 11 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎coderd/database/queries/groupmembers.sql

Lines changed: 3 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -35,16 +35,10 @@ FROM
3535

3636
-- name: DeleteGroupMembersByOrgAndUser :exec
3737
DELETE FROM
38-
group_members
39-
USING
40-
group_members AS gm
41-
LEFT JOIN
42-
groups
43-
ON
44-
groups.id = gm.group_id
38+
group_members
4539
WHERE
46-
groups.organization_id = @organization_id AND
47-
gm.user_id = @user_id;
40+
group_members.user_id = @user_id
41+
AND group_id = ANY(SELECT id FROM groups WHERE organization_id = @organization_id);
4842

4943
-- name: InsertGroupMember :exec
5044
INSERT INTO

‎coderd/userauth.go

Lines changed: 29 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -477,6 +477,10 @@ type OIDCConfig struct {
477477
// UsernameField selects the claim field to be used as the created user's
478478
// username.
479479
UsernameField string
480+
// GroupField selects the claim field to be used as the created user's
481+
// groups. If the group field is the empty string, then no group updates
482+
// will ever come from the OIDC provider.
483+
GroupField string
480484
// SignInText is the text to display on the OIDC login button
481485
SignInText string
482486
// IconURL points to the URL of an icon to display on the OIDC login button
@@ -609,21 +613,27 @@ func (api *API) userOIDC(rw http.ResponseWriter, r *http.Request) {
609613
}
610614
}
611615

616+
var usingGroups bool
612617
var groups []string
613-
groupsRaw, ok := claims["groups"]
614-
if ok {
615-
// Convert the []interface{} we get to a []string.
616-
groupsInterface, ok := groupsRaw.([]interface{})
617-
if ok {
618-
for _, groupInterface := range groupsInterface {
619-
group, ok := groupInterface.(string)
620-
if !ok {
621-
httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{
622-
Message: fmt.Sprintf("Invalid group type. Expected string, got: %t", emailRaw),
623-
})
624-
return
618+
// If the GroupField is the empty string, then groups from OIDC are not used.
619+
// This is so we can support manual group assignment.
620+
if api.OIDCConfig.GroupField != "" {
621+
usingGroups = true
622+
groupsRaw, ok := claims[api.OIDCConfig.GroupField]
623+
if ok && api.OIDCConfig.GroupField != "" {
624+
// Convert the []interface{} we get to a []string.
625+
groupsInterface, ok := groupsRaw.([]interface{})
626+
if ok {
627+
for _, groupInterface := range groupsInterface {
628+
group, ok := groupInterface.(string)
629+
if !ok {
630+
httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{
631+
Message: fmt.Sprintf("Invalid group type. Expected string, got: %t", emailRaw),
632+
})
633+
return
634+
}
635+
groups = append(groups, group)
625636
}
626-
groups = append(groups, group)
627637
}
628638
}
629639
}
@@ -684,6 +694,7 @@ func (api *API) userOIDC(rw http.ResponseWriter, r *http.Request) {
684694
Email: email,
685695
Username: username,
686696
AvatarURL: picture,
697+
UsingGroups: usingGroups,
687698
Groups: groups,
688699
})
689700
var httpErr httpError
@@ -725,7 +736,10 @@ type oauthLoginParams struct {
725736
Email string
726737
Username string
727738
AvatarURL string
728-
Groups []string
739+
// Is UsingGroups is true, then the user will be assigned
740+
// to the Groups provided.
741+
UsingGroups bool
742+
Groups []string
729743
}
730744

731745
type httpError struct {
@@ -865,7 +879,7 @@ func (api *API) oauthLogin(r *http.Request, params oauthLoginParams) (*http.Cook
865879
}
866880

867881
// Ensure groups are correct.
868-
if len(params.Groups) > 0 {
882+
if params.UsingGroups {
869883
//nolint:gocritic
870884
err := api.Options.SetUserGroups(dbauthz.AsSystemRestricted(ctx), tx, user.ID, params.Groups)
871885
if err != nil {

‎codersdk/deployment.go

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -220,6 +220,7 @@ type OIDCConfig struct {
220220
Scopes clibase.Strings `json:"scopes" typescript:",notnull"`
221221
IgnoreEmailVerified clibase.Bool `json:"ignore_email_verified" typescript:",notnull"`
222222
UsernameField clibase.String `json:"username_field" typescript:",notnull"`
223+
GroupField clibase.String `json:"groups_field" typescript:",notnull"`
223224
SignInText clibase.String `json:"sign_in_text" typescript:",notnull"`
224225
IconURL clibase.URL `json:"icon_url" typescript:",notnull"`
225226
}
@@ -818,6 +819,21 @@ when required by your organization's security policy.`,
818819
Group: &deploymentGroupOIDC,
819820
YAML: "usernameField",
820821
},
822+
{
823+
Name: "OIDC Group Field",
824+
Description: "Change the OIDC default 'groups' claim field. By default, will be 'groups' if present in the oidc scopes argument.",
825+
Flag: "oidc-group-field",
826+
Env: "OIDC_GROUP_FIELD",
827+
// This value is intentionally blank. If this is empty, then OIDC group
828+
// behavior is disabled. If 'oidc-scopes' contains 'groups', then the
829+
// default value will be 'groups'. If the user wants to use a different claim
830+
// such as 'memberOf', they can override the default 'groups' claim value
831+
// that comes from the oidc scopes.
832+
Default: "",
833+
Value: &c.OIDC.GroupField,
834+
Group: &deploymentGroupOIDC,
835+
YAML: "groupField",
836+
},
821837
{
822838
Name: "OpenID Connect sign in text",
823839
Description: "The text to show on the OpenID Connect sign in button",

‎docs/admin/auth.md

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -183,7 +183,9 @@ CODER_TLS_CLIENT_KEY_FILE=/path/to/key.pem
183183
If your OpenID Connect provider supports group claims, you can configure Coder
184184
to synchronize groups in your auth provider to groups within Coder.
185185

186-
To enable group sync, ensure that the `group` claim is set:
186+
To enable group sync, ensure that the `groups` claim is set. If group sync is
187+
enabled, the user's groups will be controlled by the OIDC provider. This means
188+
manual group additions/removals will be overwritten on the next login.
187189

188190
```console
189191
# as an environment variable

‎docs/api/general.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -230,6 +230,7 @@ curl -X GET http://coder-server:8080/api/v2/deployment/config \
230230
"client_id": "string",
231231
"client_secret": "string",
232232
"email_domain": ["string"],
233+
"groups_field": "string",
233234
"icon_url": {
234235
"forceQuery": true,
235236
"fragment": "string",

‎docs/api/schemas.md

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1762,6 +1762,7 @@ CreateParameterRequest is a structure used to create a new parameter value for a
17621762
"client_id": "string",
17631763
"client_secret": "string",
17641764
"email_domain": ["string"],
1765+
"groups_field": "string",
17651766
"icon_url": {
17661767
"forceQuery": true,
17671768
"fragment": "string",
@@ -2101,6 +2102,7 @@ CreateParameterRequest is a structure used to create a new parameter value for a
21012102
"client_id": "string",
21022103
"client_secret": "string",
21032104
"email_domain": ["string"],
2105+
"groups_field": "string",
21042106
"icon_url": {
21052107
"forceQuery": true,
21062108
"fragment": "string",
@@ -2761,6 +2763,7 @@ CreateParameterRequest is a structure used to create a new parameter value for a
27612763
"client_id": "string",
27622764
"client_secret": "string",
27632765
"email_domain": ["string"],
2766+
"groups_field": "string",
27642767
"icon_url": {
27652768
"forceQuery": true,
27662769
"fragment": "string",
@@ -2790,6 +2793,7 @@ CreateParameterRequest is a structure used to create a new parameter value for a
27902793
| `client_id` | string | false | | |
27912794
| `client_secret` | string | false | | |
27922795
| `email_domain` | array of string | false | | |
2796+
| `groups_field` | string | false | | |
27932797
| `icon_url` | [clibase.URL](#clibaseurl) | false | | |
27942798
| `ignore_email_verified` | boolean | false | | |
27952799
| `issuer_url` | string | false | | |

0 commit comments

Comments
 (0)