diff --git a/cli/server.go b/cli/server.go index 94f4188aa2934..fe94ce1611ba2 100644 --- 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 index 966bff2bf4d45..6b8943b4d7812 100644 --- a/coderd/apidoc/docs.go +++ b/coderd/apidoc/docs.go @@ -7078,6 +7078,9 @@ const docTemplate = `{ "type": "string" } }, + "groups_field": { + "type": "string" + }, "icon_url": { "$ref": "#/definitions/clibase.URL" }, diff --git a/coderd/apidoc/swagger.json b/coderd/apidoc/swagger.json index b64e2cd4d79d2..1d7a821dee173 100644 --- a/coderd/apidoc/swagger.json +++ b/coderd/apidoc/swagger.json @@ -6338,6 +6338,9 @@ "type": "string" } }, + "groups_field": { + "type": "string" + }, "icon_url": { "$ref": "#/definitions/clibase.URL" }, diff --git a/coderd/coderdtest/coderdtest.go b/coderd/coderdtest/coderdtest.go index 0b4222763dd1a..e05e00649f879 100644 --- 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 index 40e004e157527..8a8eb652ddc7e 100644 --- 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 index 707e87ee6f4e7..b13551978fad6 100644 --- a/coderd/database/queries.sql.go +++ b/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 } diff --git a/coderd/database/queries/groupmembers.sql b/coderd/database/queries/groupmembers.sql index 2ffa1e4f5caca..a7c04d85f20a8 100644 --- 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 index 7fab1df83fda1..9ca7dfec53c3b 100644 --- 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 index 5066d9ff11f45..4135a95bfec46 100644 --- 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 index 8cf905fb3022f..fb632fffddb19 100644 --- 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 index 2516d79e995c2..679b6405af156 100644 --- 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 index bfe21043b84b9..8ce9eac55540b 100644 --- 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 | | | diff --git a/enterprise/coderd/userauth_test.go b/enterprise/coderd/userauth_test.go index a2dbd051d7714..5e93b63611795 100644 --- a/enterprise/coderd/userauth_test.go +++ b/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{ @@ -52,6 +57,55 @@ func TestUserOIDC(t *testing.T) { require.NoError(t, err) require.Len(t, group.Members, 0) + resp := oidcCallback(t, client, conf.EncodeClaims(t, jwt.MapClaims{ + "email": "colin@coder.com", + groupClaim: []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, 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}, @@ -60,8 +114,25 @@ func TestUserOIDC(t *testing.T) { group, err = client.Group(ctx, group.ID) require.NoError(t, err) - require.Len(t, group.Members, 1) + 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() diff --git a/site/src/api/typesGenerated.ts b/site/src/api/typesGenerated.ts index f9bd7bcf48b19..18647dcd68161 100644 --- a/site/src/api/typesGenerated.ts +++ b/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 }