allow mapping 1:many

coder · Emyrk · Jul 24, 2023 · Jul 19, 2023 · Jul 19, 2023 · Jul 19, 2023
commit 1e6bd370b2fd57c7de4509943224cbe89ed96cb7
diff --git a/coderd/database/dbauthz/dbauthz.go b/coderd/database/dbauthz/dbauthz.go
@@ -207,7 +207,7 @@ var (
 					rbac.ResourceWildcard.Type:           {rbac.ActionRead},
 					rbac.ResourceAPIKey.Type:             {rbac.ActionCreate, rbac.ActionUpdate, rbac.ActionDelete},
 					rbac.ResourceGroup.Type:              {rbac.ActionCreate, rbac.ActionUpdate},
-					rbac.ResourceRoleAssignment.Type:     {rbac.ActionCreate},
+					rbac.ResourceRoleAssignment.Type:     {rbac.ActionCreate, rbac.ActionDelete},
 					rbac.ResourceSystem.Type:             {rbac.WildcardSymbol},
 					rbac.ResourceOrganization.Type:       {rbac.ActionCreate},
 					rbac.ResourceOrganizationMember.Type: {rbac.ActionCreate},

diff --git a/coderd/rbac/roles.go b/coderd/rbac/roles.go
@@ -283,10 +283,13 @@ func ReloadBuiltinRoles(opts *RoleOptions) {
 //	map[actor_role][assign_role]<can_assign>
 var assignRoles = map[string]map[string]bool{
 	"system": {
-		owner:     true,
-		member:    true,
-		orgAdmin:  true,
-		orgMember: true,
+		owner:         true,
+		auditor:       true,
+		member:        true,
+		orgAdmin:      true,
+		orgMember:     true,
+		templateAdmin: true,
+		userAdmin:     true,
 	},
 	owner: {
 		owner:         true,

diff --git a/coderd/userauth.go b/coderd/userauth.go
@@ -691,7 +691,7 @@ type OIDCConfig struct {
 	// UserRoleMapping controls how groups returned by the OIDC provider get mapped
 	// to roles within Coder.
 	// map[oidcRoleName]coderRoleName
-	UserRoleMapping map[string]string
+	UserRoleMapping map[string][]string
 	// UserRolesDefault is the default set of roles to assign to a user if role sync
 	// is enabled.
 	UserRolesDefault []string
@@ -905,55 +905,6 @@ func (api *API) userOIDC(rw http.ResponseWriter, r *http.Request) {
 		}
 	}
 
-	roles := api.OIDCConfig.UserRolesDefault
-	if api.OIDCConfig.RoleSyncEnabled() {
-		rolesRow, ok := claims[api.OIDCConfig.UserRoleField]
-		if !ok {
-			logger.Error(ctx, "oidc user roles are missing from claim")
-			httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{
-				Message: "Login disabled until OIDC config is fixed, contact your administrator. Missing OIDC user roles in claim.",
-				Detail:  "If role sync is enabled, then the OIDC user roles must be present in the claim. Disabling role sync will allow login to proceed.",
-			})
-			return
-		}
-
-		// Convert the []interface{} we get to a []string.
-		rolesInterface, ok := rolesRow.([]interface{})
-		if !ok {
-			api.Logger.Error(ctx, "oidc claim user roles field was an unknown type",
-				slog.F("type", fmt.Sprintf("%T", rolesRow)),
-			)
-			httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{
-				Message: "Login disabled until OIDC config is fixed, contact your administrator. Missing OIDC user roles in claim.",
-				Detail:  fmt.Sprintf("Roles claim must be an array of strings, type found: %T. Disabling role sync will allow login to proceed.", rolesRow),
-			})
-			return
-		}
-
-		api.Logger.Debug(ctx, "roles returned in oidc claims",
-			slog.F("len", len(rolesInterface)),
-			slog.F("roles", rolesInterface),
-		)
-		for _, roleInterface := range rolesInterface {
-			role, ok := roleInterface.(string)
-			if !ok {
-				api.Logger.Error(ctx, "invalid oidc user role type",
-					slog.F("type", fmt.Sprintf("%T", rolesRow)),
-				)
-				httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{
-					Message: fmt.Sprintf("Invalid user role type. Expected string, got: %T", roleInterface),
-				})
-				return
-			}
-
-			if mappedRole, ok := api.OIDCConfig.UserRoleMapping[role]; ok {
-				role = mappedRole
-			}
-
-			roles = append(roles, role)
-		}
-	}
-
 	// This conditional is purely to warn the user they might have misconfigured their OIDC
 	// configuration.
 	if _, groupClaimExists := claims["groups"]; !usingGroups && groupClaimExists {
@@ -1006,6 +957,63 @@ func (api *API) userOIDC(rw http.ResponseWriter, r *http.Request) {
 		return
 	}
 
+	roles := api.OIDCConfig.UserRolesDefault
+	if api.OIDCConfig.RoleSyncEnabled() {
+		rolesRow, ok := claims[api.OIDCConfig.UserRoleField]
+		if !ok {
+			// If no claim is provided than we can assume the user is just
+			// a member. This is because there is no way to tell the difference
+			// between []string{} and nil for OIDC claims. IDPs omit claims
+			// if they are empty ([]string{}).
+			rolesRow = []string{}
+		}
+
+		// Convert the []interface{} we get to a []string.
+		rolesInterface, ok := rolesRow.([]interface{})
+		if !ok {
+			api.Logger.Error(ctx, "oidc claim user roles field was an unknown type",
+				slog.F("type", fmt.Sprintf("%T", rolesRow)),
+			)
+			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
+		}
+
+		api.Logger.Debug(ctx, "roles returned in oidc claims",
+			slog.F("len", len(rolesInterface)),
+			slog.F("roles", rolesInterface),
+		)
+		for _, roleInterface := range rolesInterface {
+			role, ok := roleInterface.(string)
+			if !ok {
+				api.Logger.Error(ctx, "invalid oidc user role type",
+					slog.F("type", fmt.Sprintf("%T", rolesRow)),
+				)
+				httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{
+					Message: fmt.Sprintf("Invalid user role type. Expected string, got: %T", roleInterface),
+				})
+				return
+			}
+
+			if mappedRoles, ok := api.OIDCConfig.UserRoleMapping[role]; ok {
+				if len(mappedRoles) == 0 {
+					continue
+				}
+				// Mapped roles are added to the list of roles
+				roles = append(roles, mappedRoles...)
+				continue
+			}
+
+			roles = append(roles, role)
+		}
+	}
+
 	// If a new user is authenticating for the first time
 	// the audit action is 'register', not 'login'
 	if user.ID == uuid.Nil {
@@ -1178,6 +1186,7 @@ func (api *API) oauthLogin(r *http.Request, params *oauthLoginParams) ([]*http.C
 		ctx     = r.Context()
 		user    database.User
 		cookies []*http.Cookie
+		logger  = api.Logger.Named(userAuthLoggerName)
 	)
 
 	var isConvertLoginType bool
@@ -1320,10 +1329,32 @@ func (api *API) oauthLogin(r *http.Request, params *oauthLoginParams) ([]*http.C
 
 		// Ensure roles are correct.
 		if params.UsingRoles {
+			ignored := make([]string, 0)
+			filtered := make([]string, 0, len(params.Roles))
+			for _, role := range params.Roles {
+				if _, err := rbac.RoleByName(role); err == nil {
+					filtered = append(filtered, role)
+				} else {
+					ignored = append(ignored, role)
+				}
+			}
+
 			//nolint:gocritic
-			err := api.Options.SetUserSiteRoles(dbauthz.AsSystemRestricted(ctx), tx, user.ID, params.Roles)
+			err := api.Options.SetUserSiteRoles(dbauthz.AsSystemRestricted(ctx), tx, user.ID, filtered)
 			if err != nil {
-				return xerrors.Errorf("set user groups: %w", err)
+				return httpError{
+					code:             http.StatusBadRequest,
+					msg:              "Invalid roles through OIDC claim",
+					detail:           fmt.Sprintf("Error from role assignment attempt: %s", err.Error()),
+					renderStaticPage: true,
+				}
+			}
+			if len(ignored) > 0 {
+				logger.Debug(ctx, "OIDC roles ignored in assignment",
+					slog.F("ignored", ignored),
+					slog.F("assigned", filtered),
+					slog.F("user_id", user.ID),
+				)
 			}
 		}
 

diff --git a/coderd/users.go b/coderd/users.go
@@ -909,7 +909,7 @@ func (api *API) putUserRoles(rw http.ResponseWriter, r *http.Request) {
 		return
 	}
 
-	updatedUser, err := api.UpdateSiteUserRoles(ctx, database.UpdateUserRolesParams{
+	updatedUser, err := UpdateSiteUserRoles(ctx, api.Database, database.UpdateUserRolesParams{
 		GrantedRoles: params.Roles,
 		ID:           user.ID,
 	})
@@ -939,7 +939,7 @@ func (api *API) putUserRoles(rw http.ResponseWriter, r *http.Request) {
 
 // UpdateSiteUserRoles will ensure only site wide roles are passed in as arguments.
 // If an organization role is included, an error is returned.
-func (api *API) UpdateSiteUserRoles(ctx context.Context, args database.UpdateUserRolesParams) (database.User, error) {
+func UpdateSiteUserRoles(ctx context.Context, db database.Store, args database.UpdateUserRolesParams) (database.User, error) {
 	// Enforce only site wide roles.
 	for _, r := range args.GrantedRoles {
 		if _, ok := rbac.IsOrgRole(r); ok {
@@ -951,7 +951,7 @@ func (api *API) UpdateSiteUserRoles(ctx context.Context, args database.UpdateUse
 		}
 	}
 
-	updatedUser, err := api.Database.UpdateUserRoles(ctx, args)
+	updatedUser, err := db.UpdateUserRoles(ctx, args)
 	if err != nil {
 		return database.User{}, xerrors.Errorf("update site roles: %w", err)
 	}

diff --git a/codersdk/deployment.go b/codersdk/deployment.go
@@ -256,24 +256,24 @@ type OAuth2GithubConfig struct {
 }
 
 type OIDCConfig struct {
-	AllowSignups        clibase.Bool                      `json:"allow_signups" typescript:",notnull"`
-	ClientID            clibase.String                    `json:"client_id" typescript:",notnull"`
-	ClientSecret        clibase.String                    `json:"client_secret" typescript:",notnull"`
-	EmailDomain         clibase.StringArray               `json:"email_domain" typescript:",notnull"`
-	IssuerURL           clibase.String                    `json:"issuer_url" typescript:",notnull"`
-	Scopes              clibase.StringArray               `json:"scopes" typescript:",notnull"`
-	IgnoreEmailVerified clibase.Bool                      `json:"ignore_email_verified" typescript:",notnull"`
-	UsernameField       clibase.String                    `json:"username_field" typescript:",notnull"`
-	EmailField          clibase.String                    `json:"email_field" typescript:",notnull"`
-	AuthURLParams       clibase.Struct[map[string]string] `json:"auth_url_params" typescript:",notnull"`
-	IgnoreUserInfo      clibase.Bool                      `json:"ignore_user_info" 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"`
-	UserRoleMapping     clibase.Struct[map[string]string] `json:"user_role_mapping" typescript:",notnull"`
-	UserRolesDefault    clibase.StringArray               `json:"user_roles_default" typescript:",notnull"`
-	SignInText          clibase.String                    `json:"sign_in_text" typescript:",notnull"`
-	IconURL             clibase.URL                       `json:"icon_url" typescript:",notnull"`
+	AllowSignups        clibase.Bool                        `json:"allow_signups" typescript:",notnull"`
+	ClientID            clibase.String                      `json:"client_id" typescript:",notnull"`
+	ClientSecret        clibase.String                      `json:"client_secret" typescript:",notnull"`
+	EmailDomain         clibase.StringArray                 `json:"email_domain" typescript:",notnull"`
+	IssuerURL           clibase.String                      `json:"issuer_url" typescript:",notnull"`
+	Scopes              clibase.StringArray                 `json:"scopes" typescript:",notnull"`
+	IgnoreEmailVerified clibase.Bool                        `json:"ignore_email_verified" typescript:",notnull"`
+	UsernameField       clibase.String                      `json:"username_field" typescript:",notnull"`
+	EmailField          clibase.String                      `json:"email_field" typescript:",notnull"`
+	AuthURLParams       clibase.Struct[map[string]string]   `json:"auth_url_params" typescript:",notnull"`
+	IgnoreUserInfo      clibase.Bool                        `json:"ignore_user_info" 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"`
+	UserRoleMapping     clibase.Struct[map[string][]string] `json:"user_role_mapping" typescript:",notnull"`
+	UserRolesDefault    clibase.StringArray                 `json:"user_roles_default" typescript:",notnull"`
+	SignInText          clibase.String                      `json:"sign_in_text" typescript:",notnull"`
+	IconURL             clibase.URL                         `json:"icon_url" typescript:",notnull"`
 }
 
 type TelemetryConfig struct {
@@ -1046,7 +1046,7 @@ when required by your organization's security policy.`,
 		},
 		{
 			Name:        "OIDC User Role Mapping",
-			Description: "A map of the OIDC passed in user roles and the groups in Coder it should map to. This is useful if the group names do not match.",
+			Description: "A map of the OIDC passed in user roles and the groups in Coder it should map to. This is useful if the group names do not match. If mapped to the empty string, the role will ignored.",
 			Flag:        "oidc-user-role-mapping",
 			Env:         "CODER_OIDC_USER_ROLE_MAPPING",
 			Default:     "{}",

diff --git a/enterprise/coderd/userauth.go b/enterprise/coderd/userauth.go
@@ -3,6 +3,8 @@ package coderd
 import (
 	"context"
 
+	"github.com/coder/coder/coderd"
+
 	"github.com/google/uuid"
 	"golang.org/x/xerrors"
 
@@ -54,12 +56,12 @@ func (api *API) setUserGroups(ctx context.Context, db database.Store, userID uui
 func (api *API) setUserSiteRoles(ctx context.Context, db database.Store, userID uuid.UUID, roles []string) error {
 	// Should this be feature protected?
 	return db.InTx(func(tx database.Store) error {
-		_, err := api.AGPL.UpdateSiteUserRoles(ctx, database.UpdateUserRolesParams{
+		_, err := coderd.UpdateSiteUserRoles(ctx, db, database.UpdateUserRolesParams{
 			GrantedRoles: roles,
 			ID:           userID,
 		})
 		if err != nil {
-			return xerrors.Errorf("set user roles: %w", err)
+			return xerrors.Errorf("set user roles(%s): %w", userID.String(), err)
 		}
 
 		return nil

diff --git a/enterprise/coderd/userauth_test.go b/enterprise/coderd/userauth_test.go
@@ -16,6 +16,7 @@ import (
 
 	"github.com/coder/coder/coderd"
 	"github.com/coder/coder/coderd/coderdtest"
+	"github.com/coder/coder/coderd/rbac"
 	"github.com/coder/coder/codersdk"
 	"github.com/coder/coder/enterprise/coderd/coderdenttest"
 	"github.com/coder/coder/testutil"
@@ -24,6 +25,61 @@ import (
 // nolint:bodyclose
 func TestUserOIDC(t *testing.T) {
 	t.Parallel()
+	t.Run("Roles", func(t *testing.T) {
+		t.Parallel()
+
+		t.Run("NewUser", func(t *testing.T) {
+			t.Parallel()
+
+			ctx := testutil.Context(t, testutil.WaitMedium)
+			conf := coderdtest.NewOIDCConfig(t, "")
+
+			oidcRoleName := "TemplateAuthor"
+
+			config := conf.OIDCConfig(t, jwt.MapClaims{}, func(cfg *coderd.OIDCConfig) {
+				cfg.UserRoleMapping = map[string][]string{oidcRoleName: {rbac.RoleTemplateAdmin(), rbac.RoleUserAdmin()}}
+			})
+			config.AllowSignups = true
+			config.UserRoleField = "roles"
+
+			client, _ := coderdenttest.New(t, &coderdenttest.Options{
+				Options: &coderdtest.Options{
+					OIDCConfig: config,
+				},
+				LicenseOptions: &coderdenttest.LicenseOptions{
+					Features: license.Features{codersdk.FeatureTemplateRBAC: 1},
+				},
+			})
+
+			admin, err := client.User(ctx, "me")
+			require.NoError(t, err)
+			require.Len(t, admin.OrganizationIDs, 1)
+
+			resp := oidcCallback(t, client, conf.EncodeClaims(t, jwt.MapClaims{
+				"email": "alice@coder.com",
+				"roles": []string{"random", oidcRoleName, rbac.RoleOwner()},
+			}))
+			require.Equal(t, http.StatusTemporaryRedirect, resp.StatusCode)
+			user, err := client.User(ctx, "alice")
+			require.NoError(t, err)
+
+			require.Len(t, user.Roles, 3)
+			roleNames := []string{user.Roles[0].Name, user.Roles[1].Name, user.Roles[2].Name}
+			require.ElementsMatch(t, roleNames, []string{rbac.RoleTemplateAdmin(), rbac.RoleUserAdmin(), rbac.RoleOwner()})
+
+			// Now remove the roles with a new oidc login
+			resp = oidcCallback(t, client, conf.EncodeClaims(t, jwt.MapClaims{
+				"email": "alice@coder.com",
+				"roles": []string{"random"},
+			}))
+			require.Equal(t, http.StatusTemporaryRedirect, resp.StatusCode)
+			user, err = client.User(ctx, "alice")
+			require.NoError(t, err)
+
+			require.Len(t, user.Roles, 0)
+		})
+	})
+
 	t.Run("Groups", func(t *testing.T) {
 		t.Parallel()
 		t.Run("Assigns", func(t *testing.T) {

diff --git a/site/src/api/typesGenerated.ts b/site/src/api/typesGenerated.ts
@@ -605,6 +605,12 @@ export interface OIDCConfig {
   // Named type "github.com/coder/coder/cli/clibase.Struct[map[string]string]" unknown, using "any"
   // eslint-disable-next-line @typescript-eslint/no-explicit-any -- External type
   readonly group_mapping: any
+  readonly user_role_field: string
+  // Named type "github.com/coder/coder/cli/clibase.Struct[map[string][]string]" unknown, using "any"
+  // eslint-disable-next-line @typescript-eslint/no-explicit-any -- External type
+  readonly user_role_mapping: any
+  // This is likely an enum in an external package ("github.com/coder/coder/cli/clibase.StringArray")
+  readonly user_roles_default: string[]
   readonly sign_in_text: string
   readonly icon_url: string
 }