From ed6fd1fa76a06a5424c21f93020aa7997b1ff88a Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Mon, 25 Apr 2022 13:14:12 -0500 Subject: [PATCH 1/9] WIP --- coderd/httpmw/rbac.go | 9 ++++++ coderd/rbac/builtin.go | 61 +++++++++++++++++++++++++++++++++++++ coderd/rbac/example_test.go | 11 +++++-- coderd/rbac/role.go | 7 +++-- 4 files changed, 83 insertions(+), 5 deletions(-) create mode 100644 coderd/httpmw/rbac.go create mode 100644 coderd/rbac/builtin.go diff --git a/coderd/httpmw/rbac.go b/coderd/httpmw/rbac.go new file mode 100644 index 0000000000000..63377cd4ca7fb --- /dev/null +++ b/coderd/httpmw/rbac.go @@ -0,0 +1,9 @@ +package httpmw + +//func Can(action rbac.Action, object rbac.Object) func(http.Handler) http.Handler { +// return func(next http.Handler) http.Handler { +// return http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) { +// +// }, +// } +//} diff --git a/coderd/rbac/builtin.go b/coderd/rbac/builtin.go new file mode 100644 index 0000000000000..c867004de807d --- /dev/null +++ b/coderd/rbac/builtin.go @@ -0,0 +1,61 @@ +package rbac + +import ( + "strings" + + "golang.org/x/xerrors" +) + +const ( + Admin = "admin" + Member = "member" + + OrganizationMember = "organization-member" + OrganizationAdmin = "organization-admin" +) + +// RoleByName returns the permissions associated with a given role name. +// This allows just the role names to be stored. +func RoleByName(name string) (Role, error) { + arr := strings.Split(name, ":") + if len(arr) > 2 { + return Role{}, xerrors.Errorf("too many semicolons in role name") + } + + roleName := arr[0] + var scopeID string + if len(arr) > 1 { + scopeID = arr[1] + } + + // If the role requires a scope, the scope will be checked at the end + // of the switch statement. + var scopedRole Role + switch roleName { + case Admin: + return RoleAdmin, nil + case Member: + return RoleMember, nil + case OrganizationMember: + scopedRole = RoleOrgMember(scopeID) + case OrganizationAdmin: + scopedRole = RoleOrgAdmin(scopeID) + default: + // No role found + return Role{}, xerrors.Errorf("role %q not found", roleName) + } + + // Scoped roles should be checked their scope is set + if scopeID == "" { + return Role{}, xerrors.Errorf("%q requires a scope id", roleName) + } + + return scopedRole, nil +} + +func RoleName(name string, scopeID string) string { + if scopeID == "" { + return name + } + return name + ":" + scopeID +} diff --git a/coderd/rbac/example_test.go b/coderd/rbac/example_test.go index e9c4222f9c4a9..5baf4d950360c 100644 --- a/coderd/rbac/example_test.go +++ b/coderd/rbac/example_test.go @@ -22,8 +22,8 @@ func TestExample(t *testing.T) { user := subject{ UserID: "alice", Roles: []rbac.Role{ - rbac.RoleOrgAdmin("default"), - rbac.RoleMember, + must(rbac.RoleByName(rbac.Member)), + must(rbac.RoleByName(rbac.RoleName(rbac.OrganizationMember, "default"))), }, } @@ -52,3 +52,10 @@ func TestExample(t *testing.T) { require.NoError(t, err, "this user can read workspace '1234'") }) } + +func must[T any](value T, err error) T { + if err != nil { + panic(err) + } + return value +} diff --git a/coderd/rbac/role.go b/coderd/rbac/role.go index 4788a189e6363..08023a54a289e 100644 --- a/coderd/rbac/role.go +++ b/coderd/rbac/role.go @@ -2,6 +2,7 @@ package rbac import "fmt" +// Permission is the format passed into the rego. type Permission struct { // Negate makes this a negative permission Negate bool `json:"negate"` @@ -57,7 +58,7 @@ var ( func RoleOrgDenyAll(orgID string) Role { return Role{ - Name: "org-deny-" + orgID, + Name: RoleName("org-deny", orgID), Org: map[string][]Permission{ orgID: { { @@ -75,7 +76,7 @@ func RoleOrgDenyAll(orgID string) Role { // organization scope. func RoleOrgAdmin(orgID string) Role { return Role{ - Name: "org-admin-" + orgID, + Name: RoleName("org-admin:", orgID) Org: map[string][]Permission{ orgID: { { @@ -93,7 +94,7 @@ func RoleOrgAdmin(orgID string) Role { // organization scope. func RoleOrgMember(orgID string) Role { return Role{ - Name: "org-member-" + orgID, + Name: RoleName("org-member:" , orgID), Org: map[string][]Permission{ orgID: {}, }, From 47c5bf227feb939f8892d0f0e0940a28b55e16ac Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Tue, 26 Apr 2022 21:12:00 -0500 Subject: [PATCH 2/9] chore: Rework roles to be expandable by name alone --- coderd/rbac/authz_test.go | 53 +++++++++--- coderd/rbac/builtin.go | 163 +++++++++++++++++++++++++++++++----- coderd/rbac/example_test.go | 4 +- coderd/rbac/role.go | 121 ++------------------------ 4 files changed, 190 insertions(+), 151 deletions(-) diff --git a/coderd/rbac/authz_test.go b/coderd/rbac/authz_test.go index c37b3d0f83a10..b4b8c18271480 100644 --- a/coderd/rbac/authz_test.go +++ b/coderd/rbac/authz_test.go @@ -3,6 +3,7 @@ package rbac_test import ( "context" "encoding/json" + "fmt" "testing" "golang.org/x/xerrors" @@ -14,8 +15,11 @@ import ( // subject is required because rego needs type subject struct { - UserID string `json:"id"` - Roles []rbac.Role `json:"roles"` + UserID string `json:"id"` + // For the unit test we want to pass in the roles directly, instead of just + // by name. This allows us to test custom roles that do not exist in the product, + // but test edge cases of the implementation. + Roles []rbac.Role `json:"roles"` } // TestAuthorizeDomain test the very basic roles that are commonly used. @@ -26,7 +30,10 @@ func TestAuthorizeDomain(t *testing.T) { user := subject{ UserID: "me", - Roles: []rbac.Role{rbac.RoleMember, rbac.RoleOrgMember(defOrg)}, + Roles: []rbac.Role{ + must(rbac.RoleByName(rbac.RoleMember())), + must(rbac.RoleByName(rbac.RoleOrgMember(defOrg))), + }, } testAuthorize(t, "Member", user, []authTestCase{ @@ -126,8 +133,8 @@ func TestAuthorizeDomain(t *testing.T) { user = subject{ UserID: "me", Roles: []rbac.Role{ - rbac.RoleOrgAdmin(defOrg), - rbac.RoleMember, + must(rbac.RoleByName(rbac.RoleOrgAdmin(defOrg))), + must(rbac.RoleByName(rbac.RoleMember())), }, } @@ -173,8 +180,8 @@ func TestAuthorizeDomain(t *testing.T) { user = subject{ UserID: "me", Roles: []rbac.Role{ - rbac.RoleAdmin, - rbac.RoleMember, + must(rbac.RoleByName(rbac.RoleAdmin())), + must(rbac.RoleByName(rbac.RoleMember())), }, } @@ -221,7 +228,19 @@ func TestAuthorizeDomain(t *testing.T) { user = subject{ UserID: "me", Roles: []rbac.Role{ - rbac.RoleWorkspaceAgent(wrkID), + { + Name: fmt.Sprintf("agent-%s", wrkID), + // This is at the site level to prevent the token from losing access if the user + // is kicked from the org + Site: []rbac.Permission{ + { + Negate: false, + ResourceType: rbac.ResourceWorkspace.Type, + ResourceID: wrkID, + Action: rbac.ActionRead, + }, + }, + }, }, } @@ -439,8 +458,20 @@ func TestAuthorizeLevels(t *testing.T) { user := subject{ UserID: "me", Roles: []rbac.Role{ - rbac.RoleAdmin, - rbac.RoleOrgDenyAll(defOrg), + must(rbac.RoleByName(rbac.RoleAdmin())), + { + Name: "org-deny" + defOrg, + Org: map[string][]rbac.Permission{ + defOrg: { + { + Negate: true, + ResourceType: "*", + ResourceID: "*", + Action: "*", + }, + }, + }, + }, { Name: "user-deny-all", // List out deny permissions explicitly @@ -514,7 +545,7 @@ func TestAuthorizeLevels(t *testing.T) { }, }, }, - rbac.RoleOrgAdmin(defOrg), + must(rbac.RoleByName(rbac.RoleOrgAdmin(defOrg))), { Name: "user-deny-all", // List out deny permissions explicitly diff --git a/coderd/rbac/builtin.go b/coderd/rbac/builtin.go index c867004de807d..abad54e03477f 100644 --- a/coderd/rbac/builtin.go +++ b/coderd/rbac/builtin.go @@ -7,15 +7,113 @@ import ( ) const ( - Admin = "admin" - Member = "member" + admin string = "admin" + member string = "member" + auditor string = "auditor" - OrganizationMember = "organization-member" - OrganizationAdmin = "organization-admin" + orgAdmin string = "organization-admin" + orgMember string = "organization-member" +) + +// RoleName is a string that represents a registered rbac role. We want to store +// strings in the database to allow the underlying role permissions to be migrated +// or modified. +// All RoleNames should have an entry in 'builtInRoles'. +// We use functions to retrieve the name incase we need to add a scope. +type RoleName = string + +func RoleAdmin() string { + return roleName(admin, "") +} + +func RoleMember() string { + return roleName(member, "") +} + +func RoleOrgAdmin(organizationID string) RoleName { + return roleName(orgAdmin, organizationID) +} + +func RoleOrgMember(organizationID string) RoleName { + return roleName(orgMember, organizationID) +} + +var ( + // builtInRoles are just a hard coded set for now. Ideally we store these in + // the database. Right now they are functions because the org id should scope + // certain roles. If we store them in the database, we will need to store + // them such that the "org" permissions are dynamically changed by the + // scopeID passed in. This isn't a hard problem to solve, it's just easier + // as a function right now. + builtInRoles = map[string]func(scopeID string) Role{ + // admin grants all actions to all resources. + admin: func(_ string) Role { + return Role{ + Name: admin, + Site: permissions(map[Object][]Action{ + ResourceWildcard: {WildcardSymbol}, + }), + } + }, + + // member grants all actions to all resources owned by the user + member: func(_ string) Role { + return Role{ + Name: member, + User: permissions(map[Object][]Action{ + ResourceWildcard: {WildcardSymbol}, + }), + } + }, + + // auditor provides all permissions required to effectively read and understand + // audit log events. + // TODO: Finish the auditor as we add resources. + auditor: func(_ string) Role { + return Role{ + Name: "auditor", + Site: permissions(map[Object][]Action{ + //ResourceAuditLogs: {ActionRead}, + // Should be able to read all template details, even in orgs they + // are not in. + ResourceTemplate: {ActionRead}, + }), + } + }, + + // orgAdmin returns a role with all actions allows in a given + // organization scope. + orgAdmin: func(organizationID string) Role { + return Role{ + Name: roleName(orgAdmin, organizationID), + Org: map[string][]Permission{ + organizationID: { + { + Negate: false, + ResourceType: "*", + ResourceID: "*", + Action: "*", + }, + }, + }, + } + }, + + // orgMember has an empty set of permissions, this just implies their membership + // in an organization. + orgMember: func(organizationID string) Role { + return Role{ + Name: roleName(orgMember, organizationID), + Org: map[string][]Permission{ + organizationID: {}, + }, + } + }, + } ) // RoleByName returns the permissions associated with a given role name. -// This allows just the role names to be stored. +// This allows just the role names to be stored and expanded when required. func RoleByName(name string) (Role, error) { arr := strings.Split(name, ":") if len(arr) > 2 { @@ -28,34 +126,53 @@ func RoleByName(name string) (Role, error) { scopeID = arr[1] } - // If the role requires a scope, the scope will be checked at the end - // of the switch statement. - var scopedRole Role - switch roleName { - case Admin: - return RoleAdmin, nil - case Member: - return RoleMember, nil - case OrganizationMember: - scopedRole = RoleOrgMember(scopeID) - case OrganizationAdmin: - scopedRole = RoleOrgAdmin(scopeID) - default: + roleFunc, ok := builtInRoles[roleName] + if !ok { // No role found return Role{}, xerrors.Errorf("role %q not found", roleName) } - // Scoped roles should be checked their scope is set - if scopeID == "" { - return Role{}, xerrors.Errorf("%q requires a scope id", roleName) + // Ensure all org roles are properly scoped a non-empty organization id. + // This is just some defensive programming. + role := roleFunc(scopeID) + if len(role.Org) > 0 && scopeID == "" { + return Role{}, xerrors.Errorf("expect a scope id for role %q", roleName) } - return scopedRole, nil + return role, nil } -func RoleName(name string, scopeID string) string { +// roleName is a quick helper function to return +// role_name:scopeID +// If no scopeID is required, only 'role_name' is returned +func roleName(name string, scopeID string) string { if scopeID == "" { return name } return name + ":" + scopeID } + +// permissions is just a helper function to make building roles that list out resources +// and actions a bit easier. +func permissions(perms map[Object][]Action) []Permission { + list := make([]Permission, 0, len(perms)) + for k, actions := range perms { + for _, act := range actions { + act := act + list = append(list, Permission{ + Negate: false, + ResourceType: k.Type, + ResourceID: WildcardSymbol, + Action: act, + }) + } + } + return list +} + +func must[T any](value T, err error) T { + if err != nil { + panic(err) + } + return value +} diff --git a/coderd/rbac/example_test.go b/coderd/rbac/example_test.go index 5baf4d950360c..8230b13cac943 100644 --- a/coderd/rbac/example_test.go +++ b/coderd/rbac/example_test.go @@ -22,8 +22,8 @@ func TestExample(t *testing.T) { user := subject{ UserID: "alice", Roles: []rbac.Role{ - must(rbac.RoleByName(rbac.Member)), - must(rbac.RoleByName(rbac.RoleName(rbac.OrganizationMember, "default"))), + must(rbac.RoleByName(rbac.RoleMember())), + must(rbac.RoleByName(rbac.RoleOrgAdmin("default"))), }, } diff --git a/coderd/rbac/role.go b/coderd/rbac/role.go index 08023a54a289e..362d11bf952b9 100644 --- a/coderd/rbac/role.go +++ b/coderd/rbac/role.go @@ -1,7 +1,5 @@ package rbac -import "fmt" - // Permission is the format passed into the rego. type Permission struct { // Negate makes this a negative permission @@ -15,122 +13,15 @@ type Permission struct { // - Site level permissions apply EVERYWHERE // - Org level permissions apply to EVERYTHING in a given ORG // - User level permissions are the lowest -// In most cases, you will just want to use the pre-defined roles -// below. +// This is the type passed into the rego as a json payload. +// 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 RoleName `json:"name"` Site []Permission `json:"site"` // Org is a map of orgid to permissions. We represent orgid as a string. + // We scope the organizations in the role so we can easily combine all the + // roles. Org map[string][]Permission `json:"org"` User []Permission `json:"user"` } - -// Roles are stored as structs, so they can be serialized and stored. Until we store them elsewhere, -// const's will do just fine. -var ( - // RoleAdmin is a role that allows everything everywhere. - RoleAdmin = Role{ - Name: "admin", - Site: permissions(map[Object][]Action{ - ResourceWildcard: {WildcardSymbol}, - }), - } - - // RoleMember is a role that allows access to user-level resources. - RoleMember = Role{ - Name: "member", - User: permissions(map[Object][]Action{ - ResourceWildcard: {WildcardSymbol}, - }), - } - - // RoleAuditor is an example on how to give more precise permissions - RoleAuditor = Role{ - Name: "auditor", - Site: permissions(map[Object][]Action{ - //ResourceAuditLogs: {ActionRead}, - // Should be able to read user details to associate with logs. - // Without this the user-id in logs is not very helpful - ResourceWorkspace: {ActionRead}, - }), - } -) - -func RoleOrgDenyAll(orgID string) Role { - return Role{ - Name: RoleName("org-deny", orgID), - Org: map[string][]Permission{ - orgID: { - { - Negate: true, - ResourceType: "*", - ResourceID: "*", - Action: "*", - }, - }, - }, - } -} - -// RoleOrgAdmin returns a role with all actions allows in a given -// organization scope. -func RoleOrgAdmin(orgID string) Role { - return Role{ - Name: RoleName("org-admin:", orgID) - Org: map[string][]Permission{ - orgID: { - { - Negate: false, - ResourceType: "*", - ResourceID: "*", - Action: "*", - }, - }, - }, - } -} - -// RoleOrgMember returns a role with default permissions in a given -// organization scope. -func RoleOrgMember(orgID string) Role { - return Role{ - Name: RoleName("org-member:" , orgID), - Org: map[string][]Permission{ - orgID: {}, - }, - } -} - -// RoleWorkspaceAgent returns a role with permission to read a given -// workspace. -func RoleWorkspaceAgent(workspaceID string) Role { - return Role{ - Name: fmt.Sprintf("agent-%s", workspaceID), - // This is at the site level to prevent the token from losing access if the user - // is kicked from the org - Site: []Permission{ - { - Negate: false, - ResourceType: ResourceWorkspace.Type, - ResourceID: workspaceID, - Action: ActionRead, - }, - }, - } -} - -func permissions(perms map[Object][]Action) []Permission { - list := make([]Permission, 0, len(perms)) - for k, actions := range perms { - for _, act := range actions { - act := act - list = append(list, Permission{ - Negate: false, - ResourceType: k.Type, - ResourceID: WildcardSymbol, - Action: act, - }) - } - } - return list -} From 691d9a5ce0acbc7c097e8a36b367de322835e79c Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Tue, 26 Apr 2022 21:30:42 -0500 Subject: [PATCH 3/9] Add role migrations --- .../migrations/000007_rbac_user_roles.down.sql | 2 ++ .../migrations/000007_rbac_user_roles.up.sql | 18 ++++++++++++++++++ 2 files changed, 20 insertions(+) create mode 100644 coderd/database/migrations/000007_rbac_user_roles.down.sql create mode 100644 coderd/database/migrations/000007_rbac_user_roles.up.sql diff --git a/coderd/database/migrations/000007_rbac_user_roles.down.sql b/coderd/database/migrations/000007_rbac_user_roles.down.sql new file mode 100644 index 0000000000000..327561894eab6 --- /dev/null +++ b/coderd/database/migrations/000007_rbac_user_roles.down.sql @@ -0,0 +1,2 @@ +ALTER TABLE ONLY workspaces + DROP COLUMN IF EXISTS rbac_roles; \ No newline at end of file diff --git a/coderd/database/migrations/000007_rbac_user_roles.up.sql b/coderd/database/migrations/000007_rbac_user_roles.up.sql new file mode 100644 index 0000000000000..adc7d72b24c20 --- /dev/null +++ b/coderd/database/migrations/000007_rbac_user_roles.up.sql @@ -0,0 +1,18 @@ +ALTER TABLE ONLY users + ADD COLUMN IF NOT EXISTS rbac_roles text[] DEFAULT '{}' NOT NULL; + +-- All users are site members. So give them the standard role. +-- Also give them membership to the first org we retrieve. We should only have +-- 1 organization at this point in the product. +UPDATE + users +SET + rbac_roles = ARRAY ['member', 'organization-member:' || (SELECT id FROM organizations LIMIT 1)]; + +-- Give the first user created the admin role +UPDATE + users +SET + rbac_roles = rbac_roles || ARRAY ['admin'] +WHERE + id = (SELECT id FROM users ORDER BY created_at ASC LIMIT 1) From d2ca5c91459a9836cfb5a5f66500cb1ca729b81b Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Tue, 26 Apr 2022 21:32:19 -0500 Subject: [PATCH 4/9] Make gen --- coderd/database/dump.sql | 3 ++- coderd/database/models.go | 1 + coderd/database/queries.sql.go | 15 ++++++++++----- peerbroker/proto/peerbroker_drpc.pb.go | 2 +- provisionerd/proto/provisionerd_drpc.pb.go | 2 +- provisionersdk/proto/provisioner_drpc.pb.go | 2 +- 6 files changed, 16 insertions(+), 9 deletions(-) diff --git a/coderd/database/dump.sql b/coderd/database/dump.sql index fbc137f1223e0..2082673b2a050 100644 --- a/coderd/database/dump.sql +++ b/coderd/database/dump.sql @@ -221,7 +221,8 @@ CREATE TABLE users ( username text DEFAULT ''::text NOT NULL, hashed_password bytea NOT NULL, created_at timestamp with time zone NOT NULL, - updated_at timestamp with time zone NOT NULL + updated_at timestamp with time zone NOT NULL, + rbac_roles text[] DEFAULT '{}'::text[] NOT NULL ); CREATE TABLE workspace_agents ( diff --git a/coderd/database/models.go b/coderd/database/models.go index 56dc8fd77a58c..78fd687a99778 100644 --- a/coderd/database/models.go +++ b/coderd/database/models.go @@ -378,6 +378,7 @@ type User struct { HashedPassword []byte `db:"hashed_password" json:"hashed_password"` CreatedAt time.Time `db:"created_at" json:"created_at"` UpdatedAt time.Time `db:"updated_at" json:"updated_at"` + RbacRoles []string `db:"rbac_roles" json:"rbac_roles"` } type Workspace struct { diff --git a/coderd/database/queries.sql.go b/coderd/database/queries.sql.go index 3f76b362f4bb2..8feca67b6be4f 100644 --- a/coderd/database/queries.sql.go +++ b/coderd/database/queries.sql.go @@ -1782,7 +1782,7 @@ func (q *sqlQuerier) UpdateTemplateVersionByID(ctx context.Context, arg UpdateTe const getUserByEmailOrUsername = `-- name: GetUserByEmailOrUsername :one SELECT - id, email, username, hashed_password, created_at, updated_at + id, email, username, hashed_password, created_at, updated_at, rbac_roles FROM users WHERE @@ -1807,13 +1807,14 @@ func (q *sqlQuerier) GetUserByEmailOrUsername(ctx context.Context, arg GetUserBy &i.HashedPassword, &i.CreatedAt, &i.UpdatedAt, + pq.Array(&i.RbacRoles), ) return i, err } const getUserByID = `-- name: GetUserByID :one SELECT - id, email, username, hashed_password, created_at, updated_at + id, email, username, hashed_password, created_at, updated_at, rbac_roles FROM users WHERE @@ -1832,6 +1833,7 @@ func (q *sqlQuerier) GetUserByID(ctx context.Context, id uuid.UUID) (User, error &i.HashedPassword, &i.CreatedAt, &i.UpdatedAt, + pq.Array(&i.RbacRoles), ) return i, err } @@ -1852,7 +1854,7 @@ func (q *sqlQuerier) GetUserCount(ctx context.Context) (int64, error) { const getUsers = `-- name: GetUsers :many SELECT - id, email, username, hashed_password, created_at, updated_at + id, email, username, hashed_password, created_at, updated_at, rbac_roles FROM users WHERE @@ -1922,6 +1924,7 @@ func (q *sqlQuerier) GetUsers(ctx context.Context, arg GetUsersParams) ([]User, &i.HashedPassword, &i.CreatedAt, &i.UpdatedAt, + pq.Array(&i.RbacRoles), ); err != nil { return nil, err } @@ -1947,7 +1950,7 @@ INSERT INTO updated_at ) VALUES - ($1, $2, $3, $4, $5, $6) RETURNING id, email, username, hashed_password, created_at, updated_at + ($1, $2, $3, $4, $5, $6) RETURNING id, email, username, hashed_password, created_at, updated_at, rbac_roles ` type InsertUserParams struct { @@ -1976,6 +1979,7 @@ func (q *sqlQuerier) InsertUser(ctx context.Context, arg InsertUserParams) (User &i.HashedPassword, &i.CreatedAt, &i.UpdatedAt, + pq.Array(&i.RbacRoles), ) return i, err } @@ -1988,7 +1992,7 @@ SET username = $3, updated_at = $4 WHERE - id = $1 RETURNING id, email, username, hashed_password, created_at, updated_at + id = $1 RETURNING id, email, username, hashed_password, created_at, updated_at, rbac_roles ` type UpdateUserProfileParams struct { @@ -2013,6 +2017,7 @@ func (q *sqlQuerier) UpdateUserProfile(ctx context.Context, arg UpdateUserProfil &i.HashedPassword, &i.CreatedAt, &i.UpdatedAt, + pq.Array(&i.RbacRoles), ) return i, err } diff --git a/peerbroker/proto/peerbroker_drpc.pb.go b/peerbroker/proto/peerbroker_drpc.pb.go index ae06f79a01371..a31d184cbd639 100644 --- a/peerbroker/proto/peerbroker_drpc.pb.go +++ b/peerbroker/proto/peerbroker_drpc.pb.go @@ -1,5 +1,5 @@ // Code generated by protoc-gen-go-drpc. DO NOT EDIT. -// protoc-gen-go-drpc version: v0.0.26 +// protoc-gen-go-drpc version: v0.0.30 // source: peerbroker/proto/peerbroker.proto package proto diff --git a/provisionerd/proto/provisionerd_drpc.pb.go b/provisionerd/proto/provisionerd_drpc.pb.go index 646f855eabc70..c583c320c90e2 100644 --- a/provisionerd/proto/provisionerd_drpc.pb.go +++ b/provisionerd/proto/provisionerd_drpc.pb.go @@ -1,5 +1,5 @@ // Code generated by protoc-gen-go-drpc. DO NOT EDIT. -// protoc-gen-go-drpc version: v0.0.26 +// protoc-gen-go-drpc version: v0.0.30 // source: provisionerd/proto/provisionerd.proto package proto diff --git a/provisionersdk/proto/provisioner_drpc.pb.go b/provisionersdk/proto/provisioner_drpc.pb.go index c990f6f645b7f..0dba37415265b 100644 --- a/provisionersdk/proto/provisioner_drpc.pb.go +++ b/provisionersdk/proto/provisioner_drpc.pb.go @@ -1,5 +1,5 @@ // Code generated by protoc-gen-go-drpc. DO NOT EDIT. -// protoc-gen-go-drpc version: v0.0.26 +// protoc-gen-go-drpc version: v0.0.30 // source: provisionersdk/proto/provisioner.proto package proto From be5d0452cfdf7bc706db22985ffe2a01d45a338b Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Tue, 26 Apr 2022 21:46:52 -0500 Subject: [PATCH 5/9] Add admin role to the first user --- coderd/database/querier.go | 1 + coderd/database/queries.sql.go | 38 +++++++++++++++++++++++++++++-- coderd/database/queries/users.sql | 15 ++++++++++-- coderd/rbac/builtin.go | 9 ++++---- coderd/users.go | 22 +++++++++++++++++- 5 files changed, 76 insertions(+), 9 deletions(-) diff --git a/coderd/database/querier.go b/coderd/database/querier.go index 3b8f317b620b3..00b7013bbfbd5 100644 --- a/coderd/database/querier.go +++ b/coderd/database/querier.go @@ -57,6 +57,7 @@ type querier interface { GetWorkspaceResourcesByJobID(ctx context.Context, jobID uuid.UUID) ([]WorkspaceResource, error) GetWorkspacesByTemplateID(ctx context.Context, arg GetWorkspacesByTemplateIDParams) ([]Workspace, error) GetWorkspacesByUserID(ctx context.Context, arg GetWorkspacesByUserIDParams) ([]Workspace, error) + GrantUserRole(ctx context.Context, arg GrantUserRoleParams) (User, error) InsertAPIKey(ctx context.Context, arg InsertAPIKeyParams) (APIKey, error) InsertFile(ctx context.Context, arg InsertFileParams) (File, error) InsertGitSSHKey(ctx context.Context, arg InsertGitSSHKeyParams) (GitSSHKey, error) diff --git a/coderd/database/queries.sql.go b/coderd/database/queries.sql.go index 8feca67b6be4f..904358cef2e5f 100644 --- a/coderd/database/queries.sql.go +++ b/coderd/database/queries.sql.go @@ -1939,6 +1939,37 @@ func (q *sqlQuerier) GetUsers(ctx context.Context, arg GetUsersParams) ([]User, return items, nil } +const grantUserRole = `-- name: GrantUserRole :one +UPDATE + users +SET + -- Append new roles and remove duplicates just to keep things clean. + rbac_roles = ARRAY(SELECT DISTINCT UNNEST(rbac_roles || $1 :: text[])) +WHERE + id = $2 +RETURNING id, email, username, hashed_password, created_at, updated_at, rbac_roles +` + +type GrantUserRoleParams struct { + GrantedRoles []string `db:"granted_roles" json:"granted_roles"` + ID uuid.UUID `db:"id" json:"id"` +} + +func (q *sqlQuerier) GrantUserRole(ctx context.Context, arg GrantUserRoleParams) (User, error) { + row := q.db.QueryRowContext(ctx, grantUserRole, pq.Array(arg.GrantedRoles), arg.ID) + var i User + err := row.Scan( + &i.ID, + &i.Email, + &i.Username, + &i.HashedPassword, + &i.CreatedAt, + &i.UpdatedAt, + pq.Array(&i.RbacRoles), + ) + return i, err +} + const insertUser = `-- name: InsertUser :one INSERT INTO users ( @@ -1947,10 +1978,11 @@ INSERT INTO username, hashed_password, created_at, - updated_at + updated_at, + rbac_roles ) VALUES - ($1, $2, $3, $4, $5, $6) RETURNING id, email, username, hashed_password, created_at, updated_at, rbac_roles + ($1, $2, $3, $4, $5, $6, $7) RETURNING id, email, username, hashed_password, created_at, updated_at, rbac_roles ` type InsertUserParams struct { @@ -1960,6 +1992,7 @@ type InsertUserParams struct { HashedPassword []byte `db:"hashed_password" json:"hashed_password"` CreatedAt time.Time `db:"created_at" json:"created_at"` UpdatedAt time.Time `db:"updated_at" json:"updated_at"` + RbacRoles []string `db:"rbac_roles" json:"rbac_roles"` } func (q *sqlQuerier) InsertUser(ctx context.Context, arg InsertUserParams) (User, error) { @@ -1970,6 +2003,7 @@ func (q *sqlQuerier) InsertUser(ctx context.Context, arg InsertUserParams) (User arg.HashedPassword, arg.CreatedAt, arg.UpdatedAt, + pq.Array(arg.RbacRoles), ) var i User err := row.Scan( diff --git a/coderd/database/queries/users.sql b/coderd/database/queries/users.sql index c8cc39b42ff1f..04f05b2bfd9e3 100644 --- a/coderd/database/queries/users.sql +++ b/coderd/database/queries/users.sql @@ -33,10 +33,11 @@ INSERT INTO username, hashed_password, created_at, - updated_at + updated_at, + rbac_roles ) VALUES - ($1, $2, $3, $4, $5, $6) RETURNING *; + ($1, $2, $3, $4, $5, $6, $7) RETURNING *; -- name: UpdateUserProfile :one UPDATE @@ -48,6 +49,16 @@ SET WHERE id = $1 RETURNING *; +-- name: GrantUserRole :one +UPDATE + users +SET + -- Append new roles and remove duplicates just to keep things clean. + rbac_roles = ARRAY(SELECT DISTINCT UNNEST(rbac_roles || @granted_roles :: text[])) +WHERE + id = @id +RETURNING *; + -- name: GetUsers :many SELECT * diff --git a/coderd/rbac/builtin.go b/coderd/rbac/builtin.go index abad54e03477f..9b253a7ef3e3f 100644 --- a/coderd/rbac/builtin.go +++ b/coderd/rbac/builtin.go @@ -1,6 +1,7 @@ package rbac import ( + "github.com/google/uuid" "strings" "golang.org/x/xerrors" @@ -30,12 +31,12 @@ func RoleMember() string { return roleName(member, "") } -func RoleOrgAdmin(organizationID string) RoleName { - return roleName(orgAdmin, organizationID) +func RoleOrgAdmin(organizationID uuid.UUID) RoleName { + return roleName(orgAdmin, organizationID.String()) } -func RoleOrgMember(organizationID string) RoleName { - return roleName(orgMember, organizationID) +func RoleOrgMember(organizationID uuid.UUID) RoleName { + return roleName(orgMember, organizationID.String()) } var ( diff --git a/coderd/users.go b/coderd/users.go index 579fcffd7e862..20ffdf01f37d0 100644 --- a/coderd/users.go +++ b/coderd/users.go @@ -11,6 +11,8 @@ import ( "strconv" "time" + "github.com/coder/coder/coderd/rbac" + "github.com/go-chi/chi/v5" "github.com/go-chi/render" "github.com/google/uuid" @@ -84,6 +86,21 @@ func (api *api) postFirstUser(rw http.ResponseWriter, r *http.Request) { return } + // TODO: @emyrk this currently happens outside the database tx used to create + // the user. Maybe I add this ability to grant roles in the createUser api + // and add some rbac bypass when calling api functions this way?? + // Add the admin role to this first user + _, err = api.Database.GrantUserRole(r.Context(), database.GrantUserRoleParams{ + GrantedRoles: []string{rbac.RoleAdmin()}, + ID: user.ID, + }) + if err != nil { + httpapi.Write(rw, http.StatusInternalServerError, httpapi.Response{ + Message: err.Error(), + }) + return + } + httpapi.Write(rw, http.StatusCreated, codersdk.CreateFirstUserResponse{ UserID: user.ID, OrganizationID: organizationID, @@ -892,6 +909,8 @@ func (api *api) createUser(ctx context.Context, req codersdk.CreateUserRequest) Username: req.Username, CreatedAt: database.Now(), UpdatedAt: database.Now(), + // All new users are defaulted to members of the site. + RbacRoles: []string{rbac.RoleMember()}, } // If a user signs up with OAuth, they can have no password! if req.Password != "" { @@ -927,7 +946,8 @@ func (api *api) createUser(ctx context.Context, req codersdk.CreateUserRequest) UserID: user.ID, CreatedAt: database.Now(), UpdatedAt: database.Now(), - Roles: []string{}, + // By default give them membership to the organization + Roles: []string{rbac.RoleOrgMember(req.OrganizationID)}, }) if err != nil { return xerrors.Errorf("create organization member: %w", err) From baf4843ca2f9b7f81373ae402e39e02b74dd37a7 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Wed, 27 Apr 2022 10:08:47 -0500 Subject: [PATCH 6/9] Implement builtin roles --- coderd/rbac/authz_test.go | 4 +- coderd/rbac/builtin.go | 90 +++++++++++++++++++++++++++++++++------ 2 files changed, 80 insertions(+), 14 deletions(-) diff --git a/coderd/rbac/authz_test.go b/coderd/rbac/authz_test.go index b4b8c18271480..69dfdf890b032 100644 --- a/coderd/rbac/authz_test.go +++ b/coderd/rbac/authz_test.go @@ -6,6 +6,8 @@ import ( "fmt" "testing" + "github.com/google/uuid" + "golang.org/x/xerrors" "github.com/stretchr/testify/require" @@ -25,7 +27,7 @@ type subject struct { // TestAuthorizeDomain test the very basic roles that are commonly used. func TestAuthorizeDomain(t *testing.T) { t.Parallel() - defOrg := "default" + defOrg := uuid.New() wrkID := "1234" user := subject{ diff --git a/coderd/rbac/builtin.go b/coderd/rbac/builtin.go index 9b253a7ef3e3f..cb39b303a6c74 100644 --- a/coderd/rbac/builtin.go +++ b/coderd/rbac/builtin.go @@ -12,8 +12,9 @@ const ( member string = "member" auditor string = "auditor" - orgAdmin string = "organization-admin" - orgMember string = "organization-member" + orgAdmin string = "organization-admin" + orgMember string = "organization-member" + orgManager string = "organization-manager" ) // RoleName is a string that represents a registered rbac role. We want to store @@ -23,11 +24,16 @@ const ( // We use functions to retrieve the name incase we need to add a scope. type RoleName = string -func RoleAdmin() string { +// The functions below ONLY need to exist for roles that are "defaulted" in some way. +// Any other roles (like auditor), can be listed and let the user select/assigned. +// Once we have a database implementation, the "default" roles be be defined on the +// site and orgs, and these functions can be removed. + +func RoleAdmin() RoleName { return roleName(admin, "") } -func RoleMember() string { +func RoleMember() RoleName { return roleName(member, "") } @@ -35,6 +41,7 @@ func RoleOrgAdmin(organizationID uuid.UUID) RoleName { return roleName(orgAdmin, organizationID.String()) } +// member:uuid func RoleOrgMember(organizationID uuid.UUID) RoleName { return roleName(orgMember, organizationID.String()) } @@ -46,6 +53,9 @@ var ( // them such that the "org" permissions are dynamically changed by the // scopeID passed in. This isn't a hard problem to solve, it's just easier // as a function right now. + // + // This map will be replaced by database storage defined by this ticket. + // https://github.com/coder/coder/issues/1194 builtInRoles = map[string]func(scopeID string) Role{ // admin grants all actions to all resources. admin: func(_ string) Role { @@ -110,21 +120,63 @@ var ( }, } }, + + orgManager: func(organizationID string) Role { + return Role{ + Name: roleName(orgMember, organizationID), + Org: map[string][]Permission{ + organizationID: permissions(map[Object][]Action{ + ResourceWorkspace: {WildcardSymbol}, + }), + }, + } + }, } ) +// ListOrgRoles lists all roles that can be applied to an organization user +// in the given organization. +// Note: This should be a list in a database, but until then we build +// the list from the builtins. +func ListOrgRoles(organizationID uuid.UUID) []string { + var roles []string + for role, _ := range builtInRoles { + _, scope, err := roleSplit(role) + if err != nil { + // This should never happen + continue + } + if scope == organizationID.String() { + roles = append(roles, role) + } + } + return roles +} + +// ListSiteRoles lists all roles that can be applied to a user. +// Note: This should be a list in a database, but until then we build +// the list from the builtins. +func ListSiteRoles() []string { + var roles []string + for role, _ := range builtInRoles { + _, scope, err := roleSplit(role) + if err != nil { + // This should never happen + continue + } + if scope == "" { + roles = append(roles, role) + } + } + return roles +} + // RoleByName returns the permissions associated with a given role name. // This allows just the role names to be stored and expanded when required. func RoleByName(name string) (Role, error) { - arr := strings.Split(name, ":") - if len(arr) > 2 { - return Role{}, xerrors.Errorf("too many semicolons in role name") - } - - roleName := arr[0] - var scopeID string - if len(arr) > 1 { - scopeID = arr[1] + roleName, scopeID, err := roleSplit(name) + if err != nil { + return Role{}, xerrors.Errorf(":%w", err) } roleFunc, ok := builtInRoles[roleName] @@ -153,6 +205,18 @@ func roleName(name string, scopeID string) string { return name + ":" + scopeID } +func roleSplit(role string) (name string, scopeID string, err error) { + arr := strings.Split(name, ":") + if len(arr) > 2 { + return "", "", xerrors.Errorf("too many colons in role name") + } + + if len(arr) == 2 { + return arr[0], arr[1], nil + } + return arr[0], "", nil +} + // permissions is just a helper function to make building roles that list out resources // and actions a bit easier. func permissions(perms map[Object][]Action) []Permission { From 8b9e3bc4ec56c0f870223b1c6aa53542d5ea9c6d Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Wed, 27 Apr 2022 14:53:11 -0500 Subject: [PATCH 7/9] Add fake for GrantRoles --- coderd/database/databasefake/databasefake.go | 30 ++++ coderd/rbac/authz.go | 17 ++ coderd/rbac/authz_test.go | 170 ++++++++++--------- coderd/rbac/builtin.go | 6 +- coderd/rbac/example_test.go | 10 +- coderd/rbac/object.go | 8 +- 6 files changed, 150 insertions(+), 91 deletions(-) diff --git a/coderd/database/databasefake/databasefake.go b/coderd/database/databasefake/databasefake.go index acf9f0913f3f1..5b75afd31c261 100644 --- a/coderd/database/databasefake/databasefake.go +++ b/coderd/database/databasefake/databasefake.go @@ -1123,6 +1123,36 @@ func (q *fakeQuerier) InsertUser(_ context.Context, arg database.InsertUserParam return user, nil } +func (q *fakeQuerier) GrantUserRole(ctx context.Context, arg database.GrantUserRoleParams) (database.User, error) { + q.mutex.Lock() + defer q.mutex.Unlock() + + for index, user := range q.users { + if user.ID != arg.ID { + continue + } + + // Append the new roles + user.RbacRoles = append(user.RbacRoles, arg.GrantedRoles...) + // Remove duplicates and sort + uniqueRoles := make([]string, 0, len(user.RbacRoles)) + exist := make(map[string]struct{}) + for _, r := range user.RbacRoles { + if _, ok := exist[r]; ok { + continue + } + exist[r] = struct{}{} + uniqueRoles = append(uniqueRoles, r) + } + sort.Strings(uniqueRoles) + user.RbacRoles = uniqueRoles + + q.users[index] = user + return user, nil + } + return database.User{}, sql.ErrNoRows +} + func (q *fakeQuerier) UpdateUserProfile(_ context.Context, arg database.UpdateUserProfileParams) (database.User, error) { q.mutex.Lock() defer q.mutex.Unlock() diff --git a/coderd/rbac/authz.go b/coderd/rbac/authz.go index f1ae3335cc4d8..6c71f4f3d1ba2 100644 --- a/coderd/rbac/authz.go +++ b/coderd/rbac/authz.go @@ -38,6 +38,23 @@ type authSubject struct { Roles []Role `json:"roles"` } +// AuthorizeByRoleName will expand all roleNames into roles before calling Authorize(). +// This is the function intended to be used outside this package. +// The role is fetched from the builtin map located in memory. +func (a RegoAuthorizer) AuthorizeByRoleName(ctx context.Context, subjectID string, roleNames []RoleName, action Action, object Object) error { + roles := make([]Role, 0, len(roleNames)) + for _, n := range roleNames { + r, err := RoleByName(n) + if err != nil { + return xerrors.Errorf("get role permissions: %w", err) + } + roles = append(roles, r) + } + return a.Authorize(ctx, subjectID, roles, action, object) +} + +// Authorize allows passing in custom Roles. +// This is really helpful for unit testing, as we can create custom roles to exercise edge cases. func (a RegoAuthorizer) Authorize(ctx context.Context, subjectID string, roles []Role, action Action, object Object) error { input := map[string]interface{}{ "subject": authSubject{ diff --git a/coderd/rbac/authz_test.go b/coderd/rbac/authz_test.go index 69dfdf890b032..3e797b238e982 100644 --- a/coderd/rbac/authz_test.go +++ b/coderd/rbac/authz_test.go @@ -28,6 +28,7 @@ type subject struct { func TestAuthorizeDomain(t *testing.T) { t.Parallel() defOrg := uuid.New() + unuseID := uuid.New() wrkID := "1234" user := subject{ @@ -53,10 +54,10 @@ func TestAuthorizeDomain(t *testing.T) { {resource: rbac.ResourceWorkspace.All(), actions: allActions(), allow: false}, // Other org + me + id - {resource: rbac.ResourceWorkspace.InOrg("other").WithOwner(user.UserID).WithID(wrkID), actions: allActions(), allow: false}, - {resource: rbac.ResourceWorkspace.InOrg("other").WithOwner(user.UserID), actions: allActions(), allow: false}, - {resource: rbac.ResourceWorkspace.InOrg("other").WithID(wrkID), actions: allActions(), allow: false}, - {resource: rbac.ResourceWorkspace.InOrg("other"), actions: allActions(), allow: false}, + {resource: rbac.ResourceWorkspace.InOrg(unuseID).WithOwner(user.UserID).WithID(wrkID), actions: allActions(), allow: false}, + {resource: rbac.ResourceWorkspace.InOrg(unuseID).WithOwner(user.UserID), actions: allActions(), allow: false}, + {resource: rbac.ResourceWorkspace.InOrg(unuseID).WithID(wrkID), actions: allActions(), allow: false}, + {resource: rbac.ResourceWorkspace.InOrg(unuseID), actions: allActions(), allow: false}, // Other org + other user + id {resource: rbac.ResourceWorkspace.InOrg(defOrg).WithOwner("not-me").WithID(wrkID), actions: allActions(), allow: false}, @@ -66,10 +67,10 @@ func TestAuthorizeDomain(t *testing.T) { {resource: rbac.ResourceWorkspace.WithOwner("not-me"), actions: allActions(), allow: false}, // Other org + other use + other id - {resource: rbac.ResourceWorkspace.InOrg("other").WithOwner("not-me").WithID("not-id"), actions: allActions(), allow: false}, - {resource: rbac.ResourceWorkspace.InOrg("other").WithOwner("not-me"), actions: allActions(), allow: false}, - {resource: rbac.ResourceWorkspace.InOrg("other").WithID("not-id"), actions: allActions(), allow: false}, - {resource: rbac.ResourceWorkspace.InOrg("other"), actions: allActions(), allow: false}, + {resource: rbac.ResourceWorkspace.InOrg(unuseID).WithOwner("not-me").WithID("not-id"), actions: allActions(), allow: false}, + {resource: rbac.ResourceWorkspace.InOrg(unuseID).WithOwner("not-me"), actions: allActions(), allow: false}, + {resource: rbac.ResourceWorkspace.InOrg(unuseID).WithID("not-id"), actions: allActions(), allow: false}, + {resource: rbac.ResourceWorkspace.InOrg(unuseID), actions: allActions(), allow: false}, {resource: rbac.ResourceWorkspace.WithOwner("not-me").WithID("not-id"), actions: allActions(), allow: false}, {resource: rbac.ResourceWorkspace.WithOwner("not-me"), actions: allActions(), allow: false}, @@ -108,10 +109,10 @@ func TestAuthorizeDomain(t *testing.T) { {resource: rbac.ResourceWorkspace.All(), actions: allActions(), allow: false}, // Other org + me + id - {resource: rbac.ResourceWorkspace.InOrg("other").WithOwner(user.UserID).WithID(wrkID), actions: allActions(), allow: false}, - {resource: rbac.ResourceWorkspace.InOrg("other").WithOwner(user.UserID), actions: allActions(), allow: false}, - {resource: rbac.ResourceWorkspace.InOrg("other").WithID(wrkID), actions: allActions(), allow: false}, - {resource: rbac.ResourceWorkspace.InOrg("other"), actions: allActions(), allow: false}, + {resource: rbac.ResourceWorkspace.InOrg(unuseID).WithOwner(user.UserID).WithID(wrkID), actions: allActions(), allow: false}, + {resource: rbac.ResourceWorkspace.InOrg(unuseID).WithOwner(user.UserID), actions: allActions(), allow: false}, + {resource: rbac.ResourceWorkspace.InOrg(unuseID).WithID(wrkID), actions: allActions(), allow: false}, + {resource: rbac.ResourceWorkspace.InOrg(unuseID), actions: allActions(), allow: false}, // Other org + other user + id {resource: rbac.ResourceWorkspace.InOrg(defOrg).WithOwner("not-me").WithID(wrkID), actions: allActions(), allow: false}, @@ -121,10 +122,10 @@ func TestAuthorizeDomain(t *testing.T) { {resource: rbac.ResourceWorkspace.WithOwner("not-me"), actions: allActions(), allow: false}, // Other org + other use + other id - {resource: rbac.ResourceWorkspace.InOrg("other").WithOwner("not-me").WithID("not-id"), actions: allActions(), allow: false}, - {resource: rbac.ResourceWorkspace.InOrg("other").WithOwner("not-me"), actions: allActions(), allow: false}, - {resource: rbac.ResourceWorkspace.InOrg("other").WithID("not-id"), actions: allActions(), allow: false}, - {resource: rbac.ResourceWorkspace.InOrg("other"), actions: allActions(), allow: false}, + {resource: rbac.ResourceWorkspace.InOrg(unuseID).WithOwner("not-me").WithID("not-id"), actions: allActions(), allow: false}, + {resource: rbac.ResourceWorkspace.InOrg(unuseID).WithOwner("not-me"), actions: allActions(), allow: false}, + {resource: rbac.ResourceWorkspace.InOrg(unuseID).WithID("not-id"), actions: allActions(), allow: false}, + {resource: rbac.ResourceWorkspace.InOrg(unuseID), actions: allActions(), allow: false}, {resource: rbac.ResourceWorkspace.WithOwner("not-me").WithID("not-id"), actions: allActions(), allow: false}, {resource: rbac.ResourceWorkspace.WithOwner("not-me"), actions: allActions(), allow: false}, @@ -155,10 +156,10 @@ func TestAuthorizeDomain(t *testing.T) { {resource: rbac.ResourceWorkspace.All(), actions: allActions(), allow: false}, // Other org + me + id - {resource: rbac.ResourceWorkspace.InOrg("other").WithOwner(user.UserID).WithID(wrkID), actions: allActions(), allow: false}, - {resource: rbac.ResourceWorkspace.InOrg("other").WithOwner(user.UserID), actions: allActions(), allow: false}, - {resource: rbac.ResourceWorkspace.InOrg("other").WithID(wrkID), actions: allActions(), allow: false}, - {resource: rbac.ResourceWorkspace.InOrg("other"), actions: allActions(), allow: false}, + {resource: rbac.ResourceWorkspace.InOrg(unuseID).WithOwner(user.UserID).WithID(wrkID), actions: allActions(), allow: false}, + {resource: rbac.ResourceWorkspace.InOrg(unuseID).WithOwner(user.UserID), actions: allActions(), allow: false}, + {resource: rbac.ResourceWorkspace.InOrg(unuseID).WithID(wrkID), actions: allActions(), allow: false}, + {resource: rbac.ResourceWorkspace.InOrg(unuseID), actions: allActions(), allow: false}, // Other org + other user + id {resource: rbac.ResourceWorkspace.InOrg(defOrg).WithOwner("not-me").WithID(wrkID), actions: allActions(), allow: true}, @@ -168,10 +169,10 @@ func TestAuthorizeDomain(t *testing.T) { {resource: rbac.ResourceWorkspace.WithOwner("not-me"), actions: allActions(), allow: false}, // Other org + other use + other id - {resource: rbac.ResourceWorkspace.InOrg("other").WithOwner("not-me").WithID("not-id"), actions: allActions(), allow: false}, - {resource: rbac.ResourceWorkspace.InOrg("other").WithOwner("not-me"), actions: allActions(), allow: false}, - {resource: rbac.ResourceWorkspace.InOrg("other").WithID("not-id"), actions: allActions(), allow: false}, - {resource: rbac.ResourceWorkspace.InOrg("other"), actions: allActions(), allow: false}, + {resource: rbac.ResourceWorkspace.InOrg(unuseID).WithOwner("not-me").WithID("not-id"), actions: allActions(), allow: false}, + {resource: rbac.ResourceWorkspace.InOrg(unuseID).WithOwner("not-me"), actions: allActions(), allow: false}, + {resource: rbac.ResourceWorkspace.InOrg(unuseID).WithID("not-id"), actions: allActions(), allow: false}, + {resource: rbac.ResourceWorkspace.InOrg(unuseID), actions: allActions(), allow: false}, {resource: rbac.ResourceWorkspace.WithOwner("not-me").WithID("not-id"), actions: allActions(), allow: false}, {resource: rbac.ResourceWorkspace.WithOwner("not-me"), actions: allActions(), allow: false}, @@ -202,10 +203,10 @@ func TestAuthorizeDomain(t *testing.T) { {resource: rbac.ResourceWorkspace.All(), actions: allActions(), allow: true}, // Other org + me + id - {resource: rbac.ResourceWorkspace.InOrg("other").WithOwner(user.UserID).WithID(wrkID), actions: allActions(), allow: true}, - {resource: rbac.ResourceWorkspace.InOrg("other").WithOwner(user.UserID), actions: allActions(), allow: true}, - {resource: rbac.ResourceWorkspace.InOrg("other").WithID(wrkID), actions: allActions(), allow: true}, - {resource: rbac.ResourceWorkspace.InOrg("other"), actions: allActions(), allow: true}, + {resource: rbac.ResourceWorkspace.InOrg(unuseID).WithOwner(user.UserID).WithID(wrkID), actions: allActions(), allow: true}, + {resource: rbac.ResourceWorkspace.InOrg(unuseID).WithOwner(user.UserID), actions: allActions(), allow: true}, + {resource: rbac.ResourceWorkspace.InOrg(unuseID).WithID(wrkID), actions: allActions(), allow: true}, + {resource: rbac.ResourceWorkspace.InOrg(unuseID), actions: allActions(), allow: true}, // Other org + other user + id {resource: rbac.ResourceWorkspace.InOrg(defOrg).WithOwner("not-me").WithID(wrkID), actions: allActions(), allow: true}, @@ -215,10 +216,10 @@ func TestAuthorizeDomain(t *testing.T) { {resource: rbac.ResourceWorkspace.WithOwner("not-me"), actions: allActions(), allow: true}, // Other org + other use + other id - {resource: rbac.ResourceWorkspace.InOrg("other").WithOwner("not-me").WithID("not-id"), actions: allActions(), allow: true}, - {resource: rbac.ResourceWorkspace.InOrg("other").WithOwner("not-me"), actions: allActions(), allow: true}, - {resource: rbac.ResourceWorkspace.InOrg("other").WithID("not-id"), actions: allActions(), allow: true}, - {resource: rbac.ResourceWorkspace.InOrg("other"), actions: allActions(), allow: true}, + {resource: rbac.ResourceWorkspace.InOrg(unuseID).WithOwner("not-me").WithID("not-id"), actions: allActions(), allow: true}, + {resource: rbac.ResourceWorkspace.InOrg(unuseID).WithOwner("not-me"), actions: allActions(), allow: true}, + {resource: rbac.ResourceWorkspace.InOrg(unuseID).WithID("not-id"), actions: allActions(), allow: true}, + {resource: rbac.ResourceWorkspace.InOrg(unuseID), actions: allActions(), allow: true}, {resource: rbac.ResourceWorkspace.WithOwner("not-me").WithID("not-id"), actions: allActions(), allow: true}, {resource: rbac.ResourceWorkspace.WithOwner("not-me"), actions: allActions(), allow: true}, @@ -266,10 +267,10 @@ func TestAuthorizeDomain(t *testing.T) { {resource: rbac.ResourceWorkspace.All(), allow: false}, // Other org + me + id - {resource: rbac.ResourceWorkspace.InOrg("other").WithOwner(user.UserID).WithID(wrkID), allow: true}, - {resource: rbac.ResourceWorkspace.InOrg("other").WithOwner(user.UserID), allow: false}, - {resource: rbac.ResourceWorkspace.InOrg("other").WithID(wrkID), allow: true}, - {resource: rbac.ResourceWorkspace.InOrg("other"), allow: false}, + {resource: rbac.ResourceWorkspace.InOrg(unuseID).WithOwner(user.UserID).WithID(wrkID), allow: true}, + {resource: rbac.ResourceWorkspace.InOrg(unuseID).WithOwner(user.UserID), allow: false}, + {resource: rbac.ResourceWorkspace.InOrg(unuseID).WithID(wrkID), allow: true}, + {resource: rbac.ResourceWorkspace.InOrg(unuseID), allow: false}, // Other org + other user + id {resource: rbac.ResourceWorkspace.InOrg(defOrg).WithOwner("not-me").WithID(wrkID), allow: true}, @@ -279,10 +280,10 @@ func TestAuthorizeDomain(t *testing.T) { {resource: rbac.ResourceWorkspace.WithOwner("not-me"), allow: false}, // Other org + other use + other id - {resource: rbac.ResourceWorkspace.InOrg("other").WithOwner("not-me").WithID("not-id"), allow: false}, - {resource: rbac.ResourceWorkspace.InOrg("other").WithOwner("not-me"), allow: false}, - {resource: rbac.ResourceWorkspace.InOrg("other").WithID("not-id"), allow: false}, - {resource: rbac.ResourceWorkspace.InOrg("other"), allow: false}, + {resource: rbac.ResourceWorkspace.InOrg(unuseID).WithOwner("not-me").WithID("not-id"), allow: false}, + {resource: rbac.ResourceWorkspace.InOrg(unuseID).WithOwner("not-me"), allow: false}, + {resource: rbac.ResourceWorkspace.InOrg(unuseID).WithID("not-id"), allow: false}, + {resource: rbac.ResourceWorkspace.InOrg(unuseID), allow: false}, {resource: rbac.ResourceWorkspace.WithOwner("not-me").WithID("not-id"), allow: false}, {resource: rbac.ResourceWorkspace.WithOwner("not-me"), allow: false}, @@ -309,10 +310,10 @@ func TestAuthorizeDomain(t *testing.T) { {resource: rbac.ResourceWorkspace.All()}, // Other org + me + id - {resource: rbac.ResourceWorkspace.InOrg("other").WithOwner(user.UserID).WithID(wrkID)}, - {resource: rbac.ResourceWorkspace.InOrg("other").WithOwner(user.UserID)}, - {resource: rbac.ResourceWorkspace.InOrg("other").WithID(wrkID)}, - {resource: rbac.ResourceWorkspace.InOrg("other")}, + {resource: rbac.ResourceWorkspace.InOrg(unuseID).WithOwner(user.UserID).WithID(wrkID)}, + {resource: rbac.ResourceWorkspace.InOrg(unuseID).WithOwner(user.UserID)}, + {resource: rbac.ResourceWorkspace.InOrg(unuseID).WithID(wrkID)}, + {resource: rbac.ResourceWorkspace.InOrg(unuseID)}, // Other org + other user + id {resource: rbac.ResourceWorkspace.InOrg(defOrg).WithOwner("not-me").WithID(wrkID)}, @@ -322,10 +323,10 @@ func TestAuthorizeDomain(t *testing.T) { {resource: rbac.ResourceWorkspace.WithOwner("not-me")}, // Other org + other use + other id - {resource: rbac.ResourceWorkspace.InOrg("other").WithOwner("not-me").WithID("not-id")}, - {resource: rbac.ResourceWorkspace.InOrg("other").WithOwner("not-me")}, - {resource: rbac.ResourceWorkspace.InOrg("other").WithID("not-id")}, - {resource: rbac.ResourceWorkspace.InOrg("other")}, + {resource: rbac.ResourceWorkspace.InOrg(unuseID).WithOwner("not-me").WithID("not-id")}, + {resource: rbac.ResourceWorkspace.InOrg(unuseID).WithOwner("not-me")}, + {resource: rbac.ResourceWorkspace.InOrg(unuseID).WithID("not-id")}, + {resource: rbac.ResourceWorkspace.InOrg(unuseID)}, {resource: rbac.ResourceWorkspace.WithOwner("not-me").WithID("not-id")}, {resource: rbac.ResourceWorkspace.WithOwner("not-me")}, @@ -342,7 +343,7 @@ func TestAuthorizeDomain(t *testing.T) { Name: "ReadOnlyOrgAndUser", Site: []rbac.Permission{}, Org: map[string][]rbac.Permission{ - defOrg: {{ + defOrg.String(): {{ Negate: false, ResourceType: "*", ResourceID: "*", @@ -381,10 +382,10 @@ func TestAuthorizeDomain(t *testing.T) { {resource: rbac.ResourceWorkspace.All(), allow: false}, // Other org + me + id - {resource: rbac.ResourceWorkspace.InOrg("other").WithOwner(user.UserID).WithID(wrkID), allow: false}, - {resource: rbac.ResourceWorkspace.InOrg("other").WithOwner(user.UserID), allow: false}, - {resource: rbac.ResourceWorkspace.InOrg("other").WithID(wrkID), allow: false}, - {resource: rbac.ResourceWorkspace.InOrg("other"), allow: false}, + {resource: rbac.ResourceWorkspace.InOrg(unuseID).WithOwner(user.UserID).WithID(wrkID), allow: false}, + {resource: rbac.ResourceWorkspace.InOrg(unuseID).WithOwner(user.UserID), allow: false}, + {resource: rbac.ResourceWorkspace.InOrg(unuseID).WithID(wrkID), allow: false}, + {resource: rbac.ResourceWorkspace.InOrg(unuseID), allow: false}, // Other org + other user + id {resource: rbac.ResourceWorkspace.InOrg(defOrg).WithOwner("not-me").WithID(wrkID), allow: true}, @@ -394,10 +395,10 @@ func TestAuthorizeDomain(t *testing.T) { {resource: rbac.ResourceWorkspace.WithOwner("not-me"), allow: false}, // Other org + other use + other id - {resource: rbac.ResourceWorkspace.InOrg("other").WithOwner("not-me").WithID("not-id"), allow: false}, - {resource: rbac.ResourceWorkspace.InOrg("other").WithOwner("not-me"), allow: false}, - {resource: rbac.ResourceWorkspace.InOrg("other").WithID("not-id"), allow: false}, - {resource: rbac.ResourceWorkspace.InOrg("other"), allow: false}, + {resource: rbac.ResourceWorkspace.InOrg(unuseID).WithOwner("not-me").WithID("not-id"), allow: false}, + {resource: rbac.ResourceWorkspace.InOrg(unuseID).WithOwner("not-me"), allow: false}, + {resource: rbac.ResourceWorkspace.InOrg(unuseID).WithID("not-id"), allow: false}, + {resource: rbac.ResourceWorkspace.InOrg(unuseID), allow: false}, {resource: rbac.ResourceWorkspace.WithOwner("not-me").WithID("not-id"), allow: false}, {resource: rbac.ResourceWorkspace.WithOwner("not-me"), allow: false}, @@ -426,10 +427,10 @@ func TestAuthorizeDomain(t *testing.T) { {resource: rbac.ResourceWorkspace.All()}, // Other org + me + id - {resource: rbac.ResourceWorkspace.InOrg("other").WithOwner(user.UserID).WithID(wrkID)}, - {resource: rbac.ResourceWorkspace.InOrg("other").WithOwner(user.UserID)}, - {resource: rbac.ResourceWorkspace.InOrg("other").WithID(wrkID)}, - {resource: rbac.ResourceWorkspace.InOrg("other")}, + {resource: rbac.ResourceWorkspace.InOrg(unuseID).WithOwner(user.UserID).WithID(wrkID)}, + {resource: rbac.ResourceWorkspace.InOrg(unuseID).WithOwner(user.UserID)}, + {resource: rbac.ResourceWorkspace.InOrg(unuseID).WithID(wrkID)}, + {resource: rbac.ResourceWorkspace.InOrg(unuseID)}, // Other org + other user + id {resource: rbac.ResourceWorkspace.InOrg(defOrg).WithOwner("not-me").WithID(wrkID)}, @@ -439,10 +440,10 @@ func TestAuthorizeDomain(t *testing.T) { {resource: rbac.ResourceWorkspace.WithOwner("not-me")}, // Other org + other use + other id - {resource: rbac.ResourceWorkspace.InOrg("other").WithOwner("not-me").WithID("not-id")}, - {resource: rbac.ResourceWorkspace.InOrg("other").WithOwner("not-me")}, - {resource: rbac.ResourceWorkspace.InOrg("other").WithID("not-id")}, - {resource: rbac.ResourceWorkspace.InOrg("other")}, + {resource: rbac.ResourceWorkspace.InOrg(unuseID).WithOwner("not-me").WithID("not-id")}, + {resource: rbac.ResourceWorkspace.InOrg(unuseID).WithOwner("not-me")}, + {resource: rbac.ResourceWorkspace.InOrg(unuseID).WithID("not-id")}, + {resource: rbac.ResourceWorkspace.InOrg(unuseID)}, {resource: rbac.ResourceWorkspace.WithOwner("not-me").WithID("not-id")}, {resource: rbac.ResourceWorkspace.WithOwner("not-me")}, @@ -454,7 +455,8 @@ func TestAuthorizeDomain(t *testing.T) { // TestAuthorizeLevels ensures level overrides are acting appropriately //nolint:paralleltest func TestAuthorizeLevels(t *testing.T) { - defOrg := "default" + defOrg := uuid.New() + unusedID := uuid.New() wrkID := "1234" user := subject{ @@ -462,9 +464,9 @@ func TestAuthorizeLevels(t *testing.T) { Roles: []rbac.Role{ must(rbac.RoleByName(rbac.RoleAdmin())), { - Name: "org-deny" + defOrg, + Name: "org-deny:" + defOrg.String(), Org: map[string][]rbac.Permission{ - defOrg: { + defOrg.String(): { { Negate: true, ResourceType: "*", @@ -509,10 +511,10 @@ func TestAuthorizeLevels(t *testing.T) { {resource: rbac.ResourceWorkspace.All()}, // Other org + me + id - {resource: rbac.ResourceWorkspace.InOrg("other").WithOwner(user.UserID).WithID(wrkID)}, - {resource: rbac.ResourceWorkspace.InOrg("other").WithOwner(user.UserID)}, - {resource: rbac.ResourceWorkspace.InOrg("other").WithID(wrkID)}, - {resource: rbac.ResourceWorkspace.InOrg("other")}, + {resource: rbac.ResourceWorkspace.InOrg(unusedID).WithOwner(user.UserID).WithID(wrkID)}, + {resource: rbac.ResourceWorkspace.InOrg(unusedID).WithOwner(user.UserID)}, + {resource: rbac.ResourceWorkspace.InOrg(unusedID).WithID(wrkID)}, + {resource: rbac.ResourceWorkspace.InOrg(unusedID)}, // Other org + other user + id {resource: rbac.ResourceWorkspace.InOrg(defOrg).WithOwner("not-me").WithID(wrkID)}, @@ -522,10 +524,10 @@ func TestAuthorizeLevels(t *testing.T) { {resource: rbac.ResourceWorkspace.WithOwner("not-me")}, // Other org + other use + other id - {resource: rbac.ResourceWorkspace.InOrg("other").WithOwner("not-me").WithID("not-id")}, - {resource: rbac.ResourceWorkspace.InOrg("other").WithOwner("not-me")}, - {resource: rbac.ResourceWorkspace.InOrg("other").WithID("not-id")}, - {resource: rbac.ResourceWorkspace.InOrg("other")}, + {resource: rbac.ResourceWorkspace.InOrg(unusedID).WithOwner("not-me").WithID("not-id")}, + {resource: rbac.ResourceWorkspace.InOrg(unusedID).WithOwner("not-me")}, + {resource: rbac.ResourceWorkspace.InOrg(unusedID).WithID("not-id")}, + {resource: rbac.ResourceWorkspace.InOrg(unusedID)}, {resource: rbac.ResourceWorkspace.WithOwner("not-me").WithID("not-id")}, {resource: rbac.ResourceWorkspace.WithOwner("not-me")}, @@ -582,10 +584,10 @@ func TestAuthorizeLevels(t *testing.T) { {resource: rbac.ResourceWorkspace.All(), allow: false}, // Other org + me + id - {resource: rbac.ResourceWorkspace.InOrg("other").WithOwner(user.UserID).WithID(wrkID), allow: false}, - {resource: rbac.ResourceWorkspace.InOrg("other").WithOwner(user.UserID), allow: false}, - {resource: rbac.ResourceWorkspace.InOrg("other").WithID(wrkID), allow: false}, - {resource: rbac.ResourceWorkspace.InOrg("other"), allow: false}, + {resource: rbac.ResourceWorkspace.InOrg(unusedID).WithOwner(user.UserID).WithID(wrkID), allow: false}, + {resource: rbac.ResourceWorkspace.InOrg(unusedID).WithOwner(user.UserID), allow: false}, + {resource: rbac.ResourceWorkspace.InOrg(unusedID).WithID(wrkID), allow: false}, + {resource: rbac.ResourceWorkspace.InOrg(unusedID), allow: false}, // Other org + other user + id {resource: rbac.ResourceWorkspace.InOrg(defOrg).WithOwner("not-me").WithID(wrkID), allow: true}, @@ -595,10 +597,10 @@ func TestAuthorizeLevels(t *testing.T) { {resource: rbac.ResourceWorkspace.WithOwner("not-me"), allow: false}, // Other org + other use + other id - {resource: rbac.ResourceWorkspace.InOrg("other").WithOwner("not-me").WithID("not-id"), allow: false}, - {resource: rbac.ResourceWorkspace.InOrg("other").WithOwner("not-me"), allow: false}, - {resource: rbac.ResourceWorkspace.InOrg("other").WithID("not-id"), allow: false}, - {resource: rbac.ResourceWorkspace.InOrg("other"), allow: false}, + {resource: rbac.ResourceWorkspace.InOrg(unusedID).WithOwner("not-me").WithID("not-id"), allow: false}, + {resource: rbac.ResourceWorkspace.InOrg(unusedID).WithOwner("not-me"), allow: false}, + {resource: rbac.ResourceWorkspace.InOrg(unusedID).WithID("not-id"), allow: false}, + {resource: rbac.ResourceWorkspace.InOrg(unusedID), allow: false}, {resource: rbac.ResourceWorkspace.WithOwner("not-me").WithID("not-id"), allow: false}, {resource: rbac.ResourceWorkspace.WithOwner("not-me"), allow: false}, diff --git a/coderd/rbac/builtin.go b/coderd/rbac/builtin.go index cb39b303a6c74..e945e9038fa8a 100644 --- a/coderd/rbac/builtin.go +++ b/coderd/rbac/builtin.go @@ -206,11 +206,15 @@ func roleName(name string, scopeID string) string { } func roleSplit(role string) (name string, scopeID string, err error) { - arr := strings.Split(name, ":") + arr := strings.Split(role, ":") if len(arr) > 2 { return "", "", xerrors.Errorf("too many colons in role name") } + if arr[0] == "" { + return "", "", xerrors.Errorf("role cannot be the empty string") + } + if len(arr) == 2 { return arr[0], arr[1], nil } diff --git a/coderd/rbac/example_test.go b/coderd/rbac/example_test.go index 8230b13cac943..092fbad967961 100644 --- a/coderd/rbac/example_test.go +++ b/coderd/rbac/example_test.go @@ -2,6 +2,7 @@ package rbac_test import ( "context" + "github.com/google/uuid" "testing" "github.com/stretchr/testify/require" @@ -16,6 +17,7 @@ func TestExample(t *testing.T) { ctx := context.Background() authorizer, err := rbac.NewAuthorizer() require.NoError(t, err) + defaultOrg := uuid.New() // user will become an authn object, and can even be a database.User if it // fulfills the interface. Until then, use a placeholder. @@ -23,7 +25,7 @@ func TestExample(t *testing.T) { UserID: "alice", Roles: []rbac.Role{ must(rbac.RoleByName(rbac.RoleMember())), - must(rbac.RoleByName(rbac.RoleOrgAdmin("default"))), + must(rbac.RoleByName(rbac.RoleOrgAdmin(defaultOrg))), }, } @@ -38,17 +40,17 @@ func TestExample(t *testing.T) { //nolint:paralleltest t.Run("ReadOrgWorkspaces", func(t *testing.T) { // To read all workspaces on the org 'default' - err := authorizer.Authorize(ctx, user.UserID, user.Roles, rbac.ActionRead, rbac.ResourceWorkspace.InOrg("default")) + err := authorizer.Authorize(ctx, user.UserID, user.Roles, rbac.ActionRead, rbac.ResourceWorkspace.InOrg(defaultOrg)) require.NoError(t, err, "this user can read all org workspaces in 'default'") }) //nolint:paralleltest t.Run("ReadMyWorkspace", func(t *testing.T) { // Note 'database.Workspace' could fulfill the object interface and be passed in directly - err := authorizer.Authorize(ctx, user.UserID, user.Roles, rbac.ActionRead, rbac.ResourceWorkspace.InOrg("default").WithOwner(user.UserID)) + err := authorizer.Authorize(ctx, user.UserID, user.Roles, rbac.ActionRead, rbac.ResourceWorkspace.InOrg(defaultOrg).WithOwner(user.UserID)) require.NoError(t, err, "this user can their workspace") - err = authorizer.Authorize(ctx, user.UserID, user.Roles, rbac.ActionRead, rbac.ResourceWorkspace.InOrg("default").WithOwner(user.UserID).WithID("1234")) + err = authorizer.Authorize(ctx, user.UserID, user.Roles, rbac.ActionRead, rbac.ResourceWorkspace.InOrg(defaultOrg).WithOwner(user.UserID).WithID("1234")) require.NoError(t, err, "this user can read workspace '1234'") }) } diff --git a/coderd/rbac/object.go b/coderd/rbac/object.go index c8b7c94f1d9dc..1e86165f24cf4 100644 --- a/coderd/rbac/object.go +++ b/coderd/rbac/object.go @@ -1,5 +1,9 @@ package rbac +import ( + "github.com/google/uuid" +) + const WildcardSymbol = "*" // Resources are just typed objects. Making resources this way allows directly @@ -46,11 +50,11 @@ func (z Object) All() Object { } // InOrg adds an org OwnerID to the resource -func (z Object) InOrg(orgID string) Object { +func (z Object) InOrg(orgID uuid.UUID) Object { return Object{ ResourceID: z.ResourceID, Owner: z.Owner, - OrgID: orgID, + OrgID: orgID.String(), Type: z.Type, } } From aa6fe882a30d0a982b24b98ff59cbb688226f111 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Wed, 27 Apr 2022 14:57:17 -0500 Subject: [PATCH 8/9] Correct indentation on sql --- coderd/database/queries/users.sql | 2 +- coderd/httpmw/rbac.go | 9 --------- 2 files changed, 1 insertion(+), 10 deletions(-) delete mode 100644 coderd/httpmw/rbac.go diff --git a/coderd/database/queries/users.sql b/coderd/database/queries/users.sql index 04f05b2bfd9e3..a9f46a3eb8296 100644 --- a/coderd/database/queries/users.sql +++ b/coderd/database/queries/users.sql @@ -56,7 +56,7 @@ SET -- Append new roles and remove duplicates just to keep things clean. rbac_roles = ARRAY(SELECT DISTINCT UNNEST(rbac_roles || @granted_roles :: text[])) WHERE - id = @id + id = @id RETURNING *; -- name: GetUsers :many diff --git a/coderd/httpmw/rbac.go b/coderd/httpmw/rbac.go deleted file mode 100644 index 63377cd4ca7fb..0000000000000 --- a/coderd/httpmw/rbac.go +++ /dev/null @@ -1,9 +0,0 @@ -package httpmw - -//func Can(action rbac.Action, object rbac.Object) func(http.Handler) http.Handler { -// return func(next http.Handler) http.Handler { -// return http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) { -// -// }, -// } -//} From a6436708df68b1aa334b18c0be0686ef678e1b2c Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Wed, 27 Apr 2022 15:41:35 -0500 Subject: [PATCH 9/9] Add endpoints to grant new roles to a user Currently can't remove roles. So be careful --- coderd/coderd.go | 5 ++ coderd/database/databasefake/databasefake.go | 16 ++++ coderd/database/querier.go | 1 + coderd/database/queries.sql.go | 40 +++++++++- .../database/queries/organizationmembers.sql | 9 +++ coderd/rbac/builtin.go | 25 ++---- coderd/users.go | 78 ++++++++++++++++++- coderd/users_test.go | 57 ++++++++++++++ codersdk/users.go | 36 +++++++++ site/src/api/typesGenerated.ts | 12 +-- 10 files changed, 249 insertions(+), 30 deletions(-) diff --git a/coderd/coderd.go b/coderd/coderd.go index 2f8bde36e4638..4d6b7c8b56ca0 100644 --- a/coderd/coderd.go +++ b/coderd/coderd.go @@ -153,6 +153,7 @@ func New(options *Options) (http.Handler, func()) { r.Get("/listen", api.provisionerDaemonsListen) }) }) + r.Route("/users", func(r chi.Router) { r.Get("/first", api.firstUser) r.Post("/first", api.postFirstUser) @@ -173,6 +174,10 @@ func New(options *Options) (http.Handler, func()) { r.Use(httpmw.ExtractUserParam(options.Database)) r.Get("/", api.userByName) r.Put("/profile", api.putUserProfile) + // TODO: @emyrk Might want to move these to a /roles group instead of /user. + // As we include more roles like org roles, it makes less sense to scope these here. + r.Put("/roles", api.putUserRoles) + r.Get("/roles", api.getUserRoles) r.Get("/organizations", api.organizationsByUser) r.Post("/organizations", api.postOrganizationsByUser) r.Post("/keys", api.postAPIKey) diff --git a/coderd/database/databasefake/databasefake.go b/coderd/database/databasefake/databasefake.go index 5b75afd31c261..276f52f8c0ff9 100644 --- a/coderd/database/databasefake/databasefake.go +++ b/coderd/database/databasefake/databasefake.go @@ -689,6 +689,21 @@ func (q *fakeQuerier) GetOrganizationMemberByUserID(_ context.Context, arg datab return database.OrganizationMember{}, sql.ErrNoRows } +func (q *fakeQuerier) GetOrganizationMembershipsByUserID(ctx context.Context, userID uuid.UUID) ([]database.OrganizationMember, error) { + q.mutex.RLock() + defer q.mutex.RUnlock() + + var memberships []database.OrganizationMember + for _, organizationMember := range q.organizationMembers { + mem := organizationMember + if mem.UserID != userID { + continue + } + memberships = append(memberships, mem) + } + return memberships, nil +} + func (q *fakeQuerier) GetProvisionerDaemons(_ context.Context) ([]database.ProvisionerDaemon, error) { q.mutex.RLock() defer q.mutex.RUnlock() @@ -1118,6 +1133,7 @@ func (q *fakeQuerier) InsertUser(_ context.Context, arg database.InsertUserParam CreatedAt: arg.CreatedAt, UpdatedAt: arg.UpdatedAt, Username: arg.Username, + RbacRoles: arg.RbacRoles, } q.users = append(q.users, user) return user, nil diff --git a/coderd/database/querier.go b/coderd/database/querier.go index 00b7013bbfbd5..0cf42280f4af5 100644 --- a/coderd/database/querier.go +++ b/coderd/database/querier.go @@ -18,6 +18,7 @@ type querier interface { GetOrganizationByID(ctx context.Context, id uuid.UUID) (Organization, error) GetOrganizationByName(ctx context.Context, name string) (Organization, error) GetOrganizationMemberByUserID(ctx context.Context, arg GetOrganizationMemberByUserIDParams) (OrganizationMember, error) + GetOrganizationMembershipsByUserID(ctx context.Context, userID uuid.UUID) ([]OrganizationMember, error) GetOrganizations(ctx context.Context) ([]Organization, error) GetOrganizationsByUserID(ctx context.Context, userID uuid.UUID) ([]Organization, error) GetParameterSchemasByJobID(ctx context.Context, jobID uuid.UUID) ([]ParameterSchema, error) diff --git a/coderd/database/queries.sql.go b/coderd/database/queries.sql.go index 904358cef2e5f..541b69313fc3b 100644 --- a/coderd/database/queries.sql.go +++ b/coderd/database/queries.sql.go @@ -336,6 +336,44 @@ func (q *sqlQuerier) GetOrganizationMemberByUserID(ctx context.Context, arg GetO return i, err } +const getOrganizationMembershipsByUserID = `-- name: GetOrganizationMembershipsByUserID :many +SELECT + user_id, organization_id, created_at, updated_at, roles +FROM + organization_members +WHERE + user_id = $1 +` + +func (q *sqlQuerier) GetOrganizationMembershipsByUserID(ctx context.Context, userID uuid.UUID) ([]OrganizationMember, error) { + rows, err := q.db.QueryContext(ctx, getOrganizationMembershipsByUserID, userID) + if err != nil { + return nil, err + } + defer rows.Close() + var items []OrganizationMember + for rows.Next() { + var i OrganizationMember + if err := rows.Scan( + &i.UserID, + &i.OrganizationID, + &i.CreatedAt, + &i.UpdatedAt, + pq.Array(&i.Roles), + ); err != nil { + return nil, err + } + items = append(items, i) + } + if err := rows.Close(); err != nil { + return nil, err + } + if err := rows.Err(); err != nil { + return nil, err + } + return items, nil +} + const insertOrganizationMember = `-- name: InsertOrganizationMember :one INSERT INTO organization_members ( @@ -1946,7 +1984,7 @@ SET -- Append new roles and remove duplicates just to keep things clean. rbac_roles = ARRAY(SELECT DISTINCT UNNEST(rbac_roles || $1 :: text[])) WHERE - id = $2 + id = $2 RETURNING id, email, username, hashed_password, created_at, updated_at, rbac_roles ` diff --git a/coderd/database/queries/organizationmembers.sql b/coderd/database/queries/organizationmembers.sql index 243bdc26c9878..cc3bfac9c48cb 100644 --- a/coderd/database/queries/organizationmembers.sql +++ b/coderd/database/queries/organizationmembers.sql @@ -20,3 +20,12 @@ INSERT INTO ) VALUES ($1, $2, $3, $4, $5) RETURNING *; + + +-- name: GetOrganizationMembershipsByUserID :many +SELECT + * +FROM + organization_members +WHERE + user_id = $1; \ No newline at end of file diff --git a/coderd/rbac/builtin.go b/coderd/rbac/builtin.go index e945e9038fa8a..f1eae9e705bf7 100644 --- a/coderd/rbac/builtin.go +++ b/coderd/rbac/builtin.go @@ -12,9 +12,8 @@ const ( member string = "member" auditor string = "auditor" - orgAdmin string = "organization-admin" - orgMember string = "organization-member" - orgManager string = "organization-manager" + orgAdmin string = "organization-admin" + orgMember string = "organization-member" ) // RoleName is a string that represents a registered rbac role. We want to store @@ -26,7 +25,7 @@ type RoleName = string // The functions below ONLY need to exist for roles that are "defaulted" in some way. // Any other roles (like auditor), can be listed and let the user select/assigned. -// Once we have a database implementation, the "default" roles be be defined on the +// Once we have a database implementation, the "default" roles can be defined on the // site and orgs, and these functions can be removed. func RoleAdmin() RoleName { @@ -49,10 +48,9 @@ func RoleOrgMember(organizationID uuid.UUID) RoleName { var ( // builtInRoles are just a hard coded set for now. Ideally we store these in // the database. Right now they are functions because the org id should scope - // certain roles. If we store them in the database, we will need to store - // them such that the "org" permissions are dynamically changed by the - // scopeID passed in. This isn't a hard problem to solve, it's just easier - // as a function right now. + // certain roles. When we store them in the database, each organization should + // create the roles that are assignable in the org. This isn't a hard problem to solve, + // it's just easier as a function right now. // // This map will be replaced by database storage defined by this ticket. // https://github.com/coder/coder/issues/1194 @@ -120,17 +118,6 @@ var ( }, } }, - - orgManager: func(organizationID string) Role { - return Role{ - Name: roleName(orgMember, organizationID), - Org: map[string][]Permission{ - organizationID: permissions(map[Object][]Action{ - ResourceWorkspace: {WildcardSymbol}, - }), - }, - } - }, } ) diff --git a/coderd/users.go b/coderd/users.go index 20ffdf01f37d0..419ea84b6b2c6 100644 --- a/coderd/users.go +++ b/coderd/users.go @@ -11,8 +11,6 @@ import ( "strconv" "time" - "github.com/coder/coder/coderd/rbac" - "github.com/go-chi/chi/v5" "github.com/go-chi/render" "github.com/google/uuid" @@ -23,6 +21,7 @@ import ( "github.com/coder/coder/coderd/gitsshkey" "github.com/coder/coder/coderd/httpapi" "github.com/coder/coder/coderd/httpmw" + "github.com/coder/coder/coderd/rbac" "github.com/coder/coder/coderd/userpassword" "github.com/coder/coder/codersdk" "github.com/coder/coder/cryptorand" @@ -300,6 +299,69 @@ func (api *api) putUserProfile(rw http.ResponseWriter, r *http.Request) { httpapi.Write(rw, http.StatusOK, convertUser(updatedUserProfile)) } +func (api *api) getUserRoles(rw http.ResponseWriter, r *http.Request) { + user := httpmw.UserParam(r) + + resp := codersdk.UserRoles{ + Roles: user.RbacRoles, + } + + memberships, err := api.Database.GetOrganizationMembershipsByUserID(r.Context(), user.ID) + if err != nil { + httpapi.Write(rw, http.StatusInternalServerError, httpapi.Response{ + Message: fmt.Sprintf("get user memberships: %s", err), + }) + return + } + + for _, mem := range memberships { + resp.Roles = append(resp.Roles, mem.Roles...) + } + + httpapi.Write(rw, http.StatusOK, resp) +} + +func (api *api) putUserRoles(rw http.ResponseWriter, r *http.Request) { + // User is the user to modify + // TODO: Until rbac authorize is implemented, only be able to change your + // own roles. This also means you can grant yourself whatever roles you want. + user := httpmw.UserParam(r) + apiKey := httpmw.APIKey(r) + if apiKey.UserID != user.ID { + httpapi.Write(rw, http.StatusUnauthorized, httpapi.Response{ + Message: fmt.Sprintf("modifying other users is not supported at this time"), + }) + return + } + + var params codersdk.GrantUserRoles + if !httpapi.Read(rw, r, ¶ms) { + return + } + + for _, r := range params.Roles { + if _, err := rbac.RoleByName(r); err != nil { + httpapi.Write(rw, http.StatusBadRequest, httpapi.Response{ + Message: fmt.Sprintf("%q is not a supported role", r), + }) + return + } + } + + updatedUser, err := api.Database.GrantUserRole(r.Context(), database.GrantUserRoleParams{ + GrantedRoles: params.Roles, + ID: user.ID, + }) + if err != nil { + httpapi.Write(rw, http.StatusInternalServerError, httpapi.Response{ + Message: fmt.Sprintf("get user: %s", err), + }) + return + } + + httpapi.Write(rw, http.StatusOK, convertUser(updatedUser)) +} + // Returns organizations the parameterized user has access to. func (api *api) organizationsByUser(rw http.ResponseWriter, r *http.Request) { user := httpmw.UserParam(r) @@ -396,7 +458,11 @@ func (api *api) postOrganizationsByUser(rw http.ResponseWriter, r *http.Request) UserID: user.ID, CreatedAt: database.Now(), UpdatedAt: database.Now(), - Roles: []string{"organization-admin"}, + 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) @@ -889,6 +955,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 // If no organization is provided, create a new one for the user. if req.OrganizationID == uuid.Nil { organization, err := db.InsertOrganization(ctx, database.InsertOrganizationParams{ @@ -901,7 +968,10 @@ func (api *api) createUser(ctx context.Context, req codersdk.CreateUserRequest) return xerrors.Errorf("create organization: %w", err) } 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(), @@ -947,7 +1017,7 @@ func (api *api) createUser(ctx context.Context, req codersdk.CreateUserRequest) CreatedAt: database.Now(), UpdatedAt: database.Now(), // By default give them membership to the organization - Roles: []string{rbac.RoleOrgMember(req.OrganizationID)}, + Roles: orgRoles, }) if err != nil { return xerrors.Errorf("create organization member: %w", err) diff --git a/coderd/users_test.go b/coderd/users_test.go index 845179bb75ca8..1e493899da1c2 100644 --- a/coderd/users_test.go +++ b/coderd/users_test.go @@ -7,6 +7,8 @@ import ( "sort" "testing" + "github.com/coder/coder/coderd/rbac" + "github.com/google/uuid" "github.com/stretchr/testify/require" @@ -286,6 +288,61 @@ func TestUpdateUserProfile(t *testing.T) { }) } +func TestGrantRoles(t *testing.T) { + t.Parallel() + t.Run("FirstUserRoles", func(t *testing.T) { + t.Parallel() + ctx := context.Background() + client := coderdtest.New(t, nil) + first := coderdtest.CreateFirstUser(t, client) + + roles, err := client.GetUserRoles(ctx, codersdk.Me) + require.NoError(t, err) + require.ElementsMatch(t, roles.Roles, []string{ + rbac.RoleAdmin(), + rbac.RoleMember(), + rbac.RoleOrgMember(first.OrganizationID), + rbac.RoleOrgAdmin(first.OrganizationID), + }, "should be a member and admin") + }) + + t.Run("GrantAdmin", func(t *testing.T) { + t.Parallel() + ctx := context.Background() + admin := coderdtest.New(t, nil) + first := coderdtest.CreateFirstUser(t, admin) + + 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(), + rbac.RoleOrgMember(first.OrganizationID), + }, "should be a member and admin") + + // Grant + // TODO: @emyrk this should be 'admin.GrantUserRoles' once proper authz + // is enforced. + _, err = member.GrantUserRoles(ctx, codersdk.Me, codersdk.GrantUserRoles{ + Roles: []string{ + rbac.RoleAdmin(), + rbac.RoleOrgAdmin(first.OrganizationID), + }, + }) + require.NoError(t, err, "grant member admin role") + + roles, err = member.GetUserRoles(ctx, codersdk.Me) + require.NoError(t, err) + require.ElementsMatch(t, roles.Roles, []string{ + rbac.RoleMember(), + rbac.RoleOrgMember(first.OrganizationID), + + rbac.RoleAdmin(), + rbac.RoleOrgAdmin(first.OrganizationID), + }, "should be a member and admin") + }) +} + func TestUserByName(t *testing.T) { t.Parallel() client := coderdtest.New(t, nil) diff --git a/codersdk/users.go b/codersdk/users.go index da5f8f23fa475..3a6296b631841 100644 --- a/codersdk/users.go +++ b/codersdk/users.go @@ -61,6 +61,14 @@ type UpdateUserProfileRequest struct { Username string `json:"username" validate:"required,username"` } +type GrantUserRoles struct { + Roles []string `json:"roles" validate:"required"` +} + +type UserRoles struct { + Roles []string `json:"roles"` +} + // LoginWithPasswordRequest enables callers to authenticate with email and password. type LoginWithPasswordRequest struct { Email string `json:"email" validate:"required,email"` @@ -155,6 +163,34 @@ func (c *Client) UpdateUserProfile(ctx context.Context, userID uuid.UUID, req Up return user, json.NewDecoder(res.Body).Decode(&user) } +// GrantUserRoles grants the userID the specified roles +func (c *Client) GrantUserRoles(ctx context.Context, userID uuid.UUID, req GrantUserRoles) (User, error) { + res, err := c.request(ctx, http.MethodPut, fmt.Sprintf("/api/v2/users/%s/roles", uuidOrMe(userID)), req) + if err != nil { + return User{}, err + } + defer res.Body.Close() + if res.StatusCode != http.StatusOK { + return User{}, readBodyAsError(res) + } + var user User + return user, json.NewDecoder(res.Body).Decode(&user) +} + +// GetUserRoles returns all roles the user has +func (c *Client) GetUserRoles(ctx context.Context, userID uuid.UUID) (UserRoles, error) { + res, err := c.request(ctx, http.MethodGet, fmt.Sprintf("/api/v2/users/%s/roles", uuidOrMe(userID)), nil) + if err != nil { + return UserRoles{}, err + } + defer res.Body.Close() + if res.StatusCode != http.StatusOK { + return UserRoles{}, readBodyAsError(res) + } + var roles UserRoles + return roles, json.NewDecoder(res.Body).Decode(&roles) +} + // CreateAPIKey generates an API key for the user ID provided. func (c *Client) CreateAPIKey(ctx context.Context, userID uuid.UUID) (*GenerateAPIKeyResponse, error) { res, err := c.request(ctx, http.MethodPost, fmt.Sprintf("/api/v2/users/%s/keys", uuidOrMe(userID)), nil) diff --git a/site/src/api/typesGenerated.ts b/site/src/api/typesGenerated.ts index 230e600eeeb18..4ce99f8a301d3 100644 --- a/site/src/api/typesGenerated.ts +++ b/site/src/api/typesGenerated.ts @@ -105,33 +105,33 @@ export interface UpdateUserProfileRequest { readonly username: string } -// From codersdk/users.go:65:6. +// From codersdk/users.go:73:6. export interface LoginWithPasswordRequest { readonly email: string readonly password: string } -// From codersdk/users.go:71:6. +// From codersdk/users.go:79:6. export interface LoginWithPasswordResponse { readonly session_token: string } -// From codersdk/users.go:76:6. +// From codersdk/users.go:84:6. export interface GenerateAPIKeyResponse { readonly key: string } -// From codersdk/users.go:80:6. +// From codersdk/users.go:88:6. export interface CreateOrganizationRequest { readonly name: string } -// From codersdk/users.go:85:6. +// From codersdk/users.go:93:6. export interface CreateWorkspaceRequest { readonly name: string } -// From codersdk/users.go:94:6. +// From codersdk/users.go:102:6. export interface AuthMethods { readonly password: boolean readonly github: boolean