feat: Add user roles, but do not yet enforce them #1200

Emyrk · 2022-04-27T21:01:30Z

What this does

Adds some builtin roles and assigns them to users and org members. This does not implement enforcing roles at this time.

Granting user's roles was added to test the api. See how to grant roles from the SDK. The "strange" part is that the rbac library is used to find the role name. In practice, there will be an endpoint to list all roles that can be granted, and the user will select from a list. The code for this exists, but I want to add this in future PRs as I work with the FE on endpoint format.

coder/coderd/users_test.go

Lines 326 to 330 in a643670

    
           _, err = member.GrantUserRoles(ctx, codersdk.Me, codersdk.GrantUserRoles{ 
        
           	Roles: []string{ 
        
           		rbac.RoleAdmin(), 
        
           		rbac.RoleOrgAdmin(first.OrganizationID), 
        
           	},

Future Work

Add ability to remove roles
Add proper authorize usage on endpoints
List roles that can be granted

Currently can't remove roles. So be careful

Emyrk · 2022-04-27T21:02:57Z

coderd/coderd.go

+					// 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)


I'd rather not bikeshed this url placement atm. I'll move the path when I talk with FE. I know the mvp for endpoints, and will change them in another PR.

Really just trying to get some footing for roles so I can start enforcing them without breaking things.

codecov · 2022-04-27T21:04:22Z

Codecov Report

Merging #1200 (96fb498) into main (ba4c3ce) will increase coverage by 0.14%.
The diff coverage is 75.60%.

@@            Coverage Diff             @@
##             main    #1200      +/-   ##
==========================================
+ Coverage   65.45%   65.59%   +0.14%     
==========================================
  Files         268      269       +1     
  Lines       17029    17337     +308     
  Branches      162      162              
==========================================
+ Hits        11147    11373     +226     
- Misses       4710     4772      +62     
- Partials     1172     1192      +20

Flag	Coverage Δ
unittest-go-macos-latest	`52.89% <63.27%> (+0.02%)`	⬆️
unittest-go-postgres-	`64.73% <75.60%> (+0.14%)`	⬆️
unittest-go-ubuntu-latest	`55.30% <63.27%> (+0.06%)`	⬆️
unittest-go-windows-2022	`52.44% <63.27%> (-0.03%)`	⬇️
unittest-js	`68.85% <ø> (ø)`

Impacted Files	Coverage Δ
coderd/audit/table.go	`77.77% <ø> (ø)`
coderd/rbac/authz.go	`55.31% <0.00%> (-13.11%)`	⬇️
coderd/users.go	`61.20% <62.92%> (+0.19%)`	⬆️
codersdk/users.go	`64.28% <63.63%> (-0.13%)`	⬇️
coderd/members.go	`66.66% <66.66%> (ø)`
coderd/database/queries.sql.go	`81.07% <79.31%> (-0.07%)`	⬇️
coderd/rbac/builtin.go	`96.29% <96.29%> (ø)`
coderd/coderd.go	`97.66% <100.00%> (+0.12%)`	⬆️
coderd/rbac/object.go	`100.00% <100.00%> (ø)`
peer/channel.go	`84.97% <0.00%> (-0.58%)`	⬇️
... and 7 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ba4c3ce...96fb498. Read the comment docs.

kylecarbs

All minor things. I'll approve once we resolve some of the outstanding q's.

coderd/database/migrations/000008_rbac_user_roles.up.sql

coderd/database/models.go

kylecarbs · 2022-04-28T14:01:48Z

coderd/rbac/builtin.go

+	orgAdmin  string = "organization-admin"
+	orgMember string = "organization-member"


Could we reuse the admin and member strings? From what I can tell, it'll be scoped to an organization anyways.

We totally can, they are scoped. I was adding the organization prefix because I imagine in the UI they will strip the "scopeID" when assigning roles. I kinda like the fact org roles have an org prefix to prevent any misunderstanding.

Eg: A dropdown list of roles you can add to a org member, will say organization-member, not organization-member:00000000-00000000-00000000-00000000.

We probably wouldn't have a single dropdown to show organization and site-level roles though, because that'd easily be misinterpreted by users IMO.

coderd/rbac/builtin.go

kylecarbs · 2022-04-28T14:10:32Z

coderd/rbac/builtin.go

+// 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 {


Doesn't seem this is used anywhere right now!

It's not, but it will be when I add the endpoints for retrieving roles that can applied. I wrote this to show how to get the roles for that endpoint in this design.

When we go to a database model, these won't be dynamic lists, so all this looping goes away. I'll drop this and bring it back in my next pr

That makes sense, thanks for the context!

coderd/rbac/builtin.go

coderd/database/migrations/000008_rbac_user_roles.down.sql

kylecarbs · 2022-04-28T14:22:34Z

coderd/database/migrations/000008_rbac_user_roles.up.sql

+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)


We might not want to update this data in a migration right now. I'm concerned that changing this behavior will be difficult down the road, so I'd prefer if we just straight-up broke the schema right now (eg. add the rbac_roles directly to the base.up migration. I'm not strict on this though, and it's just a hunch. Any thoughts?

I think we should try to not break migrations, lest we become comfortable in the habit and it bite us squarely where we sit down the line.

Is anyone running a persistent V2? If we are only running in dev mode, I'll drop all this.
I just didn't want to break any current deployments. But if that's cool, I'm more than happy to drop this code to assign roles in the migration 👍

Apparently we have a dev and QA instance, so this will be required to not totally break those.

Fair enough fair enough

- Remove string type alias, use string - Drop unused funcs - Fix down migration

…ign_role_rebase

- UpdateRoles vs Grant/RemoveRoles - UpdateRoles and UpdateMemberRoles

johnstcn · 2022-04-29T12:20:37Z

coderd/database/migrations/000008_rbac_roles.up.sql

@@ -0,0 +1,18 @@
+ALTER TABLE ONLY users
+	ADD COLUMN IF NOT EXISTS rbac_roles text[] DEFAULT '{}' NOT NULL;


TIL postgres supports native text arrays 😄

johnstcn · 2022-04-29T12:22:39Z

coderd/database/queries/organizationmembers.sql

+	organization_members
+SET
+	-- Remove all duplicates from the roles.
+	roles = ARRAY(SELECT DISTINCT UNNEST(@granted_roles :: text[]))


Neat trick! For posterity here, UNNEST turns an array into a one-row temporary table, SELECT DISTINCT removes duplicates, and then it just becomes an array again.

johnstcn · 2022-04-29T12:23:37Z

coderd/members.go

+func (api *api) putMemberRoles(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.


nit: formatting, also "only be able to change your own roles"?

Yea, since authorize is not yet used, I let you change your roles to w/e you want. This is what we are doing for a lot of user endpoints atm

coderd/members.go

coderd/rbac/builtin.go

johnstcn · 2022-04-29T12:30:40Z

codersdk/users.go

+}
+
+// UpdateOrganizationMemberRoles grants the userID the specified roles in an org.
+// Include ALL roles the user has.


clarification: if a user has roles {A, B, C} and we call UpdateUserRoles with {D}, does this mean the user ends up with just {D} and not {A, B, C, D}? That seems like a sharp edge.

This is correct. I actually had it as grant D before, but if we match the current UI, it actually makes sense to pass {A, B, C, D}.

t

The alternative is to run the diff from the UI as 2 requests.

PUT {D}
DELETE {A, B, C}

I think we can implement this logic better in the BE. I could add a route/query param to "Add" the role vs update for example. I think this is something we can make easier once the UI needs are better understood too. But w/e the end result is, the database call is ok to do an ALL, and let the Golang figure out the diffs.

kylecarbs

The separation of user and organization roles in the API feels really nice. Great work 🥳🥳🥳

…ign_role_rebase

* chore: Rework roles to be expandable by name alone

Emyrk added 11 commits April 27, 2022 15:48

WIP

9d531cb

chore: Rework roles to be expandable by name alone

1eabb07

Add role migrations

dc6010b

Make gen

af7fcf1

Add admin role to the first user

5d47865

Implement builtin roles

d8a5ba1

Add fake for GrantRoles

e8b2e33

Correct indentation on sql

eae626c

Add endpoints to grant new roles to a user

5aba5a2

Currently can't remove roles. So be careful

Make gen after rebase

5b902de

re insert code lost in merge

7a32c38

Emyrk requested a review from a team as a code owner April 27, 2022 21:01

Emyrk mentioned this pull request Apr 27, 2022

feat: Add user roles, but do not yet enforce them #1199

Closed

Emyrk commented Apr 27, 2022

View reviewed changes

Emyrk added 2 commits April 27, 2022 19:24

Appease the linter

3b18317

Make gen

8316deb

Emyrk requested review from kylecarbs and johnstcn April 28, 2022 13:27

Audit log track rbac_roles

7d03554

kylecarbs reviewed Apr 28, 2022

View reviewed changes

Emyrk added 3 commits April 28, 2022 10:39

PR comments

b732d03

- Remove string type alias, use string - Drop unused funcs - Fix down migration

Merge remote-tracking branch 'origin/main' into stevenmasley/user_ass…

4da2c4f

…ign_role_rebase

Add orgids to user response

a77d535

Emyrk requested a review from kylecarbs April 28, 2022 19:45

Emyrk added 3 commits April 28, 2022 15:40

Rename type to use proper casing

32f5dc0

feat: Split org/site roles

2c33a82

- UpdateRoles vs Grant/RemoveRoles - UpdateRoles and UpdateMemberRoles

Add more unit testing

0e3fc29

johnstcn reviewed Apr 29, 2022

View reviewed changes

Update comment

9d47b74

kylecarbs approved these changes Apr 29, 2022

View reviewed changes

johnstcn approved these changes Apr 29, 2022

View reviewed changes

Merge remote-tracking branch 'origin/main' into stevenmasley/user_ass…

96fb498

…ign_role_rebase

Emyrk merged commit 35211e2 into main Apr 29, 2022

Emyrk deleted the stevenmasley/user_assign_role_rebase branch April 29, 2022 14:04

Emyrk mentioned this pull request May 2, 2022

Support assigning and changing users roles in the UI #602

Closed

misskniss added the V2 BETA label May 15, 2022

misskniss added this to the V2 Beta milestone May 15, 2022

kylecarbs pushed a commit that referenced this pull request Jun 10, 2022

feat: Add user roles, but do not yet enforce them (#1200)

260fb6e

* chore: Rework roles to be expandable by name alone

	_, err = member.GrantUserRoles(ctx, codersdk.Me, codersdk.GrantUserRoles{
	Roles: []string{
	rbac.RoleAdmin(),
	rbac.RoleOrgAdmin(first.OrganizationID),
	},

		orgAdmin string = "organization-admin"
		orgMember string = "organization-member"

		@@ -0,0 +1,18 @@
		ALTER TABLE ONLY users
		ADD COLUMN IF NOT EXISTS rbac_roles text[] DEFAULT '{}' NOT NULL;

feat: Add user roles, but do not yet enforce them #1200

feat: Add user roles, but do not yet enforce them #1200

Uh oh!

Conversation

Emyrk commented Apr 27, 2022

What this does

Future Work

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Apr 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

kylecarbs left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

johnstcn Apr 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Emyrk Apr 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kylecarbs left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

codecov bot commented Apr 27, 2022 •

edited

Loading

johnstcn Apr 29, 2022 •

edited

Loading

Emyrk Apr 29, 2022 •

edited

Loading