From 501e581a877779c2af1838e99500e675779fb82a Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Tue, 31 May 2022 09:15:57 -0500 Subject: [PATCH 1/8] feat: Member roles are implied and never exlpicitly added --- coderd/coderdtest/coderdtest.go | 2 +- coderd/database/databasefake/databasefake.go | 2 + coderd/database/queries/users.sql | 14 +++++-- coderd/httpmw/apikey.go | 12 ++++++ coderd/httpmw/authorize.go | 40 -------------------- coderd/httpmw/authorize_test.go | 14 +++---- coderd/members.go | 4 +- coderd/organizations.go | 2 - coderd/rbac/builtin.go | 4 +- coderd/rbac/role.go | 4 +- coderd/users.go | 10 ++--- coderd/users_test.go | 26 +++++-------- coderd/workspaces_test.go | 2 +- codersdk/users.go | 2 +- 14 files changed, 56 insertions(+), 82 deletions(-) delete mode 100644 coderd/httpmw/authorize.go diff --git a/coderd/coderdtest/coderdtest.go b/coderd/coderdtest/coderdtest.go index 48fe3b81deb72..6fbe4256d692e 100644 --- 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 index aad1b333a5274..2e051b7b7b39b 100644 --- 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 index a4a8bcfc7bec6..813d43a6d7005 100644 --- 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 index 1e4098c0be431..1f7cfd83e3db0 100644 --- 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 deleted file mode 100644 index 84bf7cbfa04b4..0000000000000 --- a/coderd/httpmw/authorize.go +++ /dev/null @@ -1,40 +0,0 @@ -package httpmw - -import ( - "context" - "net/http" - - "github.com/coder/coder/coderd/database" - "github.com/coder/coder/coderd/httpapi" -) - -// 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 -} - -// ExtractUserRoles requires authentication using a valid API key. -func ExtractUserRoles(db database.Store) func(http.Handler) http.Handler { - return func(next http.Handler) http.Handler { - return http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) { - apiKey := APIKey(r) - role, err := db.GetAllUserRoles(r.Context(), apiKey.UserID) - if err != nil { - httpapi.Write(rw, http.StatusUnauthorized, httpapi.Response{ - Message: "roles not found", - }) - return - } - - ctx := context.WithValue(r.Context(), userRolesKey{}, role) - next.ServeHTTP(rw, r.WithContext(ctx)) - }) - } -} diff --git a/coderd/httpmw/authorize_test.go b/coderd/httpmw/authorize_test.go index 279c037e2ae92..2b344cbce6d4c 100644 --- 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 index 78f44294ae87f..b8cb3f84cf570 100644 --- 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 index 9e15b8511852e..ca5180badfae2 100644 --- 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 index 407d827448b54..f6691c50830e7 100644 --- 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 index c236bf4a3cf59..9913a1091a68c 100644 --- 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 index b8c91f5778d7e..a189b214ea3f6 100644 --- 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 index 6d48f4f2320d8..9b0136d8a196d 100644 --- 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 index 3b0bfe79c728f..19f7f2683b73d 100644 --- 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 index 8d7ec54b196f2..8b3ac9ca87016 100644 --- 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 { From 51521cad4c6e142660824f69453216dcdafd665a Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Tue, 31 May 2022 09:27:25 -0500 Subject: [PATCH 2/8] Rename "GetAllUserRoles" to "GetAuthorizationRoles" --- coderd/authorize.go | 4 ++-- coderd/database/databasefake/databasefake.go | 6 ++--- coderd/database/querier.go | 3 ++- coderd/database/queries.sql.go | 25 +++++++++++++------- coderd/database/queries/users.sql | 4 +++- coderd/httpmw/apikey.go | 9 +++---- coderd/httpmw/authorize_test.go | 2 +- coderd/roles.go | 6 ++++- coderd/users.go | 2 +- 9 files changed, 38 insertions(+), 23 deletions(-) diff --git a/coderd/authorize.go b/coderd/authorize.go index b98ad5e20f83c..ed8f89506421b 100644 --- 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.UserAuthorizationRoles(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.UserAuthorizationRoles(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/database/databasefake/databasefake.go b/coderd/database/databasefake/databasefake.go index 2e051b7b7b39b..9e649db5b32e4 100644 --- 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() @@ -300,10 +300,10 @@ func (q *fakeQuerier) GetAllUserRoles(_ context.Context, userID uuid.UUID) (data } 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/querier.go b/coderd/database/querier.go index 7d4c361cea05b..bddb094c540d8 100644 --- a/coderd/database/querier.go +++ b/coderd/database/querier.go @@ -22,10 +22,11 @@ type querier interface { DeleteGitSSHKey(ctx context.Context, userID uuid.UUID) error DeleteParameterValueByID(ctx context.Context, id uuid.UUID) error GetAPIKeyByID(ctx context.Context, id string) (APIKey, error) - GetAllUserRoles(ctx context.Context, userID uuid.UUID) (GetAllUserRolesRow, error) // GetAuditLogsBefore retrieves `limit` number of audit logs before the provided // ID. GetAuditLogsBefore(ctx context.Context, arg GetAuditLogsBeforeParams) ([]AuditLog, error) + // This function + GetAuthorizationUserRoles(ctx context.Context, userID uuid.UUID) (GetAuthorizationUserRolesRow, error) GetFileByHash(ctx context.Context, hash string) (File, error) GetGitSSHKey(ctx context.Context, userID uuid.UUID) (GitSSHKey, error) GetLatestWorkspaceBuildByWorkspaceID(ctx context.Context, workspaceID uuid.UUID) (WorkspaceBuild, error) diff --git a/coderd/database/queries.sql.go b/coderd/database/queries.sql.go index 29f3eb84f3272..6d16c39454ed9 100644 --- a/coderd/database/queries.sql.go +++ b/coderd/database/queries.sql.go @@ -2088,12 +2088,18 @@ func (q *sqlQuerier) UpdateTemplateVersionDescriptionByJobID(ctx context.Context return err } -const getAllUserRoles = `-- name: GetAllUserRoles :one +const getAuthorizationUserRoles = `-- name: GetAuthorizationUserRoles :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 @@ -2102,16 +2108,17 @@ WHERE id = $1 ` -type GetAllUserRolesRow struct { +type GetAuthorizationUserRolesRow struct { ID uuid.UUID `db:"id" json:"id"` Username string `db:"username" json:"username"` Status UserStatus `db:"status" json:"status"` Roles []string `db:"roles" json:"roles"` } -func (q *sqlQuerier) GetAllUserRoles(ctx context.Context, userID uuid.UUID) (GetAllUserRolesRow, error) { - row := q.db.QueryRowContext(ctx, getAllUserRoles, userID) - var i GetAllUserRolesRow +// This function +func (q *sqlQuerier) GetAuthorizationUserRoles(ctx context.Context, userID uuid.UUID) (GetAuthorizationUserRolesRow, error) { + row := q.db.QueryRowContext(ctx, getAuthorizationUserRoles, userID) + var i GetAuthorizationUserRolesRow err := row.Scan( &i.ID, &i.Username, diff --git a/coderd/database/queries/users.sql b/coderd/database/queries/users.sql index 813d43a6d7005..be4a42a1538b7 100644 --- a/coderd/database/queries/users.sql +++ b/coderd/database/queries/users.sql @@ -134,7 +134,9 @@ 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 diff --git a/coderd/httpmw/apikey.go b/coderd/httpmw/apikey.go index 1f7cfd83e3db0..1ee46bca3e09e 100644 --- a/coderd/httpmw/apikey.go +++ b/coderd/httpmw/apikey.go @@ -34,9 +34,10 @@ func APIKey(r *http.Request) database.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) +// UserAuthorizationRoles returns the roles used for authorization. +// Comes from the ExtractAPIKey handler. +func UserAuthorizationRoles(r *http.Request) database.GetAuthorizationUserRolesRow { + apiKey, ok := r.Context().Value(userRolesKey{}).(database.GetAuthorizationUserRolesRow) if !ok { panic("developer error: user roles middleware not provided") } @@ -190,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_test.go b/coderd/httpmw/authorize_test.go index 2b344cbce6d4c..423d1203e1d85 100644 --- a/coderd/httpmw/authorize_test.go +++ b/coderd/httpmw/authorize_test.go @@ -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.UserAuthorizationRoles(r) require.ElementsMatch(t, user.ID, roles.ID) require.ElementsMatch(t, expRoles, roles.Roles) }) diff --git a/coderd/roles.go b/coderd/roles.go index eff35eb016516..4443ec1e6dc5a 100644 --- 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 diff --git a/coderd/users.go b/coderd/users.go index a189b214ea3f6..d1d5796528075 100644 --- a/coderd/users.go +++ b/coderd/users.go @@ -473,7 +473,7 @@ func (api *API) userRoles(rw http.ResponseWriter, r *http.Request) { func (api *API) putUserRoles(rw http.ResponseWriter, r *http.Request) { // User is the user to modify. user := httpmw.UserParam(r) - roles := httpmw.UserRoles(r) + roles := httpmw.UserAuthorizationRoles(r) var params codersdk.UpdateRoles if !httpapi.Read(rw, r, ¶ms) { From 844c9ce9da7e19ebe2c79cc3be8739f2f55b72f3 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Tue, 31 May 2022 09:37:34 -0500 Subject: [PATCH 3/8] feat: Add migration to remove implied roles --- .../migrations/000016_drop_member_roles.down.sql | 11 +++++++++++ .../migrations/000016_drop_member_roles.up.sql | 11 +++++++++++ 2 files changed, 22 insertions(+) create mode 100644 coderd/database/migrations/000016_drop_member_roles.down.sql create mode 100644 coderd/database/migrations/000016_drop_member_roles.up.sql diff --git a/coderd/database/migrations/000016_drop_member_roles.down.sql b/coderd/database/migrations/000016_drop_member_roles.down.sql new file mode 100644 index 0000000000000..9268cd8db7eb8 --- /dev/null +++ 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 new file mode 100644 index 0000000000000..64b67c1a64d30 --- /dev/null +++ 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); From 28e3957acbf402d6941805637901b98af12be099 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Tue, 31 May 2022 09:48:30 -0500 Subject: [PATCH 4/8] Add comment to sql --- coderd/database/querier.go | 3 ++- coderd/database/queries.sql.go | 3 ++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/coderd/database/querier.go b/coderd/database/querier.go index bddb094c540d8..fc3274e94abc8 100644 --- a/coderd/database/querier.go +++ b/coderd/database/querier.go @@ -25,7 +25,8 @@ type querier interface { // GetAuditLogsBefore retrieves `limit` number of audit logs before the provided // ID. GetAuditLogsBefore(ctx context.Context, arg GetAuditLogsBeforeParams) ([]AuditLog, error) - // This function + // This function returns roles for authorization purposes. Implied member roles + // are included. GetAuthorizationUserRoles(ctx context.Context, userID uuid.UUID) (GetAuthorizationUserRolesRow, error) GetFileByHash(ctx context.Context, hash string) (File, error) GetGitSSHKey(ctx context.Context, userID uuid.UUID) (GitSSHKey, error) diff --git a/coderd/database/queries.sql.go b/coderd/database/queries.sql.go index 6d16c39454ed9..be5f9216eff2c 100644 --- a/coderd/database/queries.sql.go +++ b/coderd/database/queries.sql.go @@ -2115,7 +2115,8 @@ type GetAuthorizationUserRolesRow struct { Roles []string `db:"roles" json:"roles"` } -// This function +// This function returns roles for authorization purposes. Implied member roles +// are included. func (q *sqlQuerier) GetAuthorizationUserRoles(ctx context.Context, userID uuid.UUID) (GetAuthorizationUserRolesRow, error) { row := q.db.QueryRowContext(ctx, getAuthorizationUserRoles, userID) var i GetAuthorizationUserRolesRow From 058e7ab3ac4e042ac3158af357fcdeef94a817aa Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Tue, 31 May 2022 09:53:18 -0500 Subject: [PATCH 5/8] test: Update test --- coderd/roles_test.go | 25 ++++++++++++++----------- 1 file changed, 14 insertions(+), 11 deletions(-) diff --git a/coderd/roles_test.go b/coderd/roles_test.go index 4820b76567aa4..baa956863ab03 100644 --- a/coderd/roles_test.go +++ b/coderd/roles_test.go @@ -113,6 +113,8 @@ func TestListRoles(t *testing.T) { require.NoError(t, err, "create org") const forbidden = "forbidden" + siteRoles := convertRoles(rbac.RoleAdmin(), "auditor") + orgRoles := convertRoles(rbac.RoleOrgAdmin(admin.OrganizationID)) testCases := []struct { Name string @@ -127,14 +129,14 @@ func TestListRoles(t *testing.T) { x, err := member.ListSiteRoles(ctx) return x, err }, - ExpectedRoles: convertRoles(rbac.SiteRoles()), + ExpectedRoles: siteRoles, }, { Name: "OrgMemberListOrg", APICall: func() ([]codersdk.Role, error) { return member.ListOrganizationRoles(ctx, admin.OrganizationID) }, - ExpectedRoles: convertRoles(rbac.OrganizationRoles(admin.OrganizationID)), + ExpectedRoles: orgRoles, }, { Name: "NonOrgMemberListOrg", @@ -149,14 +151,14 @@ func TestListRoles(t *testing.T) { APICall: func() ([]codersdk.Role, error) { return orgAdmin.ListSiteRoles(ctx) }, - ExpectedRoles: convertRoles(rbac.SiteRoles()), + ExpectedRoles: siteRoles, }, { Name: "OrgAdminListOrg", APICall: func() ([]codersdk.Role, error) { return orgAdmin.ListOrganizationRoles(ctx, admin.OrganizationID) }, - ExpectedRoles: convertRoles(rbac.OrganizationRoles(admin.OrganizationID)), + ExpectedRoles: orgRoles, }, { Name: "OrgAdminListOtherOrg", @@ -171,14 +173,14 @@ func TestListRoles(t *testing.T) { APICall: func() ([]codersdk.Role, error) { return client.ListSiteRoles(ctx) }, - ExpectedRoles: convertRoles(rbac.SiteRoles()), + ExpectedRoles: siteRoles, }, { Name: "AdminListOrg", APICall: func() ([]codersdk.Role, error) { return client.ListOrganizationRoles(ctx, admin.OrganizationID) }, - ExpectedRoles: convertRoles(rbac.OrganizationRoles(admin.OrganizationID)), + ExpectedRoles: orgRoles, }, } @@ -200,17 +202,18 @@ func TestListRoles(t *testing.T) { } } -func convertRole(role rbac.Role) codersdk.Role { +func convertRole(roleName string) codersdk.Role { + role, _ := rbac.RoleByName(roleName) return codersdk.Role{ DisplayName: role.DisplayName, Name: role.Name, } } -func convertRoles(roles []rbac.Role) []codersdk.Role { - converted := make([]codersdk.Role, 0, len(roles)) - for _, role := range roles { - converted = append(converted, convertRole(role)) +func convertRoles(roleNames ...string) []codersdk.Role { + converted := make([]codersdk.Role, 0, len(roleNames)) + for _, roleName := range roleNames { + converted = append(converted, convertRole(roleName)) } return converted } From 9a3d38e2b9e548f2efbf9619eba120108b777a7b Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Tue, 31 May 2022 10:03:39 -0500 Subject: [PATCH 6/8] handle null string array --- coderd/organizations.go | 2 +- coderd/users.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/coderd/organizations.go b/coderd/organizations.go index ca5180badfae2..49dba0bb33324 100644 --- a/coderd/organizations.go +++ b/coderd/organizations.go @@ -77,7 +77,7 @@ func (api *API) postOrganizations(rw http.ResponseWriter, r *http.Request) { }, }) 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/users.go b/coderd/users.go index d1d5796528075..07a59546d908e 100644 --- a/coderd/users.go +++ b/coderd/users.go @@ -751,7 +751,7 @@ func (api *API) createAPIKey(rw http.ResponseWriter, r *http.Request, params dat func (api *API) createUser(ctx context.Context, req codersdk.CreateUserRequest) (database.User, uuid.UUID, error) { var user database.User return user, req.OrganizationID, api.Database.InTx(func(db database.Store) error { - var orgRoles []string + orgRoles := make([]string, 0) // If no organization is provided, create a new one for the user. if req.OrganizationID == uuid.Nil { organization, err := db.InsertOrganization(ctx, database.InsertOrganizationParams{ From 6a89023eaf4355fcca6c43f2f4b92f174f80d433 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Wed, 1 Jun 2022 08:25:23 -0500 Subject: [PATCH 7/8] rename user auth role middleware --- coderd/authorize.go | 4 ++-- coderd/httpmw/apikey.go | 4 ++-- coderd/httpmw/authorize_test.go | 2 +- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/coderd/authorize.go b/coderd/authorize.go index ed8f89506421b..64d6af755413c 100644 --- 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.UserAuthorizationRoles(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.UserAuthorizationRoles(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/httpmw/apikey.go b/coderd/httpmw/apikey.go index 1ee46bca3e09e..8c58606fd1027 100644 --- a/coderd/httpmw/apikey.go +++ b/coderd/httpmw/apikey.go @@ -34,9 +34,9 @@ func APIKey(r *http.Request) database.APIKey { // User roles are the 'subject' field of Authorize() type userRolesKey struct{} -// UserAuthorizationRoles returns the roles used for authorization. +// AuthorizationUserRoles returns the roles used for authorization. // Comes from the ExtractAPIKey handler. -func UserAuthorizationRoles(r *http.Request) database.GetAuthorizationUserRolesRow { +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") diff --git a/coderd/httpmw/authorize_test.go b/coderd/httpmw/authorize_test.go index 423d1203e1d85..7ea363249b7e6 100644 --- a/coderd/httpmw/authorize_test.go +++ b/coderd/httpmw/authorize_test.go @@ -86,7 +86,7 @@ func TestExtractUserRoles(t *testing.T) { httpmw.ExtractAPIKey(db, &httpmw.OAuth2Configs{}), ) rtr.Get("/", func(_ http.ResponseWriter, r *http.Request) { - roles := httpmw.UserAuthorizationRoles(r) + roles := httpmw.AuthorizationUserRoles(r) require.ElementsMatch(t, user.ID, roles.ID) require.ElementsMatch(t, expRoles, roles.Roles) }) From ccecddad95cf7682c6089637ea53fff4a733ea80 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Wed, 1 Jun 2022 08:28:05 -0500 Subject: [PATCH 8/8] fixup! rename user auth role middleware --- coderd/users.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/coderd/users.go b/coderd/users.go index 22ce1813cb64a..d4e5d55cef80f 100644 --- a/coderd/users.go +++ b/coderd/users.go @@ -473,7 +473,7 @@ func (api *API) userRoles(rw http.ResponseWriter, r *http.Request) { func (api *API) putUserRoles(rw http.ResponseWriter, r *http.Request) { // User is the user to modify. user := httpmw.UserParam(r) - roles := httpmw.UserAuthorizationRoles(r) + roles := httpmw.AuthorizationUserRoles(r) apiKey := httpmw.APIKey(r) if apiKey.UserID == user.ID {