-
Notifications
You must be signed in to change notification settings - Fork 298
Include principal name in Polaris tokens #2389
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
base: main
Are you sure you want to change the base?
Conversation
Summary of changes: - Instead of including the principal id twice in the token, the principal name is now used as the subject claim. While the default authenticator doesn't need the principal name and works with just the principal id, not having the "real" principal name available could be a problem for other authenticator implementations. - `DecodedToken` has been refactored and renamed to `InternalPolarisCredential`. It is also now a package-private component. - `TokenBroker.verify()` now returns PolarisCredential.
runtime/service/src/test/java/org/apache/polaris/service/auth/DefaultAuthenticatorTest.java
Show resolved
Hide resolved
* @see JWTBroker | ||
*/ | ||
@PolarisImmutable | ||
abstract class InternalPolarisCredential implements PolarisCredential { |
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.
[optional : may be another name ?] isn't it still a decoded bearer token ? maybe its just me but i find it a little bit hard to disassociate with the client crendentials flow semantics, which typically requires client secret
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'm not opposed to other names.
My rationale is: this component represents credentials extracted from an internal polaris token, hence the name I picked.
Let's wait for more feedback from other reviewers and I'll change the name accordingly.
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.
Would PolarisToken
work for you?
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.
(or InternalPolarisToken
)
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.
InternalPolarisToken
works for me ! This comment is totally optional to address, agree for waiting for more peoplr to weigh in :) !
Summary of changes:
Instead of including the principal id twice in the token, the principal name is now used as the subject claim. While the default authenticator doesn't need the principal name and works with just the principal id, not having the "real" principal name available could be a problem for other authenticator implementations.
DecodedToken
has been refactored and renamed toInternalPolarisCredential
. It is also now a package-private component as it is an implementation detail of Polaris internal auth mechanism.TokenBroker.verify()
now returnsPolarisCredential
.