Rework IsRequestAuthorized to reduce auth scope mismatches #986
Conversation
|
Thinking about this, the right way to do this would be to check authorization on domain objects, rather than on request fields. I'm not ready to do that yet, but that feels like the correct direction. At which point, we might have something like: type DomainObject interface {
GetOrgOwner() int32
GetGroupOwner() int32
// etc
}or possibly simply a method that combined |
| // AuthorizedOnUser checks if the request is authorized for the given | ||
| // user, and returns an error if the request is not authorized. | ||
| func AuthorizedOnUser(ctx context.Context, userId int32) error { | ||
| claims, _ := ctx.Value(auth.TokenInfoKey).(auth.UserClaims) |
There was a problem hiding this comment.
shouldn't we check the second return value here to avoid crashing if someone puts a different type into the auth.TokenInfoKey value?
There was a problem hiding this comment.
I copy-pasted this, but you're right. Added a check.
There was a problem hiding this comment.
... and I replaced these with a proper context accessor which will return a sentinel invalid default value if the key is not in the context.
| return &resp, nil | ||
| } | ||
| return nil, status.Errorf(codes.PermissionDenied, "user is not authorized to access this resource") | ||
| return &resp, nil |
There was a problem hiding this comment.
I actually moved the auth check earlier in the code.
There was a problem hiding this comment.
yes, but I meant returning resp, nil (the happy path) in the outermost indent level, not nested. I like that.
| if !IsRequestAuthorized(ctx, req.GroupId) { | ||
| return nil, status.Errorf(codes.PermissionDenied, "user is not authorized to access this resource") | ||
| if err := AuthorizedOnGroup(ctx, req.GroupId); err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| // if we do not have a group, check if we can infer it |
There was a problem hiding this comment.
Do you think we could move the infer of the default group to before the authorisation decision so that the user doesn't have to call enroll (which is the typical caller) while specifying the group ID explicitly?
There was a problem hiding this comment.
We can just move the AuthorizedOnGroup check after the defaulting. Done.
evankanderson
force-pushed
the
auth-error
branch
from
ed15bd6
to
f4e4ab4
Compare
|
oops, a conflict. I'll just robot-approve when you push a resolved version. |
|
Merging on Evan's behalf because there is quite a few patches in flight and we risk even more conflicts |
It would be nice to be able to tie the auth scope from the proto more fully into the resource model, but sometimes the model comes from database lookups, and other times directly from the proto without any database mediation, so I ended up with an Error-level log if we use one of the constructs wrong.
Please point out if you see a way to do these better. I also tried not to change any semantics where the previous semantic intent was obvious by either proto or
IsRequestAuthorizedinvocations.