Skip to content

Commit 0c7bc32

Browse files
Emyrkkylecarbs
Emyrk
authored and
kylecarbs
committed
feat: Implied 'member' roles for site and organization (#1917)
* feat: Member roles are implied and never exlpicitly added * Rename "GetAllUserRoles" to "GetAuthorizationRoles" * feat: Add migration to remove implied roles * rename user auth role middleware
1 parent c4dbcf5 commit 0c7bc32

21 files changed

+131
-115
lines changed

‎coderd/authorize.go

+2-2
Original file line numberDiff line numberDiff line change
@@ -13,12 +13,12 @@ import (
1313
)
1414

1515
func AuthorizeFilter[O rbac.Objecter](api *API, r *http.Request, action rbac.Action, objects []O) []O {
16-
roles := httpmw.UserRoles(r)
16+
roles := httpmw.AuthorizationUserRoles(r)
1717
return rbac.Filter(r.Context(), api.Authorizer, roles.ID.String(), roles.Roles, action, objects)
1818
}
1919

2020
func (api *API) Authorize(rw http.ResponseWriter, r *http.Request, action rbac.Action, object rbac.Objecter) bool {
21-
roles := httpmw.UserRoles(r)
21+
roles := httpmw.AuthorizationUserRoles(r)
2222
err := api.Authorizer.ByRoleName(r.Context(), roles.ID.String(), roles.Roles, action, object.RBACObject())
2323
if err != nil {
2424
httpapi.Write(rw, http.StatusForbidden, httpapi.Response{

‎coderd/coderdtest/coderdtest.go

+1-1
Original file line numberDiff line numberDiff line change
@@ -281,7 +281,7 @@ func CreateAnotherUser(t *testing.T, client *codersdk.Client, organizationID uui
281281
organizationID, err := uuid.Parse(orgID)
282282
require.NoError(t, err, fmt.Sprintf("parse org id %q", orgID))
283283
_, err = client.UpdateOrganizationMemberRoles(context.Background(), organizationID, user.ID.String(),
284-
codersdk.UpdateRoles{Roles: append(roles, rbac.RoleOrgMember(organizationID))})
284+
codersdk.UpdateRoles{Roles: roles})
285285
require.NoError(t, err, "update org membership roles")
286286
}
287287
}

‎coderd/database/databasefake/databasefake.go

+5-3
Original file line numberDiff line numberDiff line change
@@ -276,7 +276,7 @@ func (q *fakeQuerier) GetUsersByIDs(_ context.Context, ids []uuid.UUID) ([]datab
276276
return users, nil
277277
}
278278

279-
func (q *fakeQuerier) GetAllUserRoles(_ context.Context, userID uuid.UUID) (database.GetAllUserRolesRow, error) {
279+
func (q *fakeQuerier) GetAuthorizationUserRoles(_ context.Context, userID uuid.UUID) (database.GetAuthorizationUserRolesRow, error) {
280280
q.mutex.RLock()
281281
defer q.mutex.RUnlock()
282282

@@ -286,6 +286,7 @@ func (q *fakeQuerier) GetAllUserRoles(_ context.Context, userID uuid.UUID) (data
286286
if u.ID == userID {
287287
u := u
288288
roles = append(roles, u.RBACRoles...)
289+
roles = append(roles, "member")
289290
user = &u
290291
break
291292
}
@@ -294,14 +295,15 @@ func (q *fakeQuerier) GetAllUserRoles(_ context.Context, userID uuid.UUID) (data
294295
for _, mem := range q.organizationMembers {
295296
if mem.UserID == userID {
296297
roles = append(roles, mem.Roles...)
298+
roles = append(roles, "organization-member:"+mem.OrganizationID.String())
297299
}
298300
}
299301

300302
if user == nil {
301-
return database.GetAllUserRolesRow{}, sql.ErrNoRows
303+
return database.GetAuthorizationUserRolesRow{}, sql.ErrNoRows
302304
}
303305

304-
return database.GetAllUserRolesRow{
306+
return database.GetAuthorizationUserRolesRow{
305307
ID: userID,
306308
Username: user.Username,
307309
Status: user.Status,

‎coderd/database/migrations/000016_drop_member_roles.down.sql

+11
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,11 @@
1+
--- Remove the now implied 'member' role.
2+
UPDATE
3+
users
4+
SET
5+
rbac_roles = array_append(rbac_roles, 'member');
6+
7+
--- Remove the now implied 'organization-member' role.
8+
UPDATE
9+
organization_members
10+
SET
11+
roles = array_append(roles, 'organization-member:'||organization_id::text);

‎coderd/database/migrations/000016_drop_member_roles.up.sql

+11
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,11 @@
1+
--- Remove the now implied 'member' role.
2+
UPDATE
3+
users
4+
SET
5+
rbac_roles = array_remove(rbac_roles, 'member');
6+
7+
--- Remove the now implied 'organization-member' role.
8+
UPDATE
9+
organization_members
10+
SET
11+
roles = array_remove(roles, 'organization-member:'||organization_id::text);

‎coderd/database/querier.go

+3-1
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎coderd/database/queries.sql.go

+17-9
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎coderd/database/queries/users.sql

+13-5
Original file line numberDiff line numberDiff line change
@@ -134,12 +134,20 @@ WHERE
134134
id = $1 RETURNING *;
135135

136136

137-
-- name: GetAllUserRoles :one
137+
-- name: GetAuthorizationUserRoles :one
138+
-- This function returns roles for authorization purposes. Implied member roles
139+
-- are included.
138140
SELECT
139-
-- username is returned just to help for logging purposes
140-
-- status is used to enforce 'suspended' users, as all roles are ignored
141-
-- when suspended.
142-
id, username, status, array_cat(users.rbac_roles, organization_members.roles) :: text[] AS roles
141+
-- username is returned just to help for logging purposes
142+
-- status is used to enforce 'suspended' users, as all roles are ignored
143+
-- when suspended.
144+
id, username, status,
145+
array_cat(
146+
-- All users are members
147+
array_append(users.rbac_roles, 'member'),
148+
-- All org_members get the org-member role for their orgs
149+
array_append(organization_members.roles, 'organization-member:'||organization_members.organization_id::text)) :: text[]
150+
AS roles
143151
FROM
144152
users
145153
LEFT JOIN organization_members

‎coderd/httpmw/apikey.go

+14-1
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,19 @@ func APIKey(r *http.Request) database.APIKey {
3131
return apiKey
3232
}
3333

34+
// User roles are the 'subject' field of Authorize()
35+
type userRolesKey struct{}
36+
37+
// AuthorizationUserRoles returns the roles used for authorization.
38+
// Comes from the ExtractAPIKey handler.
39+
func AuthorizationUserRoles(r *http.Request) database.GetAuthorizationUserRolesRow {
40+
apiKey, ok := r.Context().Value(userRolesKey{}).(database.GetAuthorizationUserRolesRow)
41+
if !ok {
42+
panic("developer error: user roles middleware not provided")
43+
}
44+
return apiKey
45+
}
46+
3447
// OAuth2Configs is a collection of configurations for OAuth-based authentication.
3548
// This should be extended to support other authentication types in the future.
3649
type OAuth2Configs struct {
@@ -178,7 +191,7 @@ func ExtractAPIKey(db database.Store, oauth *OAuth2Configs) func(http.Handler) h
178191
// If the key is valid, we also fetch the user roles and status.
179192
// The roles are used for RBAC authorize checks, and the status
180193
// is to block 'suspended' users from accessing the platform.
181-
roles, err := db.GetAllUserRoles(r.Context(), key.UserID)
194+
roles, err := db.GetAuthorizationUserRoles(r.Context(), key.UserID)
182195
if err != nil {
183196
httpapi.Write(rw, http.StatusUnauthorized, httpapi.Response{
184197
Message: "roles not found",

‎coderd/httpmw/authorize.go

-40
This file was deleted.

‎coderd/httpmw/authorize_test.go

+8-8
Original file line numberDiff line numberDiff line change
@@ -31,23 +31,23 @@ func TestExtractUserRoles(t *testing.T) {
3131
{
3232
Name: "Member",
3333
AddUser: func(db database.Store) (database.User, []string, string) {
34-
roles := []string{rbac.RoleMember()}
34+
roles := []string{}
3535
user, token := addUser(t, db, roles...)
36-
return user, roles, token
36+
return user, append(roles, rbac.RoleMember()), token
3737
},
3838
},
3939
{
4040
Name: "Admin",
4141
AddUser: func(db database.Store) (database.User, []string, string) {
42-
roles := []string{rbac.RoleMember(), rbac.RoleAdmin()}
42+
roles := []string{rbac.RoleAdmin()}
4343
user, token := addUser(t, db, roles...)
44-
return user, roles, token
44+
return user, append(roles, rbac.RoleMember()), token
4545
},
4646
},
4747
{
4848
Name: "OrgMember",
4949
AddUser: func(db database.Store) (database.User, []string, string) {
50-
roles := []string{rbac.RoleMember()}
50+
roles := []string{}
5151
user, token := addUser(t, db, roles...)
5252
org, err := db.InsertOrganization(context.Background(), database.InsertOrganizationParams{
5353
ID: uuid.New(),
@@ -58,7 +58,7 @@ func TestExtractUserRoles(t *testing.T) {
5858
})
5959
require.NoError(t, err)
6060

61-
orgRoles := []string{rbac.RoleOrgMember(org.ID)}
61+
orgRoles := []string{}
6262
_, err = db.InsertOrganizationMember(context.Background(), database.InsertOrganizationMemberParams{
6363
OrganizationID: org.ID,
6464
UserID: user.ID,
@@ -67,7 +67,7 @@ func TestExtractUserRoles(t *testing.T) {
6767
Roles: orgRoles,
6868
})
6969
require.NoError(t, err)
70-
return user, append(roles, orgRoles...), token
70+
return user, append(roles, append(orgRoles, rbac.RoleMember(), rbac.RoleOrgMember(org.ID))...), token
7171
},
7272
},
7373
}
@@ -86,7 +86,7 @@ func TestExtractUserRoles(t *testing.T) {
8686
httpmw.ExtractAPIKey(db, &httpmw.OAuth2Configs{}),
8787
)
8888
rtr.Get("/", func(_ http.ResponseWriter, r *http.Request) {
89-
roles := httpmw.UserRoles(r)
89+
roles := httpmw.AuthorizationUserRoles(r)
9090
require.ElementsMatch(t, user.ID, roles.ID)
9191
require.ElementsMatch(t, expRoles, roles.Roles)
9292
})

‎coderd/members.go

+3-1
Original file line numberDiff line numberDiff line change
@@ -34,7 +34,9 @@ func (api *API) putMemberRoles(rw http.ResponseWriter, r *http.Request) {
3434
return
3535
}
3636

37-
added, removed := rbac.ChangeRoleSet(member.Roles, params.Roles)
37+
// The org-member role is always implied.
38+
impliedTypes := append(params.Roles, rbac.RoleOrgMember(organization.ID))
39+
added, removed := rbac.ChangeRoleSet(member.Roles, impliedTypes)
3840
for _, roleName := range added {
3941
// Assigning a role requires the create permission.
4042
if !api.Authorize(rw, r, rbac.ActionCreate, rbac.ResourceOrgRoleAssignment.WithID(roleName).InOrg(organization.ID)) {

‎coderd/organizations.go

+1-3
Original file line numberDiff line numberDiff line change
@@ -73,13 +73,11 @@ func (api *API) postOrganizations(rw http.ResponseWriter, r *http.Request) {
7373
CreatedAt: database.Now(),
7474
UpdatedAt: database.Now(),
7575
Roles: []string{
76-
// Also assign member role incase they get demoted from admin
77-
rbac.RoleOrgMember(organization.ID),
7876
rbac.RoleOrgAdmin(organization.ID),
7977
},
8078
})
8179
if err != nil {
82-
return xerrors.Errorf("create organization member: %w", err)
80+
return xerrors.Errorf("create organization admin: %w", err)
8381
}
8482
return nil
8583
})

‎coderd/rbac/builtin.go

+2-2
Original file line numberDiff line numberDiff line change
@@ -63,7 +63,7 @@ var (
6363
member: func(_ string) Role {
6464
return Role{
6565
Name: member,
66-
DisplayName: "Member",
66+
DisplayName: "",
6767
Site: permissions(map[Object][]Action{
6868
// All users can read all other users and know they exist.
6969
ResourceUser: {ActionRead},
@@ -116,7 +116,7 @@ var (
116116
orgMember: func(organizationID string) Role {
117117
return Role{
118118
Name: roleName(orgMember, organizationID),
119-
DisplayName: "Organization Member",
119+
DisplayName: "",
120120
Org: map[string][]Permission{
121121
organizationID: {
122122
{

‎coderd/rbac/role.go

+3-1
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,9 @@ type Permission struct {
1717
// Users of this package should instead **only** use the role names, and
1818
// this package will expand the role names into their json payloads.
1919
type Role struct {
20-
Name string `json:"name"`
20+
Name string `json:"name"`
21+
// DisplayName is used for UI purposes. If the role has no display name,
22+
// that means the UI should never display it.
2123
DisplayName string `json:"display_name"`
2224
Site []Permission `json:"site"`
2325
// Org is a map of orgid to permissions. We represent orgid as a string.

‎coderd/roles.go

+5-1
Original file line numberDiff line numberDiff line change
@@ -45,7 +45,7 @@ func (api *API) checkPermissions(rw http.ResponseWriter, r *http.Request) {
4545
}
4646

4747
// use the roles of the user specified, not the person making the request.
48-
roles, err := api.Database.GetAllUserRoles(r.Context(), user.ID)
48+
roles, err := api.Database.GetAuthorizationUserRoles(r.Context(), user.ID)
4949
if err != nil {
5050
httpapi.Forbidden(rw)
5151
return
@@ -91,6 +91,10 @@ func convertRole(role rbac.Role) codersdk.Role {
9191
func convertRoles(roles []rbac.Role) []codersdk.Role {
9292
converted := make([]codersdk.Role, 0, len(roles))
9393
for _, role := range roles {
94+
// Roles without display names should never be shown to the ui.
95+
if role.DisplayName == "" {
96+
continue
97+
}
9498
converted = append(converted, convertRole(role))
9599
}
96100
return converted

0 commit comments

Comments
 (0)