feat: Rbac more coderd endpoints, unit test to confirm #1437

Emyrk · 2022-05-13T18:58:11Z

Want to confirm on the right path, it's just tedious work I'd rather get some checkpoints in.

What this does

This adds rbac authorize checks to many more endpoints that require authorize checks.

Authorize checks should happen in each handler. I wrote a unit test that ensures all endpoints call authorize. It does not check the permissions are set correctly, but it at least ensures we intentionally call authorize. For now, I put skips in the test for endpoints I did not authorize yet

Future work

Make it easy to test various types of users against each endpoint to test proper auth checks.
E.g: A member cannot create a new user

- Make 'request()' exported for running custom requests

…points

Skip workspace agent endpoints

- Rename authObject - Shorten 'AuthorizeByRoleName'

…points

codecov · 2022-05-13T19:01:59Z

Codecov Report

Merging #1437 (452c72d) into main (4ab7a41) will increase coverage by 8.08%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main    #1437      +/-   ##
==========================================
+ Coverage   67.18%   75.26%   +8.08%     
==========================================
  Files         287      130     -157     
  Lines       19331     1965   -17366     
  Branches      244      244              
==========================================
- Hits        12987     1479   -11508     
+ Misses       5004      420    -4584     
+ Partials     1340       66    -1274

Flag	Coverage Δ
unittest-go-macos-latest	`?`
unittest-go-postgres-	`?`
unittest-go-ubuntu-latest	`?`
unittest-go-windows-2022	`?`
unittest-js	`75.26% <ø> (ø)`

Impacted Files	Coverage Δ
coderd/coderd.go
coderd/coderdtest/coderdtest.go
coderd/httpmw/authorize.go
coderd/httpmw/oauth2.go
coderd/rbac/authz.go
coderd/rbac/builtin.go
coderd/rbac/object.go
coderd/roles.go
coderd/users.go
codersdk/buildinfo.go
... and 147 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 4ab7a41...452c72d. Read the comment docs.

…points_per_handler

johnstcn · 2022-05-16T15:37:58Z

coderd/authorize.go

+		})
+
+		// Log the errors for debugging
+		internalError := new(rbac.UnauthorizedError)


minor: should we just define a single var rbac.UnauthorizedError and use that instead in the check below?

Not sure what you mean?

johnstcn · 2022-05-16T15:39:01Z

coderd/rbac/builtin.go

@@ -64,6 +64,11 @@ var (
 			return Role{
 				Name:        member,
 				DisplayName: "Member",
+				Site: permissions(map[Object][]Action{
+					// TODO: @emyrk in EE we should restrict this to only certain fields.


minor: might move this into an issue instead of a discussion in a soon-to-be-public repo?

I'll just drop the comment.
#1439

spikecurtis

I'm still overall concerned that we don't have hardly any unit tests that assert the correctness of the Authz call, rather than just asserting that it occurs. Do we need to raise an issue to track because of the time pressure around the community MVP?

coderd/organizations.go

spikecurtis · 2022-05-16T17:12:32Z

coderd/organizations.go

+	allowed := make([]database.Workspace, 0)
+	for i := range workspaces {
+		w := workspaces[i]
+		err := api.Authorizer.ByRoleName(r.Context(), roles.ID.String(), roles.Roles, rbac.ActionRead,


on workspacesByOrganization we did one RBAC check for all results. But here, we're querying the database first then doing an RBAC check workspace-by-workspace. Why is this different?

I detailed my logic for workspacesByOrganization above.

I can't do what I did for workspacesByOrganization as the workspace's organization is important. There is no general call to know if I can read "all workspaces by this user".

I should probably do the call per workspace in workspacesByOrganization

coderd/organizations.go

coderd/rbac/object.go

spikecurtis · 2022-05-16T18:22:21Z

coderd/users.go

+	if !api.Authorize(rw, r, rbac.ActionRead,
+		rbac.ResourceOrganization.
+			InOrg(organization.ID).
+			WithID(organization.ID.String())) {


if the organization exists and the caller has access, this will return the organization regardless of whether the requested user is a member, which I don't think is what we want. Like, this is organizationByUserAndName and you're ignoring the User part of it.

You think if you are not a member, this should return 403?

Honestly I don't really get the point of this endpoint. Why would we select an organization by organization name and by username? Grabbing an org by username doesn't make sense to me 🤔

Same, unless you're actually trying to test membership, in which case we should have a much clearer name.

coderd/users.go

spikecurtis · 2022-05-16T18:31:26Z

coderd/coderd_test.go

+}
+
+type fakeAuthorizer struct {
+	Called       *authCall


you only capture the most recent authorization call, but many endpoints call this multiple times with different objects. This should be captured and asserted in testing.

Yes this is true. Most cases though the first call returns an error and the request terminates.

As I add more endpoints and see more cases, I should increase the assertions. I intend to follow this PR up with more testing and endpoints. I just want to be on the right path first.

codersdk/client.go

Emyrk · 2022-05-16T19:00:21Z

I'm still overall concerned that we don't have hardly any unit tests that assert the correctness of the Authz call, rather than just asserting that it occurs. Do we need to raise an issue to track because of the time pressure around the community MVP?

I agree with you here. I think this should be a follow up. I imagine we have to rework all of our api tests to run as multiple users with varying roles.

Right now most unit tests are run with the first user. We should run them with the first user, an admin, a member, etc.

This is just the first step.

Fix comments

spikecurtis · 2022-05-16T20:22:57Z

I agree with you here. I think this should be a follow up. I imagine we have to rework all of our api tests to run as multiple users with varying roles.

Right now most unit tests are run with the first user. We should run them with the first user, an admin, a member, etc.

Testing the API with different users, roles, and requests is going to be a combinatorial explosion. I think we want to make the actual access control decisions orthogonal to the API tests. In particular, a testing strategy I recommend is to call the APIs and assert correct {subject, role, action, object} are passed to the RBAC. This needs to be done for each endpoint, but probably only needs to be done for a single user.

Then, we have separate tests that assert that particular {subject, role, action, object} cases result in the correct authz decision. These tests are independent of endpoint.

Emyrk · 2022-05-16T20:25:38Z

I agree with you here. I think this should be a follow up. I imagine we have to rework all of our api tests to run as multiple users with varying roles.
Right now most unit tests are run with the first user. We should run them with the first user, an admin, a member, etc.

Testing the API with different users, roles, and requests is going to be a combinatorial explosion. I think we want to make the actual access control decisions orthogonal to the API tests. In particular, a testing strategy I recommend is to call the APIs and assert correct {subject, role, action, object} are passed to the RBAC. This needs to be done for each endpoint, but probably only needs to be done for a single user.

Then, we have separate tests that assert that particular {subject, role, action, object} cases result in the correct authz decision. These tests are independent of endpoint.

Ah I see. Are you ok to punt that to the next PR? I just want to get something agreed upon here, and then I can dive deeper. I know I need to improve the actual assertions.

There is still going to be a large amount of combinations to be done, but I do agree it'll be easier to do it the way you mentioned.

spikecurtis · 2022-05-16T20:38:49Z

I'm getting a little nervous because there are several examples of API actions that actually map to multiple {action, object} tuples. This makes the RBAC system hard to understand and reason about when there isn't an obvious mapping.

For example, assigning a role maps to {update, user} AND {create, user_role}. We can document around this, but my experience in K8s is that RBAC is consistently cited as the one of the hardest parts of the API to make sense of, and things like this make it harder.

spikecurtis · 2022-05-16T20:39:21Z

Ah I see. Are you ok to punt that to the next PR? I just want to get something agreed upon here, and then I can dive deeper. I know I need to improve the actual assertions.

Yes, ok to punt to next PR

Emyrk · 2022-05-16T21:51:47Z

I'm getting a little nervous because there are several examples of API actions that actually map to multiple {action, object} tuples. This makes the RBAC system hard to understand and reason about when there isn't an obvious mapping.

For example, assigning a role maps to {update, user} AND {create, user_role}. We can document around this, but my experience in K8s is that RBAC is consistently cited as the one of the hardest parts of the API to make sense of, and things like this make it harder.

That is a good point. With the change to AssignRole, I could make it just 1 rbac check.

Maybe if things are being represented as 2 actions, it means there is a mistake in either the resource or the api call. The other case this happens is createUser because right now we can also create an organization when making a user. Which seems kinda strange and maybe just a fault of the api. We could split this into 2 api calls.

…points_per_handler

spikecurtis · 2022-05-16T23:21:58Z

That is a good point. With the change to AssignRole, I could make it just 1 rbac check.

That would mean anyone with the Create AssignRole permission can give that role to any user (possibly scoped by an org for an org role?) --- this is just equivalent to dropping the {update, user} requirement in your current implementation.

That sounds fine to me, since I can't think of any other useful scoping of the power to assign a specific role.

Emyrk · 2022-05-16T23:45:44Z

Correct. If you have the AssignRole you can assign any roles. I imagine we will scope org roles yes.

I had an idea on how to select which roles can be granted by which roles. I called it "Roles as a resource" in the RBAC RFC (at the bottom).

For now since we only have "admin" and "member", I figure we just have the all en-composing resource for now for assigning all roles in the given scope (site or org)

* feat: Enforce authorize call on all endpoints - Make 'request()' exported for running custom requests * Rbac users endpoints * 401 -> 403

Emyrk added 15 commits May 11, 2022 16:46

feat: Enforce authorize call on all endpoints

bed0f8f

- Make 'request()' exported for running custom requests

Add more endpoints to the unit test

af6dc5f

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

01b2c94

…points

Rbac users endpoints

be5b0b3

Skip workspace agent endpoints

Make test pass by skipping missed endpoints

970e345

Fix broken tests

945e9fa

Import order

fdfef88

PR comment fixes

89a3678

- Rename authObject - Shorten 'AuthorizeByRoleName'

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

29da9aa

…points

omit another endpoint

63727e0

Cleanup comments

96a5727

Do not leak if an organization name exists

4b6c9b0

Update comment

cd2fda7

feat: Implement authorize for each endpoint

62ec87e

Authorize per endpoint

452c72d

misskniss added the V2 BETA label May 15, 2022

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

Emyrk mentioned this pull request May 16, 2022

feat: Rbac more coderd endpoints, unit test to confirm #1406

Closed

Emyrk marked this pull request as draft May 16, 2022 12:26

Emyrk added 4 commits May 16, 2022 07:26

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

307f6c0

…points_per_handler

feat: Move all authorize calls into each handler

32af1e6

Delete unused code

28a099f

feat: Add some perms to users

ff7bd81

Emyrk marked this pull request as ready for review May 16, 2022 13:52

johnstcn approved these changes May 16, 2022

View reviewed changes

Drop comment

d123b9f

spikecurtis requested changes May 16, 2022

View reviewed changes

Fix 401 -> 403

186eb5f

Fix comments

Emyrk added 3 commits May 16, 2022 14:26

Fix using User over UserData

5d32d9d

Rename UserRole to RoleAssignment

301d42a

Refactor workspacesByUser

a989224

misskniss modified the milestones: V2 Beta, Community MVP May 16, 2022

misskniss removed the V2 BETA label May 16, 2022

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

ed9be78

…points_per_handler

Fix some routes

1047391

Emyrk added 4 commits May 16, 2022 18:47

Drop update User auth check from assign roles

7ad069e

Correct unit tests

e68fdbf

Unit tests use 403 vs 401

1a4e7e1

401 -> 403 in unit test

f250fbe

spikecurtis approved these changes May 17, 2022

View reviewed changes

Emyrk merged commit 4ad5ac2 into main May 17, 2022

Emyrk deleted the stevenmasley/rbac_endpoints_per_handler branch May 17, 2022 18:43

misskniss mentioned this pull request May 20, 2022

UI: users page requires roles to view #1439

Closed

misskniss added the Community MVP 🏁 label Jun 3, 2022

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

feat: Rbac more coderd endpoints, unit test to confirm (#1437)

6752f13

* feat: Enforce authorize call on all endpoints - Make 'request()' exported for running custom requests * Rbac users endpoints * 401 -> 403

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Rbac more coderd endpoints, unit test to confirm #1437

feat: Rbac more coderd endpoints, unit test to confirm #1437

Emyrk commented May 13, 2022 •

edited

codecov bot commented May 13, 2022

johnstcn May 16, 2022

Emyrk May 16, 2022

johnstcn May 16, 2022

Emyrk May 16, 2022

spikecurtis left a comment

spikecurtis May 16, 2022

Emyrk May 16, 2022

spikecurtis May 16, 2022

Emyrk May 16, 2022

spikecurtis May 17, 2022

spikecurtis May 16, 2022

Emyrk May 16, 2022

Emyrk commented May 16, 2022

spikecurtis commented May 16, 2022

Emyrk commented May 16, 2022

spikecurtis commented May 16, 2022

spikecurtis commented May 16, 2022

Emyrk commented May 16, 2022

spikecurtis commented May 16, 2022

Emyrk commented May 16, 2022

feat: Rbac more coderd endpoints, unit test to confirm #1437

feat: Rbac more coderd endpoints, unit test to confirm #1437

Conversation

Emyrk commented May 13, 2022 • edited

What this does

Future work

codecov bot commented May 13, 2022

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spikecurtis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Emyrk commented May 16, 2022

spikecurtis commented May 16, 2022

Emyrk commented May 16, 2022

spikecurtis commented May 16, 2022

spikecurtis commented May 16, 2022

Emyrk commented May 16, 2022

spikecurtis commented May 16, 2022

Emyrk commented May 16, 2022

Emyrk commented May 13, 2022 •

edited