feat: Allow changing the 'group' oidc claim field (#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

cli/server.go

@@ -75,6 +75,7 @@ import (
 	"github.com/coder/coder/coderd/telemetry"
 	"github.com/coder/coder/coderd/tracing"
 	"github.com/coder/coder/coderd/updatecheck"
+	"github.com/coder/coder/coderd/util/slice"
 	"github.com/coder/coder/codersdk"
 	"github.com/coder/coder/cryptorand"
 	"github.com/coder/coder/provisioner/echo"
@@ -765,6 +766,11 @@ flags, and YAML configuration. The precedence is as follows:
 				if err != nil {
 					return xerrors.Errorf("parse oidc oauth callback url: %w", err)
 				}
+				// If the scopes contain 'groups', we enable group support.
+				// Do not override any custom value set by the user.
+				if slice.Contains(cfg.OIDC.Scopes, "groups") && cfg.OIDC.GroupField == "" {
+					cfg.OIDC.GroupField = "groups"
+				}
 				options.OIDCConfig = &coderd.OIDCConfig{
 					OAuth2Config: &oauth2.Config{
 						ClientID:     cfg.OIDC.ClientID.String(),
@@ -780,6 +786,7 @@ flags, and YAML configuration. The precedence is as follows:
 					EmailDomain:         cfg.OIDC.EmailDomain,
 					AllowSignups:        cfg.OIDC.AllowSignups.Value(),
 					UsernameField:       cfg.OIDC.UsernameField.String(),
+					GroupField:          cfg.OIDC.GroupField.String(),
 					SignInText:          cfg.OIDC.SignInText.String(),
 					IconURL:             cfg.OIDC.IconURL.String(),
 					IgnoreEmailVerified: cfg.OIDC.IgnoreEmailVerified.Value(),

coderd/apidoc/docs.go

@@ -7078,6 +7078,9 @@ const docTemplate = `{
                         "type": "string"
                     }
                 },
+                "groups_field": {
+                    "type": "string"
+                },
                 "icon_url": {
                     "$ref": "#/definitions/clibase.URL"
                 },

coderd/apidoc/swagger.json

@@ -6338,6 +6338,9 @@
             "type": "string"
           }
         },
+        "groups_field": {
+          "type": "string"
+        },
         "icon_url": {
           "$ref": "#/definitions/clibase.URL"
         },

coderd/coderdtest/coderdtest.go

@@ -939,7 +939,7 @@ func (o *OIDCConfig) EncodeClaims(t *testing.T, claims jwt.MapClaims) string {
 	return base64.StdEncoding.EncodeToString([]byte(signed))
 }
 
-func (o *OIDCConfig) OIDCConfig(t *testing.T, userInfoClaims jwt.MapClaims) *coderd.OIDCConfig {
+func (o *OIDCConfig) OIDCConfig(t *testing.T, userInfoClaims jwt.MapClaims, opts ...func(cfg *coderd.OIDCConfig)) *coderd.OIDCConfig {
 	// By default, the provider can be empty.
 	// This means it won't support any endpoints!
 	provider := &oidc.Provider{}
@@ -956,7 +956,7 @@ func (o *OIDCConfig) OIDCConfig(t *testing.T, userInfoClaims jwt.MapClaims) *cod
 		}
 		provider = cfg.NewProvider(context.Background())
 	}
-	return &coderd.OIDCConfig{
+	cfg := &coderd.OIDCConfig{
 		OAuth2Config: o,
 		Verifier: oidc.NewVerifier(o.issuer, &oidc.StaticKeySet{
 			PublicKeys: []crypto.PublicKey{o.key.Public()},
@@ -965,7 +965,12 @@ func (o *OIDCConfig) OIDCConfig(t *testing.T, userInfoClaims jwt.MapClaims) *cod
 		}),
 		Provider:      provider,
 		UsernameField: "preferred_username",
+		GroupField:    "groups",
 	}
+	for _, opt := range opts {
+		opt(cfg)
+	}
+	return cfg
 }
 
 // NewAzureInstanceIdentity returns a metadata client and ID token validator for faking

coderd/database/dbfake/databasefake.go

@@ -3905,13 +3905,22 @@ func (q *fakeQuerier) DeleteGroupMembersByOrgAndUser(_ context.Context, arg data
 
 	newMembers := q.groupMembers[:0]
 	for _, member := range q.groupMembers {
-		if member.UserID == arg.UserID {
+		if member.UserID != arg.UserID {
+			// Do not delete the other members
+			newMembers = append(newMembers, member)
+		} else if member.UserID == arg.UserID {
+			// We only want to delete from groups in the organization in the args.
 			for _, group := range q.groups {
-				if group.ID == member.GroupID && group.OrganizationID == arg.OrganizationID {
-					continue
+				// Find the group that the member is apartof.
+				if group.ID == member.GroupID {
+					// Only add back the member if the organization ID does not match
+					// the arg organization ID. Since the arg is saying which
+					// org to delete.
+					if group.OrganizationID != arg.OrganizationID {
+						newMembers = append(newMembers, member)
+					}
+					break
 				}
-
-				newMembers = append(newMembers, member)
 			}
 		}
 	}

coderd/database/queries.sql.go

@@ -960,25 +960,19 @@ func (q *sqlQuerier) DeleteGroupMemberFromGroup(ctx context.Context, arg DeleteG
 
 const deleteGroupMembersByOrgAndUser = `-- name: DeleteGroupMembersByOrgAndUser :exec
 DELETE FROM
-    group_members
-USING
-    group_members AS gm
-LEFT JOIN
-    groups
-ON
-    groups.id = gm.group_id
+	group_members
 WHERE
-    groups.organization_id = $1 AND
-    gm.user_id = $2
+	group_members.user_id = $1
+	AND group_id = ANY(SELECT id FROM groups WHERE organization_id = $2)
 `
 
 type DeleteGroupMembersByOrgAndUserParams struct {
-	OrganizationID uuid.UUID `db:"organization_id" json:"organization_id"`
 	UserID         uuid.UUID `db:"user_id" json:"user_id"`
+	OrganizationID uuid.UUID `db:"organization_id" json:"organization_id"`
 }
 
 func (q *sqlQuerier) DeleteGroupMembersByOrgAndUser(ctx context.Context, arg DeleteGroupMembersByOrgAndUserParams) error {
-	_, err := q.db.ExecContext(ctx, deleteGroupMembersByOrgAndUser, arg.OrganizationID, arg.UserID)
+	_, err := q.db.ExecContext(ctx, deleteGroupMembersByOrgAndUser, arg.UserID, arg.OrganizationID)
 	return err
 }
 

coderd/database/queries/groupmembers.sql

@@ -35,16 +35,10 @@ FROM
 
 -- name: DeleteGroupMembersByOrgAndUser :exec
 DELETE FROM
-    group_members
-USING
-    group_members AS gm
-LEFT JOIN
-    groups
-ON
-    groups.id = gm.group_id
+	group_members
 WHERE
-    groups.organization_id = @organization_id AND
-    gm.user_id = @user_id;
+	group_members.user_id = @user_id
+	AND group_id = ANY(SELECT id FROM groups WHERE organization_id = @organization_id);
 
 -- name: InsertGroupMember :exec
 INSERT INTO

coderd/userauth.go

@@ -477,6 +477,10 @@ type OIDCConfig struct {
 	// UsernameField selects the claim field to be used as the created user's
 	// username.
 	UsernameField string
+	// GroupField selects the claim field to be used as the created user's
+	// groups. If the group field is the empty string, then no group updates
+	// will ever come from the OIDC provider.
+	GroupField string
 	// SignInText is the text to display on the OIDC login button
 	SignInText string
 	// 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) {
 		}
 	}
 
+	var usingGroups bool
 	var groups []string
-	groupsRaw, ok := claims["groups"]
-	if ok {
-		// Convert the []interface{} we get to a []string.
-		groupsInterface, ok := groupsRaw.([]interface{})
-		if ok {
-			for _, groupInterface := range groupsInterface {
-				group, ok := groupInterface.(string)
-				if !ok {
-					httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{
-						Message: fmt.Sprintf("Invalid group type. Expected string, got: %t", emailRaw),
-					})
-					return
+	// 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 != "" {
+		usingGroups = true
+		groupsRaw, ok := claims[api.OIDCConfig.GroupField]
+		if ok && api.OIDCConfig.GroupField != "" {
+			// Convert the []interface{} we get to a []string.
+			groupsInterface, ok := groupsRaw.([]interface{})
+			if ok {
+				for _, groupInterface := range groupsInterface {
+					group, ok := groupInterface.(string)
+					if !ok {
+						httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{
+							Message: fmt.Sprintf("Invalid group type. Expected string, got: %t", emailRaw),
+						})
+						return
+					}
+					groups = append(groups, group)
 				}
-				groups = append(groups, group)
 			}
 		}
 	}
@@ -684,6 +694,7 @@ func (api *API) userOIDC(rw http.ResponseWriter, r *http.Request) {
 		Email:        email,
 		Username:     username,
 		AvatarURL:    picture,
+		UsingGroups:  usingGroups,
 		Groups:       groups,
 	})
 	var httpErr httpError
@@ -725,7 +736,10 @@ type oauthLoginParams struct {
 	Email        string
 	Username     string
 	AvatarURL    string
-	Groups       []string
+	// Is UsingGroups is true, then the user will be assigned
+	// to the Groups provided.
+	UsingGroups bool
+	Groups      []string
 }
 
 type httpError struct {
@@ -865,7 +879,7 @@ func (api *API) oauthLogin(r *http.Request, params oauthLoginParams) (*http.Cook
 		}
 
 		// Ensure groups are correct.
-		if len(params.Groups) > 0 {
+		if params.UsingGroups {
 			//nolint:gocritic
 			err := api.Options.SetUserGroups(dbauthz.AsSystemRestricted(ctx), tx, user.ID, params.Groups)
 			if err != nil {

codersdk/deployment.go

@@ -220,6 +220,7 @@ type OIDCConfig struct {
 	Scopes              clibase.Strings `json:"scopes" typescript:",notnull"`
 	IgnoreEmailVerified clibase.Bool    `json:"ignore_email_verified" typescript:",notnull"`
 	UsernameField       clibase.String  `json:"username_field" typescript:",notnull"`
+	GroupField          clibase.String  `json:"groups_field" typescript:",notnull"`
 	SignInText          clibase.String  `json:"sign_in_text" typescript:",notnull"`
 	IconURL             clibase.URL     `json:"icon_url" typescript:",notnull"`
 }
@@ -818,6 +819,21 @@ when required by your organization's security policy.`,
 			Group:       &deploymentGroupOIDC,
 			YAML:        "usernameField",
 		},
+		{
+			Name:        "OIDC Group Field",
+			Description: "Change the OIDC default 'groups' claim field. By default, will be 'groups' if present in the oidc scopes argument.",
+			Flag:        "oidc-group-field",
+			Env:         "OIDC_GROUP_FIELD",
+			// This value is intentionally blank. If this is empty, then OIDC group
+			// behavior is disabled. If 'oidc-scopes' contains 'groups', then the
+			// default value will be 'groups'. If the user wants to use a different claim
+			// such as 'memberOf', they can override the default 'groups' claim value
+			// that comes from the oidc scopes.
+			Default: "",
+			Value:   &c.OIDC.GroupField,
+			Group:   &deploymentGroupOIDC,
+			YAML:    "groupField",
+		},
 		{
 			Name:        "OpenID Connect sign in text",
 			Description: "The text to show on the OpenID Connect sign in button",

docs/admin/auth.md

@@ -183,7 +183,9 @@ CODER_TLS_CLIENT_KEY_FILE=/path/to/key.pem
 If your OpenID Connect provider supports group claims, you can configure Coder
 to synchronize groups in your auth provider to groups within Coder.
 
-To enable group sync, ensure that the `group` claim is set:
+To enable group sync, ensure that the `groups` claim is set. If group sync is
+enabled, the user's groups will be controlled by the OIDC provider. This means
+manual group additions/removals will be overwritten on the next login.
 
 ```console
 # as an environment variable

docs/api/general.md

@@ -230,6 +230,7 @@ curl -X GET http://coder-server:8080/api/v2/deployment/config \
       "client_id": "string",
       "client_secret": "string",
       "email_domain": ["string"],
+      "groups_field": "string",
       "icon_url": {
         "forceQuery": true,
         "fragment": "string",

docs/api/schemas.md

@@ -1762,6 +1762,7 @@ CreateParameterRequest is a structure used to create a new parameter value for a
       "client_id": "string",
       "client_secret": "string",
       "email_domain": ["string"],
+      "groups_field": "string",
       "icon_url": {
         "forceQuery": true,
         "fragment": "string",
@@ -2101,6 +2102,7 @@ CreateParameterRequest is a structure used to create a new parameter value for a
     "client_id": "string",
     "client_secret": "string",
     "email_domain": ["string"],
+    "groups_field": "string",
     "icon_url": {
       "forceQuery": true,
       "fragment": "string",
@@ -2761,6 +2763,7 @@ CreateParameterRequest is a structure used to create a new parameter value for a
   "client_id": "string",
   "client_secret": "string",
   "email_domain": ["string"],
+  "groups_field": "string",
   "icon_url": {
     "forceQuery": true,
     "fragment": "string",
@@ -2790,6 +2793,7 @@ CreateParameterRequest is a structure used to create a new parameter value for a
 | `client_id`             | string                     | false    |              |             |
 | `client_secret`         | string                     | false    |              |             |
 | `email_domain`          | array of string            | false    |              |             |
+| `groups_field`          | string                     | false    |              |             |
 | `icon_url`              | [clibase.URL](#clibaseurl) | false    |              |             |
 | `ignore_email_verified` | boolean                    | false    |              |             |
 | `issuer_url`            | string                     | false    |              |             |

enterprise/coderd/userauth_test.go

@@ -8,9 +8,11 @@ import (
 	"testing"
 
 	"github.com/golang-jwt/jwt"
+	"github.com/google/uuid"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 
+	"github.com/coder/coder/coderd"
 	"github.com/coder/coder/coderd/coderdtest"
 	"github.com/coder/coder/codersdk"
 	"github.com/coder/coder/enterprise/coderd/coderdenttest"
@@ -28,7 +30,10 @@ func TestUserOIDC(t *testing.T) {
 			ctx, _ := testutil.Context(t)
 			conf := coderdtest.NewOIDCConfig(t, "")
 
-			config := conf.OIDCConfig(t, jwt.MapClaims{})
+			const groupClaim = "custom-groups"
+			config := conf.OIDCConfig(t, jwt.MapClaims{}, func(cfg *coderd.OIDCConfig) {
+				cfg.GroupField = groupClaim
+			})
 			config.AllowSignups = true
 
 			client := coderdenttest.New(t, &coderdenttest.Options{
@@ -53,8 +58,8 @@ func TestUserOIDC(t *testing.T) {
 			require.Len(t, group.Members, 0)
 
 			resp := oidcCallback(t, client, conf.EncodeClaims(t, jwt.MapClaims{
-				"email":  "colin@coder.com",
-				"groups": []string{groupName},
+				"email":    "colin@coder.com",
+				groupClaim: []string{groupName},
 			}))
 			assert.Equal(t, http.StatusTemporaryRedirect, resp.StatusCode)
 
@@ -62,6 +67,72 @@ func TestUserOIDC(t *testing.T) {
 			require.NoError(t, err)
 			require.Len(t, group.Members, 1)
 		})
+		t.Run("AddThenRemove", func(t *testing.T) {
+			t.Parallel()
+
+			ctx, _ := testutil.Context(t)
+			conf := coderdtest.NewOIDCConfig(t, "")
+
+			config := conf.OIDCConfig(t, jwt.MapClaims{})
+			config.AllowSignups = true
+
+			client := coderdenttest.New(t, &coderdenttest.Options{
+				Options: &coderdtest.Options{
+					OIDCConfig: config,
+				},
+			})
+			firstUser := coderdtest.CreateFirstUser(t, client)
+			coderdenttest.AddLicense(t, client, coderdenttest.LicenseOptions{
+				AllFeatures: true,
+			})
+
+			// Add some extra users/groups that should be asserted after.
+			// Adding this user as there was a bug that removing 1 user removed
+			// all users from the group.
+			_, extra := coderdtest.CreateAnotherUser(t, client, firstUser.OrganizationID)
+			groupName := "bingbong"
+			group, err := client.CreateGroup(ctx, firstUser.OrganizationID, codersdk.CreateGroupRequest{
+				Name: groupName,
+			})
+			require.NoError(t, err, "create group")
+
+			group, err = client.PatchGroup(ctx, group.ID, codersdk.PatchGroupRequest{
+				AddUsers: []string{
+					firstUser.UserID.String(),
+					extra.ID.String(),
+				},
+			})
+			require.NoError(t, err, "patch group")
+			require.Len(t, group.Members, 2, "expect both members")
+
+			// Now add OIDC user into the group
+			resp := oidcCallback(t, client, conf.EncodeClaims(t, jwt.MapClaims{
+				"email":  "colin@coder.com",
+				"groups": []string{groupName},
+			}))
+			assert.Equal(t, http.StatusTemporaryRedirect, resp.StatusCode)
+
+			group, err = client.Group(ctx, group.ID)
+			require.NoError(t, err)
+			require.Len(t, group.Members, 3)
+
+			// Login to remove the OIDC user from the group
+			resp = oidcCallback(t, client, conf.EncodeClaims(t, jwt.MapClaims{
+				"email":  "colin@coder.com",
+				"groups": []string{},
+			}))
+			assert.Equal(t, http.StatusTemporaryRedirect, resp.StatusCode)
+
+			group, err = client.Group(ctx, group.ID)
+			require.NoError(t, err)
+			require.Len(t, group.Members, 2)
+			var expected []uuid.UUID
+			for _, mem := range group.Members {
+				expected = append(expected, mem.ID)
+			}
+			require.ElementsMatchf(t, expected, []uuid.UUID{firstUser.UserID, extra.ID}, "expected members")
+		})
+
 		t.Run("NoneMatch", func(t *testing.T) {
 			t.Parallel()
 

site/src/api/typesGenerated.ts

@@ -502,6 +502,7 @@ export interface OIDCConfig {
   readonly scopes: string[]
   readonly ignore_email_verified: boolean
   readonly username_field: string
+  readonly groups_field: string
   readonly sign_in_text: string
   readonly icon_url: string
 }