feat: Member roles are implied and never exlpicitly added

coder · Emyrk · Jun 1, 2022 · May 31, 2022 · May 31, 2022 · May 31, 2022
commit 501e581a877779c2af1838e99500e675779fb82a
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
@@ -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,6 +295,7 @@ 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())
 		}
 	}
 

diff --git a/coderd/database/queries/users.sql b/coderd/database/queries/users.sql
@@ -136,10 +136,16 @@ WHERE
 
 -- name: GetAllUserRoles :one
 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,18 @@ func APIKey(r *http.Request) database.APIKey {
 	return apiKey
 }
 
+// User roles are the 'subject' field of Authorize()
+type userRolesKey struct{}
+
+// UserRoles returns the API key from the ExtractUserRoles handler.
+func UserRoles(r *http.Request) database.GetAllUserRolesRow {
+	apiKey, ok := r.Context().Value(userRolesKey{}).(database.GetAllUserRolesRow)
+	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 {

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
 			},
 		},
 	}

diff --git a/coderd/members.go b/coderd/members.go
@@ -26,7 +26,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,8 +73,6 @@ 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),
 			},
 		})

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/users.go b/coderd/users.go
@@ -88,7 +88,7 @@ func (api *API) postFirstUser(rw http.ResponseWriter, r *http.Request) {
 	//	and add some rbac bypass when calling api functions this way??
 	// Add the admin role to this first user.
 	_, err = api.Database.UpdateUserRoles(r.Context(), database.UpdateUserRolesParams{
-		GrantedRoles: []string{rbac.RoleAdmin(), rbac.RoleMember()},
+		GrantedRoles: []string{rbac.RoleAdmin()},
 		ID:           user.ID,
 	})
 	if err != nil {
@@ -480,7 +480,9 @@ func (api *API) putUserRoles(rw http.ResponseWriter, r *http.Request) {
 		return
 	}
 
-	added, removed := rbac.ChangeRoleSet(roles.Roles, params.Roles)
+	// The member role is always implied.
+	impliedTypes := append(params.Roles, rbac.RoleMember())
+	added, removed := rbac.ChangeRoleSet(roles.Roles, impliedTypes)
 	for _, roleName := range added {
 		// Assigning a role requires the create permission.
 		if !api.Authorize(rw, r, rbac.ActionCreate, rbac.ResourceRoleAssignment.WithID(roleName)) {
@@ -764,8 +766,6 @@ func (api *API) createUser(ctx context.Context, req codersdk.CreateUserRequest)
 			req.OrganizationID = organization.ID
 			orgRoles = append(orgRoles, rbac.RoleOrgAdmin(req.OrganizationID))
 		}
-		// Always also be a member.
-		orgRoles = append(orgRoles, rbac.RoleOrgMember(req.OrganizationID))
 
 		params := database.InsertUserParams{
 			ID:        uuid.New(),
@@ -774,7 +774,7 @@ func (api *API) createUser(ctx context.Context, req codersdk.CreateUserRequest)
 			CreatedAt: database.Now(),
 			UpdatedAt: database.Now(),
 			// All new users are defaulted to members of the site.
-			RBACRoles: []string{rbac.RoleMember()},
+			RBACRoles: []string{},
 		}
 		// If a user signs up with OAuth, they can have no password!
 		if req.Password != "" {

diff --git a/coderd/users_test.go b/coderd/users_test.go
@@ -416,43 +416,43 @@ func TestGrantRoles(t *testing.T) {
 		require.NoError(t, err, "member user")
 
 		_, err = admin.UpdateUserRoles(ctx, codersdk.Me, codersdk.UpdateRoles{
-			Roles: []string{rbac.RoleOrgMember(first.OrganizationID)},
+			Roles: []string{rbac.RoleOrgAdmin(first.OrganizationID)},
 		})
 		require.Error(t, err, "org role in site")
 		requireStatusCode(t, err, http.StatusBadRequest)
 
 		_, err = admin.UpdateUserRoles(ctx, uuid.New().String(), codersdk.UpdateRoles{
-			Roles: []string{rbac.RoleOrgMember(first.OrganizationID)},
+			Roles: []string{rbac.RoleOrgAdmin(first.OrganizationID)},
 		})
 		require.Error(t, err, "user does not exist")
 		requireStatusCode(t, err, http.StatusBadRequest)
 
 		_, err = admin.UpdateOrganizationMemberRoles(ctx, first.OrganizationID, codersdk.Me, codersdk.UpdateRoles{
-			Roles: []string{rbac.RoleMember()},
+			Roles: []string{rbac.RoleAdmin()},
 		})
 		require.Error(t, err, "site role in org")
 		requireStatusCode(t, err, http.StatusBadRequest)
 
 		_, err = admin.UpdateOrganizationMemberRoles(ctx, uuid.New(), codersdk.Me, codersdk.UpdateRoles{
-			Roles: []string{rbac.RoleMember()},
+			Roles: []string{},
 		})
 		require.Error(t, err, "role in org without membership")
 		requireStatusCode(t, err, http.StatusNotFound)
 
 		_, err = member.UpdateUserRoles(ctx, first.UserID.String(), codersdk.UpdateRoles{
-			Roles: []string{rbac.RoleMember()},
+			Roles: []string{},
 		})
 		require.Error(t, err, "member cannot change other's roles")
 		requireStatusCode(t, err, http.StatusForbidden)
 
 		_, err = member.UpdateUserRoles(ctx, memberUser.ID.String(), codersdk.UpdateRoles{
-			Roles: []string{rbac.RoleMember()},
+			Roles: []string{},
 		})
 		require.Error(t, err, "member cannot change any roles")
 		requireStatusCode(t, err, http.StatusForbidden)
 
 		_, err = member.UpdateOrganizationMemberRoles(ctx, first.OrganizationID, first.UserID.String(), codersdk.UpdateRoles{
-			Roles: []string{rbac.RoleMember()},
+			Roles: []string{},
 		})
 		require.Error(t, err, "member cannot change other's org roles")
 		requireStatusCode(t, err, http.StatusForbidden)
@@ -468,11 +468,9 @@ func TestGrantRoles(t *testing.T) {
 		require.NoError(t, err)
 		require.ElementsMatch(t, roles.Roles, []string{
 			rbac.RoleAdmin(),
-			rbac.RoleMember(),
 		}, "should be a member and admin")
 
 		require.ElementsMatch(t, roles.OrganizationRoles[first.OrganizationID], []string{
-			rbac.RoleOrgMember(first.OrganizationID),
 			rbac.RoleOrgAdmin(first.OrganizationID),
 		}, "should be a member and admin")
 	})
@@ -486,12 +484,10 @@ func TestGrantRoles(t *testing.T) {
 		member := coderdtest.CreateAnotherUser(t, admin, first.OrganizationID)
 		roles, err := member.GetUserRoles(ctx, codersdk.Me)
 		require.NoError(t, err)
-		require.ElementsMatch(t, roles.Roles, []string{
-			rbac.RoleMember(),
-		}, "should be a member and admin")
+		require.ElementsMatch(t, roles.Roles, []string{}, "should be a member")
 		require.ElementsMatch(t,
 			roles.OrganizationRoles[first.OrganizationID],
-			[]string{rbac.RoleOrgMember(first.OrganizationID)},
+			[]string{},
 		)
 
 		memberUser, err := member.User(ctx, codersdk.Me)
@@ -501,7 +497,6 @@ func TestGrantRoles(t *testing.T) {
 		_, err = admin.UpdateUserRoles(ctx, memberUser.ID.String(), codersdk.UpdateRoles{
 			Roles: []string{
 				// Promote to site admin
-				rbac.RoleMember(),
 				rbac.RoleAdmin(),
 			},
 		})
@@ -511,7 +506,6 @@ func TestGrantRoles(t *testing.T) {
 		_, err = member.UpdateOrganizationMemberRoles(ctx, first.OrganizationID, codersdk.Me, codersdk.UpdateRoles{
 			Roles: []string{
 				// Promote to org admin
-				rbac.RoleOrgMember(first.OrganizationID),
 				rbac.RoleOrgAdmin(first.OrganizationID),
 			},
 		})
@@ -520,12 +514,10 @@ func TestGrantRoles(t *testing.T) {
 		roles, err = member.GetUserRoles(ctx, codersdk.Me)
 		require.NoError(t, err)
 		require.ElementsMatch(t, roles.Roles, []string{
-			rbac.RoleMember(),
 			rbac.RoleAdmin(),
 		}, "should be a member and admin")
 
 		require.ElementsMatch(t, roles.OrganizationRoles[first.OrganizationID], []string{
-			rbac.RoleOrgMember(first.OrganizationID),
 			rbac.RoleOrgAdmin(first.OrganizationID),
 		}, "should be a member and admin")
 	})

diff --git a/coderd/workspaces_test.go b/coderd/workspaces_test.go
@@ -88,7 +88,7 @@ func TestAdminViewAllWorkspaces(t *testing.T) {
 
 	// This other user is not in the first user's org. Since other is an admin, they can
 	// still see the "first" user's workspace.
-	other := coderdtest.CreateAnotherUser(t, client, otherOrg.ID, rbac.RoleAdmin(), rbac.RoleMember())
+	other := coderdtest.CreateAnotherUser(t, client, otherOrg.ID, rbac.RoleAdmin())
 	otherWorkspaces, err := other.Workspaces(context.Background(), codersdk.WorkspaceFilter{})
 	require.NoError(t, err, "(other) fetch workspaces")
 

diff --git a/codersdk/users.go b/codersdk/users.go
@@ -69,7 +69,7 @@ type UpdateUserPasswordRequest struct {
 }
 
 type UpdateRoles struct {
-	Roles []string `json:"roles" validate:"required"`
+	Roles []string `json:"roles" validate:""`
 }
 
 type UserRoles struct {