coder · Emyrk · Mar 10, 2023 · Mar 10, 2023 · Mar 10, 2023 · Mar 10, 2023
diff --git a/cli/server.go b/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"
@@ -764,6 +765,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(),
@@ -779,6 +785,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(),

diff --git a/coderd/apidoc/docs.go b/coderd/apidoc/docs.go
diff --git a/coderd/apidoc/swagger.json b/coderd/apidoc/swagger.json
diff --git a/coderd/coderdtest/coderdtest.go b/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

diff --git a/coderd/database/dbfake/databasefake.go b/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)
 			}
 		}
 	}

diff --git a/coderd/database/queries.sql.go b/coderd/database/queries.sql.go
diff --git a/coderd/database/queries/groupmembers.sql b/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

diff --git a/coderd/userauth.go b/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 {

diff --git a/codersdk/deployment.go b/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",

diff --git a/docs/admin/auth.md b/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

diff --git a/docs/api/general.md b/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",

diff --git a/docs/api/schemas.md b/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    |              |             |