[API-49] Give correct access based on self id and grants#63
[API-49] Give correct access based on self id and grants#63raymondjacobson merged 4 commits intomainfrom
Conversation
schottra
left a comment
schottra
left a comment
There was a problem hiding this comment.
This works but passing the function around does feel a little messy. I had the thought that we could just change the way auth middleware and context works so that:
- If you pass a user_id and a wallet, we require the wallet to either be the user or have a grant for the user. In the latter case, if you don't have a grant, we just throw a 403 immediately (in other words, you aren't allowed to make a request with a contextual user you don't have permission to be)
- Just use myID/userId in the handler and trust that it's been validated before we hit the handler (then we don't need to call the isAuthorizedRequest handler manually)
I could be misunderstanding how myId works (and also I'm not sure what the difference is between that and userId in our Context object)
|
|
||
| // If you can download it, you can stream it | ||
| streamAccess := downloadAccess || q.GetTrackAccess(ctx, arg.MyID.(int32), track.StreamConditions, &track, &user) | ||
| streamAccess := downloadAccess || q.GetTrackAccess( |
There was a problem hiding this comment.
Think maybe a wee littl test on the endpoint for these cases? :-)
| } | ||
|
|
||
| // I always have access to my own content | ||
| if authedUserId != 0 && authedUserId == myId { |
There was a problem hiding this comment.
Why does authedUserId matching mean the user owns the track?
There was a problem hiding this comment.
^ LOL good catch. need to check this against the owner not myId... ty
|
Ok change this PR - authMiddleware:
requiresAuthMiddleware:
And added new tests! |
| userId, wallet := app.recoverAuthorityFromSignatureHeaders(c) | ||
| c.Locals("authedUserId", userId) | ||
| c.Locals("authedWallet", wallet) | ||
| fmt.Println("authMiddleware", userId, wallet) |
There was a problem hiding this comment.
logger? or is this for debugging
| // - the user is not authorized to act on behalf of "requestedWallet" | ||
| func (app *ApiServer) authMiddleware(c *fiber.Ctx) error { | ||
| userId, wallet := app.recoverAuthorityFromSignatureHeaders(c) | ||
| c.Locals("authedUserId", userId) |
There was a problem hiding this comment.
I don't think this fn cares at all about authedUserId by the looks of it - it really only cares that the wallet recovered has a grant or is the wallet of the user with userId = myId
maybe we can avoid fetching the user ID on every request then, and add a clause to the isAuthorizedRequest query to check if there's a row for userId <=> wallet.
then we can remove authedUserId from the context entirely.
wdyt?
(requireAuthMiddleware can do the query still, since it explicitly wants a user ID, but that's only used in one route)
There was a problem hiding this comment.
I like what you're thinking here.
I think the query can be done in a single union all. let me merge this first and open another PR
3 participants
This PR fixes 3 issues with access:
This feels a bit prop drill-y but I couldn't easily come up with something better.
Would be great to get feedback.