coder · Emyrk · Jun 1, 2022 · May 31, 2022 · May 31, 2022 · May 31, 2022
diff --git a/coderd/authorize.go b/coderd/authorize.go
@@ -13,12 +13,12 @@ import (
 )
 
 func AuthorizeFilter[O rbac.Objecter](api *API, r *http.Request, action rbac.Action, objects []O) []O {
-	roles := httpmw.UserRoles(r)
+	roles := httpmw.AuthorizationUserRoles(r)
 	return rbac.Filter(r.Context(), api.Authorizer, roles.ID.String(), roles.Roles, action, objects)
 }
 
 func (api *API) Authorize(rw http.ResponseWriter, r *http.Request, action rbac.Action, object rbac.Objecter) bool {
-	roles := httpmw.UserRoles(r)
+	roles := httpmw.AuthorizationUserRoles(r)
 	err := api.Authorizer.ByRoleName(r.Context(), roles.ID.String(), roles.Roles, action, object.RBACObject())
 	if err != nil {
 		httpapi.Write(rw, http.StatusForbidden, httpapi.Response{

diff --git a/coderd/coderdtest/coderdtest.go b/coderd/coderdtest/coderdtest.go
@@ -281,7 +281,7 @@ func CreateAnotherUser(t *testing.T, client *codersdk.Client, organizationID uui
 			organizationID, err := uuid.Parse(orgID)
 			require.NoError(t, err, fmt.Sprintf("parse org id %q", orgID))
 			_, err = client.UpdateOrganizationMemberRoles(context.Background(), organizationID, user.ID.String(),
-				codersdk.UpdateRoles{Roles: append(roles, rbac.RoleOrgMember(organizationID))})
+				codersdk.UpdateRoles{Roles: roles})
 			require.NoError(t, err, "update org membership roles")
 		}
 	}

diff --git a/coderd/database/databasefake/databasefake.go b/coderd/database/databasefake/databasefake.go
@@ -276,7 +276,7 @@ func (q *fakeQuerier) GetUsersByIDs(_ context.Context, ids []uuid.UUID) ([]datab
 	return users, nil
 }
 
-func (q *fakeQuerier) GetAllUserRoles(_ context.Context, userID uuid.UUID) (database.GetAllUserRolesRow, error) {
+func (q *fakeQuerier) GetAuthorizationUserRoles(_ context.Context, userID uuid.UUID) (database.GetAuthorizationUserRolesRow, error) {
 	q.mutex.RLock()
 	defer q.mutex.RUnlock()
 
@@ -286,6 +286,7 @@ func (q *fakeQuerier) GetAllUserRoles(_ context.Context, userID uuid.UUID) (data
 		if u.ID == userID {
 			u := u
 			roles = append(roles, u.RBACRoles...)
+			roles = append(roles, "member")
 			user = &u
 			break
 		}
@@ -294,14 +295,15 @@ func (q *fakeQuerier) GetAllUserRoles(_ context.Context, userID uuid.UUID) (data
 	for _, mem := range q.organizationMembers {
 		if mem.UserID == userID {
 			roles = append(roles, mem.Roles...)
+			roles = append(roles, "organization-member:"+mem.OrganizationID.String())
 		}
 	}
 
 	if user == nil {
-		return database.GetAllUserRolesRow{}, sql.ErrNoRows
+		return database.GetAuthorizationUserRolesRow{}, sql.ErrNoRows
 	}
 
-	return database.GetAllUserRolesRow{
+	return database.GetAuthorizationUserRolesRow{
 		ID:       userID,
 		Username: user.Username,
 		Status:   user.Status,

diff --git a/coderd/database/migrations/000016_drop_member_roles.down.sql b/coderd/database/migrations/000016_drop_member_roles.down.sql
@@ -0,0 +1,11 @@
+--- Remove the now implied 'member' role.
+UPDATE
+	users
+SET
+	rbac_roles = array_append(rbac_roles, 'member');
+
+--- Remove the now implied 'organization-member' role.
+UPDATE
+	organization_members
+SET
+	roles = array_append(roles, 'organization-member:'||organization_id::text);
diff --git a/coderd/database/migrations/000016_drop_member_roles.up.sql b/coderd/database/migrations/000016_drop_member_roles.up.sql
@@ -0,0 +1,11 @@
+--- Remove the now implied 'member' role.
+UPDATE
+	users
+SET
+	rbac_roles = array_remove(rbac_roles, 'member');
+
+--- Remove the now implied 'organization-member' role.
+UPDATE
+	organization_members
+SET
+	roles = array_remove(roles, 'organization-member:'||organization_id::text);
diff --git a/coderd/database/querier.go b/coderd/database/querier.go
diff --git a/coderd/database/queries.sql.go b/coderd/database/queries.sql.go
diff --git a/coderd/database/queries/users.sql b/coderd/database/queries/users.sql
@@ -134,12 +134,20 @@ WHERE
 	id = $1 RETURNING *;
 
 
--- name: GetAllUserRoles :one
+-- name: GetAuthorizationUserRoles :one
+-- This function returns roles for authorization purposes. Implied member roles
+-- are included.
 SELECT
-    -- username is returned just to help for logging purposes
-    -- status is used to enforce 'suspended' users, as all roles are ignored
-    --	when suspended.
-	id, username, status, array_cat(users.rbac_roles, organization_members.roles) :: text[] AS roles
+	-- username is returned just to help for logging purposes
+	-- status is used to enforce 'suspended' users, as all roles are ignored
+	--	when suspended.
+	id, username, status,
+	array_cat(
+		-- All users are members
+			array_append(users.rbac_roles, 'member'),
+		-- All org_members get the org-member role for their orgs
+			array_append(organization_members.roles, 'organization-member:'||organization_members.organization_id::text)) :: text[]
+	    AS roles
 FROM
 	users
 LEFT JOIN organization_members

diff --git a/coderd/httpmw/apikey.go b/coderd/httpmw/apikey.go
@@ -31,6 +31,19 @@ func APIKey(r *http.Request) database.APIKey {
 	return apiKey
 }
 
+// User roles are the 'subject' field of Authorize()
+type userRolesKey struct{}
+
+// AuthorizationUserRoles returns the roles used for authorization.
+// Comes from the ExtractAPIKey handler.
+func AuthorizationUserRoles(r *http.Request) database.GetAuthorizationUserRolesRow {
+	apiKey, ok := r.Context().Value(userRolesKey{}).(database.GetAuthorizationUserRolesRow)
+	if !ok {
+		panic("developer error: user roles middleware not provided")
+	}
+	return apiKey
+}
+
 // OAuth2Configs is a collection of configurations for OAuth-based authentication.
 // This should be extended to support other authentication types in the future.
 type OAuth2Configs struct {
@@ -178,7 +191,7 @@ func ExtractAPIKey(db database.Store, oauth *OAuth2Configs) func(http.Handler) h
 			// If the key is valid, we also fetch the user roles and status.
 			// The roles are used for RBAC authorize checks, and the status
 			// is to block 'suspended' users from accessing the platform.
-			roles, err := db.GetAllUserRoles(r.Context(), key.UserID)
+			roles, err := db.GetAuthorizationUserRoles(r.Context(), key.UserID)
 			if err != nil {
 				httpapi.Write(rw, http.StatusUnauthorized, httpapi.Response{
 					Message: "roles not found",

diff --git a/coderd/httpmw/authorize.go b/coderd/httpmw/authorize.go
diff --git a/coderd/httpmw/authorize_test.go b/coderd/httpmw/authorize_test.go
@@ -31,23 +31,23 @@ func TestExtractUserRoles(t *testing.T) {
 		{
 			Name: "Member",
 			AddUser: func(db database.Store) (database.User, []string, string) {
-				roles := []string{rbac.RoleMember()}
+				roles := []string{}
 				user, token := addUser(t, db, roles...)
-				return user, roles, token
+				return user, append(roles, rbac.RoleMember()), token
 			},
 		},
 		{
 			Name: "Admin",
 			AddUser: func(db database.Store) (database.User, []string, string) {
-				roles := []string{rbac.RoleMember(), rbac.RoleAdmin()}
+				roles := []string{rbac.RoleAdmin()}
 				user, token := addUser(t, db, roles...)
-				return user, roles, token
+				return user, append(roles, rbac.RoleMember()), token
 			},
 		},
 		{
 			Name: "OrgMember",
 			AddUser: func(db database.Store) (database.User, []string, string) {
-				roles := []string{rbac.RoleMember()}
+				roles := []string{}
 				user, token := addUser(t, db, roles...)
 				org, err := db.InsertOrganization(context.Background(), database.InsertOrganizationParams{
 					ID:          uuid.New(),
@@ -58,7 +58,7 @@ func TestExtractUserRoles(t *testing.T) {
 				})
 				require.NoError(t, err)
 
-				orgRoles := []string{rbac.RoleOrgMember(org.ID)}
+				orgRoles := []string{}
 				_, err = db.InsertOrganizationMember(context.Background(), database.InsertOrganizationMemberParams{
 					OrganizationID: org.ID,
 					UserID:         user.ID,
@@ -67,7 +67,7 @@ func TestExtractUserRoles(t *testing.T) {
 					Roles:          orgRoles,
 				})
 				require.NoError(t, err)
-				return user, append(roles, orgRoles...), token
+				return user, append(roles, append(orgRoles, rbac.RoleMember(), rbac.RoleOrgMember(org.ID))...), token
 			},
 		},
 	}
@@ -86,7 +86,7 @@ func TestExtractUserRoles(t *testing.T) {
 				httpmw.ExtractAPIKey(db, &httpmw.OAuth2Configs{}),
 			)
 			rtr.Get("/", func(_ http.ResponseWriter, r *http.Request) {
-				roles := httpmw.UserRoles(r)
+				roles := httpmw.AuthorizationUserRoles(r)
 				require.ElementsMatch(t, user.ID, roles.ID)
 				require.ElementsMatch(t, expRoles, roles.Roles)
 			})

diff --git a/coderd/members.go b/coderd/members.go
@@ -34,7 +34,9 @@ func (api *API) putMemberRoles(rw http.ResponseWriter, r *http.Request) {
 		return
 	}
 
-	added, removed := rbac.ChangeRoleSet(member.Roles, params.Roles)
+	// The org-member role is always implied.
+	impliedTypes := append(params.Roles, rbac.RoleOrgMember(organization.ID))
+	added, removed := rbac.ChangeRoleSet(member.Roles, impliedTypes)
 	for _, roleName := range added {
 		// Assigning a role requires the create permission.
 		if !api.Authorize(rw, r, rbac.ActionCreate, rbac.ResourceOrgRoleAssignment.WithID(roleName).InOrg(organization.ID)) {

diff --git a/coderd/organizations.go b/coderd/organizations.go
@@ -73,13 +73,11 @@ func (api *API) postOrganizations(rw http.ResponseWriter, r *http.Request) {
 			CreatedAt:      database.Now(),
 			UpdatedAt:      database.Now(),
 			Roles: []string{
-				// Also assign member role incase they get demoted from admin
-				rbac.RoleOrgMember(organization.ID),
 				rbac.RoleOrgAdmin(organization.ID),
 			},
 		})
 		if err != nil {
-			return xerrors.Errorf("create organization member: %w", err)
+			return xerrors.Errorf("create organization admin: %w", err)
 		}
 		return nil
 	})

diff --git a/coderd/rbac/builtin.go b/coderd/rbac/builtin.go
@@ -63,7 +63,7 @@ var (
 		member: func(_ string) Role {
 			return Role{
 				Name:        member,
-				DisplayName: "Member",
+				DisplayName: "",
 				Site: permissions(map[Object][]Action{
 					// All users can read all other users and know they exist.
 					ResourceUser:           {ActionRead},
@@ -116,7 +116,7 @@ var (
 		orgMember: func(organizationID string) Role {
 			return Role{
 				Name:        roleName(orgMember, organizationID),
-				DisplayName: "Organization Member",
+				DisplayName: "",
 				Org: map[string][]Permission{
 					organizationID: {
 						{

diff --git a/coderd/rbac/role.go b/coderd/rbac/role.go
@@ -17,7 +17,9 @@ type Permission struct {
 // Users of this package should instead **only** use the role names, and
 // this package will expand the role names into their json payloads.
 type Role struct {
-	Name        string       `json:"name"`
+	Name string `json:"name"`
+	// DisplayName is used for UI purposes. If the role has no display name,
+	// that means the UI should never display it.
 	DisplayName string       `json:"display_name"`
 	Site        []Permission `json:"site"`
 	// Org is a map of orgid to permissions. We represent orgid as a string.

diff --git a/coderd/roles.go b/coderd/roles.go
@@ -45,7 +45,7 @@ func (api *API) checkPermissions(rw http.ResponseWriter, r *http.Request) {
 	}
 
 	// use the roles of the user specified, not the person making the request.
-	roles, err := api.Database.GetAllUserRoles(r.Context(), user.ID)
+	roles, err := api.Database.GetAuthorizationUserRoles(r.Context(), user.ID)
 	if err != nil {
 		httpapi.Forbidden(rw)
 		return
@@ -91,6 +91,10 @@ func convertRole(role rbac.Role) codersdk.Role {
 func convertRoles(roles []rbac.Role) []codersdk.Role {
 	converted := make([]codersdk.Role, 0, len(roles))
 	for _, role := range roles {
+		// Roles without display names should never be shown to the ui.
+		if role.DisplayName == "" {
+			continue
+		}
 		converted = append(converted, convertRole(role))
 	}
 	return converted