Use proto options to store auth config #894
Conversation
|
(This was me playing around the other evening, but let's not merge before demos are done) |
|
Let's get back to this next week. So far I really like it! |
pkg/controlplane/handlers_authz.go
Outdated
| if !noLogMethods.Has(info.FullMethod) { | ||
| opts, err := optionsForMethod(info) | ||
| if err != nil { | ||
| // Another option: log and continue with default RPC options |
There was a problem hiding this comment.
this doesn't seem to log and continue, instead it issues an error.
There was a problem hiding this comment.
This was a "what should we do" question -- I erred on the side of failing closed, but that means that RPCs without options may not be callable (I haven't tested this by hand yet).
There was a problem hiding this comment.
I think having RPCs without options fail is fine as long as we log it in the server and make it clear that this is the case.
There was a problem hiding this comment.
Updated the comment.
|
CC @eleftherias who was looking into auth in general (not asking you for review per se unless you feel like it, just thought you'd find this interesting) |
evankanderson
changed the title
pkg/controlplane/handlers_authz.go
Outdated
| if !noLogMethods.Has(info.FullMethod) { | ||
| opts, err := optionsForMethod(info) | ||
| if err != nil { | ||
| // Another option: log and continue with default RPC options |
There was a problem hiding this comment.
I think having RPCs without options fail is fine as long as we log it in the server and make it clear that this is the case.
| @@ -40,6 +64,10 @@ service ArtifactService { | |||
| option (google.api.http) = { | |||
| get: "/api/v1/artifacts/{provider}" | |||
| }; | |||
|
|
|||
| option (rpc_options) = { | |||
| root_admin_only: true | |||
There was a problem hiding this comment.
Should this be owner_only? Owners should be able to list artifacts for their groups. This doesn't actually list all artifacts. I guess we also need to group scope this.
There was a problem hiding this comment.
I was going based on the inclusion in superAdminMethods here
I agree that it doesn't seem right, but the goal here was to do a faithful translation, then fix things that seemed wrong.
I'll add a TODO.
|
|
||
| option (rpc_options) = { | ||
| auth_scope: OBJECT_OWNER_ORGANIZATION | ||
| owner_only: true |
There was a problem hiding this comment.
@jhrozek I see this is currently a global fetching of artifacts; shouldn't this be a group-scoped thing instead?
There was a problem hiding this comment.
At the moment, artifacts are always linked to repositories which AFAIK should be group-scoped. Would us supporting artifacts that come from a registry not linked to a repository make this any different? I guess not, we would still consider the registry akin to e.g. a build configuration which would be group scoped.
(the above was me thinking out loud, but in short I agree that artifacts should be group-scoped)
There was a problem hiding this comment.
This is for GetAuthorizationURL, which again is copied from resourceAuthorizations
I'll add another TODO to re-examine this
There was a problem hiding this comment.
sounds good to me, thanks @evankanderson !
| @@ -180,6 +221,10 @@ service OAuthService { | |||
| post: "/api/v1/auth/{provider}/revoke" | |||
| body: "*" | |||
| }; | |||
|
|
|||
| option (rpc_options) = { | |||
| root_admin_only: true | |||
There was a problem hiding this comment.
should this be a group owner operation instead?
There was a problem hiding this comment.
This was in superAdminMethods previously.
| @@ -40,6 +64,10 @@ service ArtifactService { | |||
| option (google.api.http) = { | |||
| get: "/api/v1/artifacts/{provider}" | |||
| }; | |||
|
|
|||
| option (rpc_options) = { | |||
| root_admin_only: true | |||
|
|
||
| option (rpc_options) = { | ||
| auth_scope: OBJECT_OWNER_ORGANIZATION | ||
| owner_only: true |
There was a problem hiding this comment.
sounds good to me, thanks @evankanderson !
Fixes #590
I was touching
handler_authz.goand realized I sort of wanted to give this a spin.Don't worry about reviewing before Monday. A couple notes:
handler_authz.goIsRequestAuthorizedis intended to be a straight port -- I'm suspicious of OBJECT_OWNER_USER, for example.