Remove numeric identifier from PolarisPrincipal #2388
Conversation
This change removes the requirement for Polaris principals to have a numeric identifier, by removing the only sites where such an identifier was required: - In the `Resolver`. Instead, the `Resolver` now performs a lookup by principal name. - In `PolarisAdminService`. Instead, the code now compares the principal name against the entity name. Note: the lookup in the `Resolver` is still necessary, because the `Resolver` also needs to fetch the grant records.
adutra
force-pushed
the
auth-refactor-remove-principal-id
branch
from
7d3bf24 to
696afc3
Compare
github-project-automation
bot
moved this from PRs In Progress
to Ready to merge
in Basic Kanban Board
| */ | ||
| private static boolean isSelfOperation(PolarisAuthorizableOperation op) { | ||
| return op.equals(PolarisAuthorizableOperation.ROTATE_CREDENTIALS) | ||
| || op.equals(PolarisAuthorizableOperation.RESET_CREDENTIALS); |
There was a problem hiding this comment.
side note: RESET_CREDENTIALS is no going to be a "self" operation after #2197
There was a problem hiding this comment.
My understanding of #2197 is that the root user will be able to reset credentials of another user, but it won't remove the possibility for any user to reset their own credentials. So in a way it it still a "self" operation. Am I missing something?
There was a problem hiding this comment.
IIRC, in current main the RESET_CREDENTIALS operation is "dead code" (but will be restored to active service by #2197).
There was a problem hiding this comment.
I do not think a self-reset of credentials will be allowed, though.
There was a problem hiding this comment.
So, should I remove this check? IMO any changes here should be made by #2197 itself, but I don't mind proactively changing this bit in this PR.
There was a problem hiding this comment.
I see the change has been made in #2197: only ROTATE_CREDENTIALS remains a "self-operation".
There was a problem hiding this comment.
I agree that this change should be made under #2197. I commented here only as a "heads up" hoping to clarify potential merge conflicts. Sorry, if I caused confusion 😅
|
@dennishuo @collado-mike PTAL. |
This change removes the requirement for Polaris principals to have a numeric identifier, by removing the only sites where such an identifier was required:
Resolver. Instead of lookups by id, theResolvernow performs lookups by principal name.PolarisAdminService. Instead of comparing entity ids, the code now compares the principal name against the entity name and type.Note: the lookup in the
Resolveris still necessary, because theResolveralso needs to fetch the grant records.