Refactor request authentication to remove duplication #3302
Conversation
dnephin
requested review from
ssoroka,
BruceMacD,
jmorganca and
pdevine
as code owners
| tx, err := srv.db.Begin(c.Request.Context()) | ||
| if err != nil { | ||
| return access.Authenticated{}, err | ||
| } | ||
| defer logError(tx.Rollback, "failed to rollback middleware transaction") | ||
|
|
||
| authned, err := requireAccessKey(c, tx, srv) | ||
| if err != nil { | ||
| if !route.authenticationOptional && err != nil { | ||
| return authned, err |
There was a problem hiding this comment.
We could be suppressing unexpected errors here in the case of an "authentication optional" route
There was a problem hiding this comment.
Yes, true. We were doing that before this change as well with authned, _ := requireAccessKey(...). I think either the error is an authentication failed error (which we do want to ignore), or it's some kind of operational error like the database not being available. In the case of an operational error I would expect the next operation to fail in the same way, so we end up not missing anything.
| // https://github.com/infrahq/infra/blob/main/docs/dev/organization-request-flow.md | ||
|
|
||
| // TODO: use an explicit setting for this, don't overload EnableSignup | ||
| if org == nil && !srv.options.EnableSignup { // is single tenant |
There was a problem hiding this comment.
I've been generally hesitant to go down the "single-tenant" path versus everything being multi-tenant but with sign-up disabled. Is there anything stopping us from assuming the default org in any case the org in nil? Seems correct to me, then we can move to request failure once existing sessions for single-tenant deployments have expired.
There was a problem hiding this comment.
Oh, looks like this was existing logic, ignore this comment for now.
There was a problem hiding this comment.
I would be fine with serving the default org in all cases. We discussed the options a bunch before and this was what we ended up agreeing on.
| } | ||
| } | ||
|
|
||
| if authned.User != nil { |
There was a problem hiding this comment.
Is this check to see if authentication was provided or is it actually looking for the user?
There was a problem hiding this comment.
It is checking to see if authentication was provided, but in handleInfraDestinationHeader which is called below here we also need to user to perform the authorization. So I guess it's both?
dnephin
force-pushed
the
dnephin/api-separate-transaction-for-middleware
branch
from
b697a95
to
ee92dec
Compare
dnephin
force-pushed
the
dnephin/authorize-using-request-context
branch
2 times, most recently
from
f20cdf1
to
28d353f
Compare
dnephin
force-pushed
the
dnephin/authorize-using-request-context
branch
from
28d353f
to
c63f4c4
Compare
that uses RequestContext instead of gin.Context. This allows us to start migrating access functions to the new strongly typed context without having to do them all at once.
These two functions were performing very similar operations, one for routes requiring authentication, and one for optional authentication. This PR combines them to remove the duplication. It also extracts a routeSettings struct so that the settings can be passed around without needing the generic parameters on route. Also renames a couple of the settings to be more obvious.
dnephin
force-pushed
the
dnephin/authorize-using-request-context
branch
from
c63f4c4
to
8420901
Compare
Summary
Branched from #3299
This PR resolves a TODO from #3299, and combines the two very similar functions we were using as request "middleware".
Best viewed by individual commit.
To accomplish this I had to make a few changes:
gin.Contextthat contains aRequestContext. We already had aCanmethod that was only used as part of the implementation ofRequireInfraRole. I repurposed that unused function as the newAuthorizefunction which accepts aRequestContextdirectly.route, but it contained generic parameters for the route handler. I extracted arouteSettingsstruct that we can pass around to functions fromwrapRoute, that removes the need to parameterize things that don't call the handler.Finally this allowed me to combine two very similar functions (
authenticateRequest, andvalidateRequestOrganization) into a single function and move theRequestContexttowrapRoute, so that we don't have to set it fromgin.Context. This brings us one step closer to having our routes accept theRequestContextdirectly (instead of acceptinggin.Context).