NotificationsYou must be signed in to change notification settings
Fork928
Star10.1k

feat: Implement list roles & enforce authorize examples#1273

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Merged

Emyrk merged 16 commits intomainfromstevenmasley/list_roles

May 3, 2022

Merged

feat: Implement list roles & enforce authorize examples#1273

Emyrk merged 16 commits intomainfromstevenmasley/list_roles

May 3, 2022

Conversation

Copy link

Member

Emyrk commentedMay 3, 2022•
edited
Loading

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

Emyrk added5 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

Copy link

codecovbot commentedMay 3, 2022•
edited
Loading

Codecov Report

Merging#1273 (c86c67c) intomain (e530ab2) willincrease coverage by0.11%.
The diff coverage is76.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%)`	⬇️
... and8 more

Continue to review full report at Codecov.

Legend -Click here to learn more
Δ = absolute <relative> (impact),ø = not affected,? = missing data
Powered byCodecov. Last updatee530ab2...c86c67c. Read thecomment docs.

kylecarbs reviewed

May 3, 2022

View reviewed changes

coderd/coderd.go

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

Copy link

Member

kylecarbsMay 3, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

Copy link

MemberAuthor

EmyrkMay 3, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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 fromOptions

Copy link

MemberAuthor

EmyrkMay 3, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

Copy link

Member

kylecarbsMay 3, 2022•
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

This could go onapi instead, that way tests couldn't mistakenly pass it viaOptions.

Copy link

MemberAuthor

EmyrkMay 3, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

coder/coderd/coderd.go

Lines 333 to 338 indb04d67

	typeapistruct {
	*Options

	websocketWaitMutex sync.Mutex
	websocketWaitGroup sync.WaitGroup
	}

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

Copy link

Member

kylecarbsMay 3, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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 OutdatedShow resolvedHide resolved

coderd/rbac/builtin.go OutdatedShow resolvedHide resolved

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 {

Copy link

Member

kylecarbsMay 3, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

What do you think about renaming this toEnforceRBAC?

Copy link

MemberAuthor

EmyrkMay 3, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

I thinkEnforceRBAC is also weak though. I was thinking the packagehttpmw provides enough context, and it does do the actionAuthorize().

Authorize is the correct word for what is happening, as it's not authentication. I feelEnforceRBAC doesn't indicate theobject andaction are included.

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

Copy link

Member

kylecarbsMay 3, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

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

Copy link

MemberAuthor

EmyrkMay 3, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

Copy link

Member

kylecarbsMay 3, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Agreed agreed

coderd/httpmw/authorize.go OutdatedShow resolvedHide resolved

codersdk/roles.goShow resolvedHide resolved

Emyrk added11 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 fromkylecarbs

May 3, 2022 19:50

kylecarbs approved these changes

May 3, 2022

View reviewed changes

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 {

Copy link

Member

kylecarbsMay 3, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

It might be confusing that this is calledWithRBACObject, but the values arerbac.ResourceX. It could be helpful to make these the same names!

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 {

Copy link

Member

kylecarbsMay 3, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Agreed agreed

Emyrk merged commitd0293e4 intomain

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 theV2 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

Labels

None yet

3 participants

Movatterモバイル変換

feat: Implement list roles & enforce authorize examples#1273

feat: Implement list roles & enforce authorize examples#1273

Uh oh!

Conversation

Emyrk commentedMay 3, 2022• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Uh oh!

codecovbot commentedMay 3, 2022• editedLoading 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!

kylecarbsMay 3, 2022• editedLoading 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 commentedMay 3, 2022•
edited
Loading

codecovbot commentedMay 3, 2022•
edited
Loading

kylecarbsMay 3, 2022•
edited
Loading