-
Notifications
You must be signed in to change notification settings - Fork 332
Remove ActiveRolesProvider #2390
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
Conversation
|
@flyrain @collado-mike @dennishuo FYI As discussed in the ML, I know that My ask is for you to assess whether you can refactor your own roles provider impl from this: @RequestScoped class MyAuthenticator implements Authenticator {}
@RequestScoped class MyActiveRolesProvider implements ActiveRolesProvider {}to this: @RequestScoped class MyAuthenticator implements Authenticator {
@Inject MyActiveRolesProvider rolesProvider;
}That is, make your own impl of I created several protected methods in
But you are also free of course to use a different |
dennishuo
left a comment
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.
LGTM! Thanks for the mailing list discussion and your notes about compatibility and how any service providers with custom impls can easily migrate; it provides a good example to follow in general for SPI evolution PRs.
a805ead to
13b2199
Compare
13b2199 to
9295f58
Compare
dimas-b
left a comment
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.
LGTM with a minor comment 👍
|
|
||
| # This property is DEPRECATED for removal; use polaris.authentication.active-roles-provider.type instead | ||
| # This property is DEPRECATED for removal; it is now unused | ||
| polaris.active-roles-provider.type=default |
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.
Since it is unused, it is worth keeping in the "default" property set?
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.
Indeed I think we can just remove it.
9295f58 to
3629a09
Compare
|
@dennishuo do you mind re-approving? Now that 1.1.0 is out, we can merge this one in preparation for 1.2.0. Thanks! |
Summary of changes: - As proposed on the ML, `ActiveRolesProvider` is removed, and `DefaultActiveRolesProvider` is merged into `DefaultAuthenticator`. `ActiveRolesAugmentor` is also merged into `AuthenticatingAugmentor`. - The implicit convention that no roles in credentials == all roles requested is removed as it is ambiguous. Credentials must explicitly include the `PRINCIPAL_ROLE:ALL` pseudo-role to request all roles available. - PersistedPolarisPrincipal is removed. It existed merely as a means of passing the `PrincipalEntity` from the authenticator to the roles provider. This is not necessary anymore.
3629a09 to
81ee74b
Compare
|
Had to rebase due to merge conflicts. @dimas-b do you mind re-approving? 🙏 |
* Avoid calling deprecated `TableMetadataParser.read(FileIO, InputFile)` method. (apache#2609) Call `read(InputFile)` instead, as instructed by Iceberg javadoc. * Add doc notes about EclipseLink removal (apache#2605) * chore(docs): add polaris-api-specs section (apache#2598) * docs(README): Updating the READMEs to Reflect the Project Structure (apache#2599) * docs(README): Updating the READMEs to Reflect the Project Structure * fix(deps): update dependency io.opentelemetry:opentelemetry-bom to v1.54.1 (apache#2613) * Add Code of Conduct entry to the ASF menu (apache#2537) * Use the ASF Code Of Conduct * Update site/hugo.yaml Co-authored-by: Robert Stupp <snazy@snazy.de> --------- Co-authored-by: Robert Stupp <snazy@snazy.de> * fix(deps): update dependency org.postgresql:postgresql to v42.7.8 (apache#2619) * chore(deps): update dependency mypy to >=1.18, <=1.18.2 (apache#2617) * Update registry.access.redhat.com/ubi9/openjdk-21-runtime Docker tag to v1.23-6.1758133907 (apache#2612) * Introduce alternate in-memory buffering event listener (apache#2574) * fix(deps): update dependency org.assertj:assertj-core to v3.27.5 (apache#2618) * chore(deps): update dependency virtualenv to >=20.34.0,<20.35.0 (apache#2614) * Add Community Meeting 20250918 (apache#2622) * Add 1.1.0-incubating release on the website (apache#2621) * Add 1.1.0-incubating release content (apache#2625) * chore(errorprone): Enabling EqualsGetClass, PatternMatchingInstanceof, and UnusedMethod in ErrorProne (apache#2600) * fix(deps): update dependency com.adobe.testing:s3mock-testcontainers to v4.9.1 (apache#2626) * Unify create/loadTable call paths (apache#2589) In preparation for implementing sending non-credential config to REST Catalog clients for apache#2207 this PR unifies calls paths for create/load table operations. This change does not have any differences in authorization. This change is not expecte to have any material behaviour differences to the affected code paths. The main idea is to consolidate decision-making for that to include into REST responses and use method parameters like `EnumSet<AccessDelegationMode> delegationModes` for driving those decisions. * Remove numeric identifier from PolarisPrincipal (apache#2388) 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. * Include principal name in Polaris tokens (apache#2389) * Include principal name in Polaris tokens 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. * rename to InternalPolarisToken * main: bump to 1.2.0-incubating-SNAPSHOT (apache#2624) * bump version.txt to 1.2.0-incubating-SNAPSHOT * virtualenv: wider version range (apache#2623) see apache#2614 (comment) * Remove ActiveRolesProvider (apache#2390) Summary of changes: - As proposed on the ML, `ActiveRolesProvider` is removed, and `DefaultActiveRolesProvider` is merged into `DefaultAuthenticator`. `ActiveRolesAugmentor` is also merged into `AuthenticatingAugmentor`. - The implicit convention that no roles in credentials == all roles requested is removed as it is ambiguous. Credentials must explicitly include the `PRINCIPAL_ROLE:ALL` pseudo-role to request all roles available. - PersistedPolarisPrincipal is removed. It existed merely as a means of passing the `PrincipalEntity` from the authenticator to the roles provider. This is not necessary anymore. * NoSQL: adaptions * Last merged commit d1d359a --------- Co-authored-by: Dmitri Bourlatchkov <dmitri.bourlatchkov@gmail.com> Co-authored-by: Artur Rakhmatulin <artur.rakhmatulin@gmail.com> Co-authored-by: Adam Christian <105929021+adam-christian-software@users.noreply.github.com> Co-authored-by: Mend Renovate <bot@renovateapp.com> Co-authored-by: JB Onofré <jbonofre@apache.org> Co-authored-by: Alexandre Dutra <adutra@apache.org>
Summary of changes:
As proposed on the ML,
ActiveRolesProvideris removed, andDefaultActiveRolesProvideris merged intoDefaultAuthenticator.ActiveRolesAugmentoris also merged intoAuthenticatingAugmentor.The implicit convention that "no roles in credentials" == "all roles requested" is removed, as it is ambiguous. From now on, credentials must explicitly include the
PRINCIPAL_ROLE:ALLpseudo-role in order to request all roles.PersistedPolarisPrincipalis removed. It existed merely as a means of passing thePrincipalEntityfrom the authenticator to the roles provider. This is not necessary anymore.