-
Notifications
You must be signed in to change notification settings - Fork 935
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Combine User
and UserTokenContext
for user-backed identities in requests
#15757
base: main
Are you sure you want to change the base?
Combine User
and UserTokenContext
for user-backed identities in requests
#15757
Conversation
@@ -104,7 +104,7 @@ class RequestUser(Caveat): | |||
user_id: StrictStr | |||
|
|||
def verify(self, request: Request, context: Any, permission: str) -> Result: | |||
if not isinstance(request.identity, UserTokenContext): | |||
if not isinstance(request.identity, UserContext): | |||
return Failure("token with user restriction without a user") | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should there be a new extra check here?:
if request.identity.macaroon is None:
return Failure("token with user restriction without a macaroon")
or will that never happen?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this can't happen, since the only place we end up checking the caveats is in MacaroonSecurityPolicy.permits
, where request.identity
will invariably either be an PublisherTokenContext
or a UserContext
that always has its macaroon set.
That being said, I see no harm in it as a sanity check either 🙂.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added!
User
and UserTokenContext
for user-backed identities
User
and UserTokenContext
for user-backed identitiesUser
and UserTokenContext
for user-backed identities in requests
45cf7cb
to
4cc055f
Compare
We have three types that a request.identity can be: - `User`: when the identity is a user backed by a login session - `UserTokenContext`: when the identity is a user backed by an API token (i.e. macaroon) - `PublisherTokenContext`: when the identity is an OIDCPublisher backed by an API token Of these, User and UserTokenContext are confusable and prone to error. This change collapses them into a single UserContext type.
4cc055f
to
d3bf7f2
Compare
Fixes #15748.
We have three types that a
request.identity
can be:User
: when the identity is a user backed by a login sessionUserTokenContext
: when the identity is a user backed by an API token (i.e. macaroon)PublisherTokenContext
: when the identity is anOIDCPublisher
backed by an API tokenOf these,
User
andUserTokenContext
are confusable and prone to error.This PR collapses them into a single
UserContext
type.cc @woodruffw @di