feat: Implement list roles & enforce authorize examples #1273

Emyrk · 2022-05-03T14:34:56Z

This is the first step to enforcing rbac roles for our endpoints. This also adds the endpoints to list the roles available to assign.

Authorize can be changed dynamically with middlewares.

codecov · 2022-05-03T15:11:58Z

Codecov Report

Merging #1273 (c86c67c) into main (e530ab2) will increase coverage by 0.11%.
The diff coverage is 76.72%.

@@            Coverage Diff             @@
##             main    #1273      +/-   ##
==========================================
+ Coverage   65.96%   66.07%   +0.11%     
==========================================
  Files         277      280       +3     
  Lines       18230    18384     +154     
  Branches      216      216              
==========================================
+ Hits        12025    12147     +122     
- Misses       4956     4973      +17     
- Partials     1249     1264      +15

Flag	Coverage Δ
unittest-go-macos-latest	`53.49% <73.58%> (+0.26%)`	⬆️
unittest-go-postgres-	`64.89% <76.72%> (+0.02%)`	⬆️
unittest-go-ubuntu-latest	`55.99% <73.58%> (+0.31%)`	⬆️
unittest-go-windows-2022	`51.92% <73.58%> (+0.27%)`	⬆️
unittest-js	`70.69% <ø> (ø)`

Impacted Files	Coverage Δ
coderd/rbac/object.go	`100.00% <ø> (ø)`
coderd/httpmw/authorize.go	`68.18% <68.18%> (ø)`
codersdk/roles.go	`72.72% <72.72%> (ø)`
coderd/rbac/builtin.go	`92.42% <75.00%> (-3.88%)`	⬇️
coderd/coderd.go	`94.44% <87.09%> (-1.14%)`	⬇️
coderd/database/queries.sql.go	`78.10% <100.00%> (+0.06%)`	⬆️
coderd/roles.go	`100.00% <100.00%> (ø)`
coderd/workspaceagents.go	`56.04% <0.00%> (-2.80%)`	⬇️
provisionerd/provisionerd.go	`76.17% <0.00%> (-0.54%)`	⬇️
coderd/provisionerdaemons.go	`62.98% <0.00%> (-0.51%)`	⬇️
... and 8 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 e530ab2...c86c67c. Read the comment docs.

kylecarbs · 2022-05-03T15:09:31Z

coderd/coderd.go

@@ -48,6 +50,7 @@ type Options struct {
 	SecureAuthCookie     bool
 	SSHKeygenAlgorithm   gitsshkey.Algorithm
 	TURNServer           *turnconn.Server
+	Authorizer           *rbac.RegoAuthorizer


Does this need to be exposed via options? It doesn't seem to be used outside of here.

I was thinking of making it an interface that is "denyall" or something to trigger in unit tests. But for now, we don't, so I'll drop it from Options

Ah wait yes. @kylecarbs this will be needed if you decide to do the rbac Authorize() check manually inside the function, instead of the middleware. So this is required.

This could go on api instead, that way tests couldn't mistakenly pass it via Options.

Right now nothing else is on the api only like that.

coder/coderd/coderd.go

Lines 333 to 338 in db04d67

type api struct {

*Options

websocketWaitMutex sync.Mutex

websocketWaitGroup sync.WaitGroup

}

I don't think it'd be bad to pass one in via options 🤷‍♂️

Oh my bad, I thought there was precedent for this 🤦

If you feel like it's fine I'll defer, but I generally think it's hasty to expose a value unless it needs to be used elsewhere. It's really easy to expose a parameter, but much harder to hide one.

codersdk/roles.go

coderd/rbac/builtin.go

kylecarbs · 2022-05-03T15:37:13Z

coderd/httpmw/authorize.go

+// Authorize will enforce if the user roles can complete the action on the AuthObject.
+// The organization and owner are found using the ExtractOrganization and
+// ExtractUser middleware if present.
+func Authorize(logger slog.Logger, auth *rbac.RegoAuthorizer, action rbac.Action) func(http.Handler) http.Handler {


Authorize feels like action, not something that would return a handler.

What do you think about renaming this to EnforceRBAC?

I think EnforceRBAC is also weak though. I was thinking the package httpmw provides enough context, and it does do the action Authorize().

Authorize is the correct word for what is happening, as it's not authentication. I feel EnforceRBAC doesn't indicate the object and action are included.

Another word that comes to mind is "Access". Idk, EnforceAccess, EnforcePermissions. Maybe EnforceRBAC isn't that bad, just felt odd to me at first.

Fair enough. I'm primarily trying to display that the RBAC package is being leveraged when calling this handle. Enforce is a bit sketchy too.

While it is authorizing, I'm nervous that this will get conflated with authentication really easily.

yea this is classic authorization vs authentication. If you aren't familiar with it, it's easy to mix up.

Agreed agreed

coderd/httpmw/authorize.go

codersdk/roles.go

kylecarbs · 2022-05-03T19:19:29Z

coderd/httpmw/authorize.go

+
+// WithRBACObject sets the object for 'Authorize()' for all routes handled
+// by this middleware. The important field to set is 'Type'
+func WithRBACObject(object rbac.Object) func(http.Handler) http.Handler {


It might be confusing that this is called WithRBACObject, but the values are rbac.ResourceX. It could be helpful to make these the same names!

kylecarbs · 2022-05-03T19:20:25Z

coderd/httpmw/authorize.go

+// Authorize will enforce if the user roles can complete the action on the AuthObject.
+// The organization and owner are found using the ExtractOrganization and
+// ExtractUser middleware if present.
+func Authorize(logger slog.Logger, auth *rbac.RegoAuthorizer, action rbac.Action) func(http.Handler) http.Handler {


Agreed agreed

Emyrk added 5 commits May 2, 2022 23:12

feat: First draft of adding authorize to an http endpoint

30e2031

WIP: Using middleware to change auth object params

2161f84

Authorize can be changed dynamically with middlewares.

feat: Implement basic authorize and unit test

54bc054

Some cleanup

d083a7c

Merge remote-tracking branch 'origin/main' into stevenmasley/list_roles

95b9a14

kylecarbs reviewed May 3, 2022

View reviewed changes

Emyrk added 11 commits May 3, 2022 11:04

Expand 'orgs' to 'organizations' in func namings

1498dcd

Renamings

f36ae37

Use rbac.object directly

b831260

Fix broken tests

db04d67

Add some comments

b76f373

Linting

117f838

Handle out of order lists

42b42ab

Add unit test

0efe72c

Add unit test for mw

dba617d

parallel unit test

190940f

style order

c86c67c

Emyrk requested a review from kylecarbs May 3, 2022 19:50

kylecarbs approved these changes May 3, 2022

View reviewed changes

Emyrk merged commit d0293e4 into main May 3, 2022

Emyrk deleted the stevenmasley/list_roles branch May 3, 2022 21:10

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: Implement list roles & enforce authorize examples (#1273)

3371441

	type api struct {
	*Options

	websocketWaitMutex sync.Mutex
	websocketWaitGroup sync.WaitGroup
	}

feat: Implement list roles & enforce authorize examples #1273

feat: Implement list roles & enforce authorize examples #1273

Uh oh!

Conversation

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

Uh oh!

Uh oh!

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

Uh oh!

Codecov Report

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!

kylecarbs May 3, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

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!

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!

Uh oh!

Emyrk commented May 3, 2022 •

edited

Loading

codecov bot commented May 3, 2022 •

edited

Loading

kylecarbs May 3, 2022 •

edited

Loading