You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@polaris.apache.org by "collado-mike (via GitHub)" <gi...@apache.org> on 2025/04/10 22:12:31 UTC
[PR] Add support for federated principal and role with block for manual role assignment [polaris]
collado-mike opened a new pull request, #1353:
URL: https://github.com/apache/polaris/pull/1353
First PR to begin implementing the design outlined in https://docs.google.com/document/d/15_3ZiRB6Lhzw0nxij341QUdxEIyFGTrI9_18bFIyJVo/edit?tab=t.0#heading=h.cu1a1acu4lc5 .
Implements https://github.com/apache/polaris/issues/441
Adds a `FEDERATED_ENTITY` property, which can be defined in Principals and PrincipalRoles. `FEDERATED` Principals cannot be created via the API, but `FEDERATED` roles can be. This allows for creating roles which can be assigned privileges, while actual principal creation is delegated to the JIT creation mechanism (not in this PR).
<!--
Possible security vulnerabilities: STOP here and contact security@apache.org instead!
Please update the title of the PR with a meaningful message - do not leave it "empty" or "generated"
Please update this summary field:
The summary should cover these topics, if applicable:
* the motivation for the change
* a description of the status quo, for example the current behavior
* the desired behavior
* etc
PR checklist:
- Do a self-review of your code before opening a pull request
- Make sure that there's good test coverage for the changes included in this PR
- Run tests locally before pushing a PR (./gradlew check)
- Code should have comments where applicable. Particularly hard-to-understand
areas deserve good in-line documentation.
- Include changes and enhancements to the documentation (in site/content/in-dev/unreleased)
- For Work In Progress Pull Requests, please use the Draft PR feature.
Make sure to add the information BELOW this comment.
Everything in this comment will NOT be added to the PR description.
-->
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add support for federated principal and role with block for manual role assignment [polaris]
Posted by "eric-maynard (via GitHub)" <gi...@apache.org>.
eric-maynard commented on code in PR #1353:
URL: https://github.com/apache/polaris/pull/1353#discussion_r2038939286
##########
spec/polaris-management-service.yml:
##########
@@ -1089,6 +1089,10 @@ components:
clientId:
type: string
description: The output-only OAuth clientId associated with this principal if applicable
+ federated:
Review Comment:
This part LGTM, but it should go to a ML vote
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add support for federated principal and role with block for manual role assignment [polaris]
Posted by "collado-mike (via GitHub)" <gi...@apache.org>.
collado-mike commented on code in PR #1353:
URL: https://github.com/apache/polaris/pull/1353#discussion_r2045079760
##########
integration-tests/src/main/java/org/apache/polaris/service/it/test/PolarisManagementServiceIntegrationTest.java:
##########
@@ -871,6 +873,42 @@ public void testCreatePrincipalAndRotateCredentials() {
// rotation that makes the old secret fall off retention.
}
+ @Test
+ public void testCreateFederatedPrincipalFails() {
Review Comment:
External catalog has a separate type because it has an extra field. Federated identity types don't have any additional fields at this point, so I'd prefer not to add an extra type. If extra fields end up getting added, then it would make sense, but for now deserializing wouldn't even be able to distinguish what type it should deserialized to.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add support for federated principal and role with block for manual role assignment [polaris]
Posted by "snazy (via GitHub)" <gi...@apache.org>.
snazy commented on code in PR #1353:
URL: https://github.com/apache/polaris/pull/1353#discussion_r2068658135
##########
polaris-core/src/main/java/org/apache/polaris/core/entity/PolarisEntity.java:
##########
@@ -182,6 +182,12 @@ public static List<NameAndId> toNameAndIdList(List<EntityNameLookupRecord> entit
.orElse(null);
}
+ public static boolean isFederated(PolarisBaseEntity entity) {
Review Comment:
Not sure why this comment's resolved.
I also agree that this attribute is, if at all, only need of a subset of all types and should only live there.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add support for federated principal and role with block for manual role assignment [polaris]
Posted by "collado-mike (via GitHub)" <gi...@apache.org>.
collado-mike commented on code in PR #1353:
URL: https://github.com/apache/polaris/pull/1353#discussion_r2073924410
##########
spec/polaris-management-service.yml:
##########
@@ -1151,6 +1175,10 @@ components:
maxLength: 256
pattern: '^(?!\s*[s|S][y|Y][s|S][t|T][e|E][m|M]\$).*$'
description: The name of the role
+ federated:
Review Comment:
Reverted this
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add support for federated principal and role with block for manual role assignment [polaris]
Posted by "eric-maynard (via GitHub)" <gi...@apache.org>.
eric-maynard commented on code in PR #1353:
URL: https://github.com/apache/polaris/pull/1353#discussion_r2038931222
##########
polaris-core/src/main/java/org/apache/polaris/core/entity/PrincipalRoleEntity.java:
##########
@@ -65,6 +65,13 @@ public Builder(PrincipalRoleEntity original) {
super(original);
}
+ public Builder setFederated(Boolean isFederated) {
+ if (isFederated != null && isFederated) {
Review Comment:
This logic doesn't look right to me. This entity would come out federated:
```
new PolarisEntity.Builder()
. . .
.setFederated(true)
.setFederated(false)
.build()
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add support for federated principal and role with block for manual role assignment [polaris]
Posted by "collado-mike (via GitHub)" <gi...@apache.org>.
collado-mike commented on code in PR #1353:
URL: https://github.com/apache/polaris/pull/1353#discussion_r2045057068
##########
polaris-core/src/main/java/org/apache/polaris/core/entity/PrincipalRoleEntity.java:
##########
@@ -65,6 +65,13 @@ public Builder(PrincipalRoleEntity original) {
super(original);
}
+ public Builder setFederated(Boolean isFederated) {
+ if (isFederated != null && isFederated) {
Review Comment:
Yeah, the idea is to avoid adding any entry for non-federated identities. I don't think the logic snippet you posted is reasonable or should be supported. I'd be in favor of an IllegalStateException if someone typed that code. Happy to add that.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add support for federated principal and role with block for manual role assignment [polaris]
Posted by "collado-mike (via GitHub)" <gi...@apache.org>.
collado-mike commented on code in PR #1353:
URL: https://github.com/apache/polaris/pull/1353#discussion_r2045082533
##########
spec/polaris-management-service.yml:
##########
@@ -1089,6 +1089,10 @@ components:
clientId:
type: string
description: The output-only OAuth clientId associated with this principal if applicable
+ federated:
Review Comment:
The main case is to support adding federated roles so that privileges can be defined. Per the original design doc, federated identities are created on the fly when a user logs in, but if we don't allow creation of federated roles, we can't define any privileges for those users until they've logged in. That makes things hard for the admins, IMO.
For Principals, I don't think federated principals should be created via the API, for the same reasoning you suggest, but we should be able to return federated principals and report that they are federated. To make that clearer, maybe I can update the spec to return one of two types, but to only allow creation of one type.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add support for federated principal and role with block for manual role assignment [polaris]
Posted by "collado-mike (via GitHub)" <gi...@apache.org>.
collado-mike commented on PR #1353:
URL: https://github.com/apache/polaris/pull/1353#issuecomment-2864554303
I responded on the ML. It doesn't really matter. @snazy if we remove the field from the Principal entity, will you remove the -1 on the PR?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add support for federated principal and role with block for manual role assignment [polaris]
Posted by "collado-mike (via GitHub)" <gi...@apache.org>.
collado-mike commented on code in PR #1353:
URL: https://github.com/apache/polaris/pull/1353#discussion_r2067815515
##########
polaris-core/src/main/java/org/apache/polaris/core/entity/PrincipalRoleEntity.java:
##########
@@ -65,6 +65,13 @@ public Builder(PrincipalRoleEntity original) {
super(original);
}
+ public Builder setFederated(Boolean isFederated) {
+ if (isFederated != null && isFederated) {
Review Comment:
I still feel it is a programming error for a builder to be invoked with this field twice. Is anyone opposed to an `IllegalStateException` in that case?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add support for federated principal and role with block for manual role assignment [polaris]
Posted by "collado-mike (via GitHub)" <gi...@apache.org>.
collado-mike commented on code in PR #1353:
URL: https://github.com/apache/polaris/pull/1353#discussion_r2070637045
##########
spec/polaris-management-service.yml:
##########
@@ -1105,6 +1105,30 @@ components:
required:
- name
+ Principal:
+ description: A Polaris principal.
+ type: object
+ allOf:
+ - $ref: '#/components/schemas/BasePrincipal'
+ - type: object
+ properties:
+ clientId:
+ type: string
+ description: The output-only OAuth clientId associated with this principal if applicable
+
+ FederatedPrincipal:
+ description: A federated Polaris principal.
+ type: object
+ allOf:
+ - $ref: '#/components/schemas/BasePrincipal'
+ - type: object
+ properties:
+ federated:
Review Comment:
The intention in this change to the spec is to clarify in the spec what can be sent to the modification APIs (create/update) and what can be returned in the read APIs. Federated principals can be returned, but can't be created or updated. I proposed making this change in response to your comments on the previous revision - https://github.com/apache/polaris/pull/1353#discussion_r2045082533
TBH, I would have preferred not creating separate types on the backend (the original revision simply added a `federated` field), but this was the only way I could force the spec to treat the two types differently.
The `federated` type has to be present in order for clients to be able to distinguish between federated and non-federated principals. `type` doesn't translate in JSON - only the presence and values of the fields in the response.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add support for federated principal and role with block for manual role assignment [polaris]
Posted by "collado-mike (via GitHub)" <gi...@apache.org>.
collado-mike commented on code in PR #1353:
URL: https://github.com/apache/polaris/pull/1353#discussion_r2047870731
##########
service/common/src/main/java/org/apache/polaris/service/admin/PolarisAdminService.java:
##########
@@ -771,6 +771,9 @@ public PrincipalWithCredentials createPrincipal(PolarisEntity entity) {
PolarisAuthorizableOperation op = PolarisAuthorizableOperation.CREATE_PRINCIPAL;
authorizeBasicRootOperationOrThrow(op);
+ if (PolarisEntity.isFederated(entity)) {
Review Comment:
The presence of a specific property is the only way that the admin API can tell if an entity is managed by an IdP. Principal Roles, federated or not, must be present in the service in order to be granted privileges. That means they have to be persisted, so the persistence API is going to return them as entities. We have to be able to introspect those entities to know when it is or isn't ok to grant access to those roles. This was outlined in the design doc at https://docs.google.com/document/d/15_3ZiRB6Lhzw0nxij341QUdxEIyFGTrI9_18bFIyJVo/edit?tab=t.0#heading=h.w9dvdtp5mw5p
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add support for federated principal and role with block for manual role assignment [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #1353:
URL: https://github.com/apache/polaris/pull/1353#discussion_r2047877788
##########
polaris-core/src/main/java/org/apache/polaris/core/entity/PrincipalRoleEntity.java:
##########
@@ -65,6 +65,13 @@ public Builder(PrincipalRoleEntity original) {
super(original);
}
+ public Builder setFederated(Boolean isFederated) {
+ if (isFederated != null && isFederated) {
Review Comment:
I support @eric-maynard 's point here. Current code is very risky in terms of latent bug, IMHO. A caller of `.setFederated(false)` can reasonably expect the property to become `false` regardless of any previous `set*` calls (which might have been made in a different context).
##########
polaris-core/src/main/java/org/apache/polaris/core/entity/PrincipalRoleEntity.java:
##########
@@ -65,6 +65,13 @@ public Builder(PrincipalRoleEntity original) {
super(original);
}
+ public Builder setFederated(Boolean isFederated) {
+ if (isFederated != null && isFederated) {
Review Comment:
I support @eric-maynard 's point here. Current code is very risky in terms of latent bugs, IMHO. A caller of `.setFederated(false)` can reasonably expect the property to become `false` regardless of any previous `set*` calls (which might have been made in a different context).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add support for federated principal and role with block for manual role assignment [polaris]
Posted by "collado-mike (via GitHub)" <gi...@apache.org>.
collado-mike commented on PR #1353:
URL: https://github.com/apache/polaris/pull/1353#issuecomment-2873916632
> Not sure why my review was dismissed, but I was busy with some quite important stuff. The last request was right before the weekend. I do want to take another look at this during this week, let's please not rush it.
@snazy I dismissed it as stale after posting an update that specifically addressed your latest comment. I had asked for some agreement on the ML last Wednesday and did not receive any response. I asked again here on Thursday and did not receive a response. So today I addressed what I felt was your specific concern and re-requested your review. I don't think anyone can say I'm not making a good faith attempt to reach consensus here.
But this PR has been open for a month. I don't think this is rushing. I thought we had agreement on the ML nearly two weeks ago. I'm willing to do the work to come together, but I need some communication to move forward.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add support for federated principal and role with block for manual role assignment [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #1353:
URL: https://github.com/apache/polaris/pull/1353#discussion_r2047914839
##########
service/common/src/main/java/org/apache/polaris/service/admin/PolarisAdminService.java:
##########
@@ -771,6 +771,9 @@ public PrincipalWithCredentials createPrincipal(PolarisEntity entity) {
PolarisAuthorizableOperation op = PolarisAuthorizableOperation.CREATE_PRINCIPAL;
authorizeBasicRootOperationOrThrow(op);
+ if (PolarisEntity.isFederated(entity)) {
Review Comment:
The design doc does talk about this, but I do not recall firm decisions on mentioned alternatives :)
Even if we create entities for federated principals, consider a use case when someone creates a principal called `A` (non-federated) and later someone else adds federation and federated principal with name `A` comes in (or the other way around). Where is this conflict resolved? This is what I mean be "manager" and I do not think `PolarisAdminServer` alone is sufficient (it generally just supports the Admin REST API).
Another use case: a federated principal `B` is created thought some IdP integration logic and brings in some properties from the IdP. A user updates principal `B` via Admin API. The doc mentions that being able to update principal properties is desirable, but where do we resolve conflicts between updated from the API and from the integration side?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add support for federated principal and role with block for manual role assignment [polaris]
Posted by "collado-mike (via GitHub)" <gi...@apache.org>.
collado-mike commented on PR #1353:
URL: https://github.com/apache/polaris/pull/1353#issuecomment-2860925450
> As mentioned on the ML, there's no need to have that "federated" flag on principals, which is still there.
Per Dmitri's comment above
> As discussed, having API-level "federated" flags does not mean that Polaris will necessarily import Principal entities from an external IdP, but it does have certain symmetry with the PrincipalRole's federated flag, so it probably makes sense to add both of them at the same time.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add support for federated principal and role with block for manual role assignment [polaris]
Posted by "collado-mike (via GitHub)" <gi...@apache.org>.
collado-mike commented on code in PR #1353:
URL: https://github.com/apache/polaris/pull/1353#discussion_r2069853446
##########
polaris-core/src/main/java/org/apache/polaris/core/entity/PolarisEntity.java:
##########
@@ -182,6 +182,12 @@ public static List<NameAndId> toNameAndIdList(List<EntityNameLookupRecord> entit
.orElse(null);
}
+ public static boolean isFederated(PolarisBaseEntity entity) {
Review Comment:
I'd resolved it because I'd fixed it in last revision. But I didn't realize my push hadn't gone through - I have ssh cert issues sometimes in my terminal đ¤ˇđŊââī¸
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add support for federated principal and role with block for manual role assignment [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #1353:
URL: https://github.com/apache/polaris/pull/1353#discussion_r2070556405
##########
spec/polaris-management-service.yml:
##########
@@ -1105,6 +1105,30 @@ components:
required:
- name
+ Principal:
+ description: A Polaris principal.
+ type: object
+ allOf:
+ - $ref: '#/components/schemas/BasePrincipal'
+ - type: object
+ properties:
+ clientId:
+ type: string
+ description: The output-only OAuth clientId associated with this principal if applicable
+
+ FederatedPrincipal:
+ description: A federated Polaris principal.
+ type: object
+ allOf:
+ - $ref: '#/components/schemas/BasePrincipal'
+ - type: object
+ properties:
+ federated:
Review Comment:
Do we need this property when the type already implies the Principal is federated?
##########
polaris-core/src/main/java/org/apache/polaris/core/entity/PrincipalEntity.java:
##########
@@ -78,6 +95,13 @@ public Builder setCredentialRotationRequiredState() {
return this;
}
+ public Builder setFederated(Boolean isFederated) {
+ if (isFederated != null && isFederated) {
+ internalProperties.put(FederatedEntities.FEDERATED_ENTITY, "true");
+ }
+ return this;
+ }
Review Comment:
Why is this setter impl different from a similar setter in `PrincipalRoleEntity`? https://github.com/apache/polaris/pull/1353/files#diff-f746c88d518ffc25b33cbd1052fe77d99e0a35711955d2f544a822b02a9e6f09R69
##########
api/management-model/build.gradle.kts:
##########
@@ -57,6 +57,8 @@ openApiGenerate {
additionalProperties.put("apiNameSuffix", "Api")
additionalProperties.put("metricsPrefix", "polaris")
serverVariables = mapOf("basePath" to "api/v1")
+ openapiNormalizer.put("REFACTOR_ALLOF_WITH_PROPERTIES_ONLY", "true")
Review Comment:
Could you add a comment about what these options mean?
##########
integration-tests/src/main/java/org/apache/polaris/service/it/test/PolarisManagementServiceIntegrationTest.java:
##########
@@ -871,6 +874,47 @@ public void testCreatePrincipalAndRotateCredentials() {
// rotation that makes the old secret fall off retention.
}
+ @Test
+ public void testCreateFederatedPrincipalFails() throws JsonProcessingException {
+ // Create a federated Principal
+ FederatedPrincipal federatedPrincipal =
+ new FederatedPrincipal(
+ true, client.newEntityName("federatedPrincipal"), Map.of(), 0L, 0L, 1);
+
+ // Attempt to create the federated Principal using the managementApi
+ try (Response createPResponse =
+ managementApi
+ .request("v1/principals")
+ .post(
+ Entity.json(
+ "{\"principal\":"
+ + new ObjectMapper().writeValueAsString(federatedPrincipal)
+ + "}"))) {
+ assertThat(createPResponse).returns(CREATED.getStatusCode(), Response::getStatus);
Review Comment:
The test's name implies that this request should fail, but it succeeds?
##########
spec/polaris-management-service.yml:
##########
@@ -1105,6 +1105,30 @@ components:
required:
- name
+ Principal:
+ description: A Polaris principal.
+ type: object
+ allOf:
+ - $ref: '#/components/schemas/BasePrincipal'
+ - type: object
+ properties:
+ clientId:
+ type: string
+ description: The output-only OAuth clientId associated with this principal if applicable
+
+ FederatedPrincipal:
+ description: A federated Polaris principal.
+ type: object
+ allOf:
+ - $ref: '#/components/schemas/BasePrincipal'
+ - type: object
+ properties:
+ federated:
Review Comment:
If we only want to treat then as sub-types in server code, but not in JSON, I believe we can still handle that in deserializers without complicating the Open API spec... WDYT?
##########
spec/polaris-management-service.yml:
##########
@@ -1151,6 +1175,10 @@ components:
maxLength: 256
pattern: '^(?!\s*[s|S][y|Y][s|S][t|T][e|E][m|M]\$).*$'
description: The name of the role
+ federated:
Review Comment:
We use sub-types for federated Principals, but not for federated Principal Roles?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add support for federated principal and role with block for manual role assignment [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #1353:
URL: https://github.com/apache/polaris/pull/1353#discussion_r2069681604
##########
polaris-core/src/main/java/org/apache/polaris/core/entity/PrincipalRoleEntity.java:
##########
@@ -65,6 +65,13 @@ public Builder(PrincipalRoleEntity original) {
super(original);
}
+ public Builder setFederated(Boolean isFederated) {
+ if (isFederated != null && isFederated) {
Review Comment:
From my POV it is way easier to allow multiple set calls with the latest call's value winning, than reducing flexibility by throwing `IllegalStateException`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add support for federated principal and role with block for manual role assignment [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #1353:
URL: https://github.com/apache/polaris/pull/1353#discussion_r2070887179
##########
spec/polaris-management-service.yml:
##########
@@ -1105,6 +1105,30 @@ components:
required:
- name
+ Principal:
+ description: A Polaris principal.
+ type: object
+ allOf:
+ - $ref: '#/components/schemas/BasePrincipal'
+ - type: object
+ properties:
+ clientId:
+ type: string
+ description: The output-only OAuth clientId associated with this principal if applicable
+
+ FederatedPrincipal:
+ description: A federated Polaris principal.
+ type: object
+ allOf:
+ - $ref: '#/components/schemas/BasePrincipal'
+ - type: object
+ properties:
+ federated:
Review Comment:
TBH, this new state of the PR looks very confusing to me:
1) The OpenAPI spec attempts to describe that clients cannot create a FederatedPrincipal, but actual Management endpoint accept such requests as valid and only block those principals in the somewhat deep impl code. Is this complication worth the effort? IMHO, the end result is more or less the same as if we had the new `federated` property added to the old `Principal` type in the API spec.
2) If clients have to rely on the presence of the `federated` property when interpreting Principals' JSON, this looks to me that the API is defined using effectively a flat type system, not a hierarchy. Again, having a separate `FederatedPrincipal` seems redundant to me.
Apologies if my comments cause a lot of back-and-forth, but I'm trying to reduce complexity.
To summarize: If we want to expose federated Principals in the API for reads (and I'm fine with that). I'd prefer to just add a simple new `federated` property to the single `Principal` type in the API spec.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add support for federated principal and role with block for manual role assignment [polaris]
Posted by "snazy (via GitHub)" <gi...@apache.org>.
snazy commented on PR #1353:
URL: https://github.com/apache/polaris/pull/1353#issuecomment-2859653597
> @snazy can we remove your -1?
As mentioned on the ML, there's no need to have that "federated" flag on principals, which is still there.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add support for federated principal and role with block for manual role assignment [polaris]
Posted by "collado-mike (via GitHub)" <gi...@apache.org>.
collado-mike commented on code in PR #1353:
URL: https://github.com/apache/polaris/pull/1353#discussion_r2047861024
##########
spec/polaris-management-service.yml:
##########
@@ -1089,6 +1089,10 @@ components:
clientId:
type: string
description: The output-only OAuth clientId associated with this principal if applicable
+ federated:
Review Comment:
I updated the spec to be more clear about when federated principals are expected as return types/arguments. This unfortunately cluttered the generated code more than I had hoped. I'll post a link on the mailing list re: this change.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add support for federated principal and role with block for manual role assignment [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #1353:
URL: https://github.com/apache/polaris/pull/1353#discussion_r2047914839
##########
service/common/src/main/java/org/apache/polaris/service/admin/PolarisAdminService.java:
##########
@@ -771,6 +771,9 @@ public PrincipalWithCredentials createPrincipal(PolarisEntity entity) {
PolarisAuthorizableOperation op = PolarisAuthorizableOperation.CREATE_PRINCIPAL;
authorizeBasicRootOperationOrThrow(op);
+ if (PolarisEntity.isFederated(entity)) {
Review Comment:
The design doc does talk about this, but I do not recall firm decisions on mentioned alternatives :)
Even if we create entities for federated principals, consider a use case when someone creates a principal called `A` (non-federated) and later someone else adds federation and federated principal with name `A` comes in (or the other way around). Where is this conflict resolved? This is what I mean be "manager".
Another use case: a federated principal `B` is created thought some IdP integration logic and brings in some properties from the IdP. A user updates principal `B` via Admin API. The doc mentions that being able to update principal properties is desirable, but where do we resolve conflicts between updates from the API and from the integration side?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add support for federated principal and role with block for manual role assignment [polaris]
Posted by "collado-mike (via GitHub)" <gi...@apache.org>.
collado-mike commented on code in PR #1353:
URL: https://github.com/apache/polaris/pull/1353#discussion_r2070630149
##########
integration-tests/src/main/java/org/apache/polaris/service/it/test/PolarisManagementServiceIntegrationTest.java:
##########
@@ -871,6 +874,47 @@ public void testCreatePrincipalAndRotateCredentials() {
// rotation that makes the old secret fall off retention.
}
+ @Test
+ public void testCreateFederatedPrincipalFails() throws JsonProcessingException {
+ // Create a federated Principal
+ FederatedPrincipal federatedPrincipal =
+ new FederatedPrincipal(
+ true, client.newEntityName("federatedPrincipal"), Map.of(), 0L, 0L, 1);
+
+ // Attempt to create the federated Principal using the managementApi
+ try (Response createPResponse =
+ managementApi
+ .request("v1/principals")
+ .post(
+ Entity.json(
+ "{\"principal\":"
+ + new ObjectMapper().writeValueAsString(federatedPrincipal)
+ + "}"))) {
+ assertThat(createPResponse).returns(CREATED.getStatusCode(), Response::getStatus);
Review Comment:
Ah, I modified this test from the previous revision. Let me see if I can improve the behavior
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add support for federated principal and role with block for manual role assignment [polaris]
Posted by "eric-maynard (via GitHub)" <gi...@apache.org>.
eric-maynard commented on code in PR #1353:
URL: https://github.com/apache/polaris/pull/1353#discussion_r2038937204
##########
integration-tests/src/main/java/org/apache/polaris/service/it/test/PolarisManagementServiceIntegrationTest.java:
##########
@@ -871,6 +873,42 @@ public void testCreatePrincipalAndRotateCredentials() {
// rotation that makes the old secret fall off retention.
}
+ @Test
+ public void testCreateFederatedPrincipalFails() {
Review Comment:
Given how different the semantics for federated principal (role)s appear to be, I wonder if we should consider a subtype? It looks like most APIs are valid for nonfederated principal (role)s are not valid for federated ones. This is reminiscent of EXTERNAL catalogs
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add support for federated principal and role with block for manual role assignment [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #1353:
URL: https://github.com/apache/polaris/pull/1353#discussion_r2043029391
##########
quarkus/service/src/test/java/org/apache/polaris/service/quarkus/admin/ManagementServiceTest.java:
##########
@@ -158,4 +173,150 @@ public void testUpdateCatalogWithDisallowedStorageConfig() {
.isInstanceOf(IllegalArgumentException.class)
.hasMessage("Unsupported storage type: FILE");
}
+
+ private PolarisMetaStoreManager setupMetaStoreManager() {
+ MetaStoreManagerFactory metaStoreManagerFactory = services.metaStoreManagerFactory();
+ RealmContext realmContext = services.realmContext();
+ return metaStoreManagerFactory.getOrCreateMetaStoreManager(realmContext);
+ }
+
+ private PolarisCallContext setupCallContext(PolarisMetaStoreManager metaStoreManager) {
+ MetaStoreManagerFactory metaStoreManagerFactory = services.metaStoreManagerFactory();
+ RealmContext realmContext = services.realmContext();
+ return new PolarisCallContext(
+ metaStoreManagerFactory.getOrCreateSessionSupplier(realmContext).get(),
+ services.polarisDiagnostics());
+ }
+
+ private PolarisAdminService setupPolarisAdminService(
+ PolarisMetaStoreManager metaStoreManager, PolarisCallContext callContext) {
+ RealmContext realmContext = services.realmContext();
+ return new PolarisAdminService(
+ CallContext.of(realmContext, callContext),
+ services.entityManagerFactory().getOrCreateEntityManager(realmContext),
+ metaStoreManager,
+ new SecurityContext() {
+ @Override
+ public Principal getUserPrincipal() {
+ return new AuthenticatedPolarisPrincipal(
+ new PrincipalEntity.Builder().setName("root").build(), Set.of("service_admin"));
+ }
+
+ @Override
+ public boolean isUserInRole(String role) {
+ return true;
+ }
+
+ @Override
+ public boolean isSecure() {
+ return false;
+ }
+
+ @Override
+ public String getAuthenticationScheme() {
+ return "";
+ }
+ },
+ new PolarisAuthorizerImpl(new DefaultConfigurationStore(Map.of())));
+ }
+
+ private PrincipalEntity createPrincipal(
+ PolarisMetaStoreManager metaStoreManager,
+ PolarisCallContext callContext,
+ String name,
+ boolean isFederated) {
+ return new PrincipalEntity.Builder()
+ .setFederated(isFederated)
+ .setName(name)
+ .setCreateTimestamp(Instant.now().toEpochMilli())
+ .setId(metaStoreManager.generateNewEntityId(callContext).getId())
+ .build();
+ }
+
+ private PrincipalRoleEntity createRole(
+ PolarisMetaStoreManager metaStoreManager,
+ PolarisCallContext callContext,
+ String name,
+ boolean isFederated) {
+ return new PrincipalRoleEntity.Builder()
+ .setId(metaStoreManager.generateNewEntityId(callContext).getId())
+ .setName(name)
+ .setFederated(isFederated)
+ .setProperties(Map.of())
+ .setCreateTimestamp(Instant.now().toEpochMilli())
+ .setLastUpdateTimestamp(Instant.now().toEpochMilli())
+ .build();
+ }
+
+ @Test
+ public void testCannotAddFederatedPrincipalToNonFederatedRole() {
+ PolarisMetaStoreManager metaStoreManager = setupMetaStoreManager();
+ PolarisCallContext callContext = setupCallContext(metaStoreManager);
+ PolarisAdminService polarisAdminService =
+ setupPolarisAdminService(metaStoreManager, callContext);
+
+ PrincipalEntity federatedPrincipal =
+ createPrincipal(metaStoreManager, callContext, "federated_id", true);
+ metaStoreManager.createPrincipal(callContext, federatedPrincipal);
+
+ PrincipalRoleEntity nonFederatedRole =
+ createRole(metaStoreManager, callContext, "non_federated_role", false);
+ EntityResult result =
+ metaStoreManager.createEntityIfNotExists(callContext, null, nonFederatedRole);
+ assertThat(result.isSuccess()).isTrue();
+
+ assertThatThrownBy(
+ () ->
+ polarisAdminService.assignPrincipalRole(
+ federatedPrincipal.getName(), nonFederatedRole.getName()))
+ .isInstanceOf(ValidationException.class);
+ }
+
+ @Test
+ public void testCannotAddNonFederatedPrincipalToFederatedRole() {
Review Comment:
The restrictions proposed in the table above make sense to me :+1:
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add support for federated principal and role with block for manual role assignment [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #1353:
URL: https://github.com/apache/polaris/pull/1353#discussion_r2043032833
##########
service/common/src/main/java/org/apache/polaris/service/admin/PolarisAdminService.java:
##########
@@ -771,6 +771,9 @@ public PrincipalWithCredentials createPrincipal(PolarisEntity entity) {
PolarisAuthorizableOperation op = PolarisAuthorizableOperation.CREATE_PRINCIPAL;
authorizeBasicRootOperationOrThrow(op);
+ if (PolarisEntity.isFederated(entity)) {
Review Comment:
Why do we expose "federated" as a property settable via java API only to add validation that it cannot be set?
Also, REST API (which is the only caller of this method) does not allow setting the "federated" property, as far as I can tell, because it does not set _internal_ properties on the `PrincipalEntity` :thinking:
##########
spec/polaris-management-service.yml:
##########
@@ -1089,6 +1089,10 @@ components:
clientId:
type: string
description: The output-only OAuth clientId associated with this principal if applicable
+ federated:
Review Comment:
+1 to ML discussion.
What's the use case for exposing federated principal in the Polaris Management API? Acting as a proxy in front of an IdP sounds questionable to me.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add support for federated principal and role with block for manual role assignment [polaris]
Posted by "eric-maynard (via GitHub)" <gi...@apache.org>.
eric-maynard commented on code in PR #1353:
URL: https://github.com/apache/polaris/pull/1353#discussion_r2045147504
##########
polaris-core/src/main/java/org/apache/polaris/core/entity/PrincipalRoleEntity.java:
##########
@@ -38,19 +38,19 @@ public static PrincipalRoleEntity of(PolarisBaseEntity sourceEntity) {
public static PrincipalRoleEntity fromPrincipalRole(PrincipalRole principalRole) {
return new Builder()
.setName(principalRole.getName())
+ .setFederated(principalRole.getFederated())
.setProperties(principalRole.getProperties())
.build();
}
public PrincipalRole asPrincipalRole() {
- PrincipalRole principalRole =
- new PrincipalRole(
- getName(),
- getPropertiesAsMap(),
- getCreateTimestamp(),
- getLastUpdateTimestamp(),
- getEntityVersion());
- return principalRole;
+ return new PrincipalRole(
+ getName(),
+ PolarisEntity.isFederated(this),
Review Comment:
Makes sense, it sounds like this folds into [another comment above](https://github.com/apache/polaris/pull/1353#discussion_r2038929198)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add support for federated principal and role with block for manual role assignment [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #1353:
URL: https://github.com/apache/polaris/pull/1353#discussion_r2047887344
##########
spec/polaris-management-service.yml:
##########
@@ -1089,6 +1089,10 @@ components:
clientId:
type: string
description: The output-only OAuth clientId associated with this principal if applicable
+ federated:
Review Comment:
Thanks for the clarification! The description of the property LGTM. However, I'm not not sure about exposing federated Principals in the Polaris API.
Granted, we have to expose federated Principal Roles in order to allow assignment to Catalog Roles, but do Principals have to be exposed?
The design doc does talk about Polaris entities for federated roles, but I did not see it mention having to define entities for federated principals. Per section `Automatic User Provisioning` there is no strict requirement for federated principal entities... hence my question.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add support for federated principal and role with block for manual role assignment [polaris]
Posted by "collado-mike (via GitHub)" <gi...@apache.org>.
collado-mike commented on code in PR #1353:
URL: https://github.com/apache/polaris/pull/1353#discussion_r2070630659
##########
polaris-core/src/main/java/org/apache/polaris/core/entity/PrincipalEntity.java:
##########
@@ -78,6 +95,13 @@ public Builder setCredentialRotationRequiredState() {
return this;
}
+ public Builder setFederated(Boolean isFederated) {
+ if (isFederated != null && isFederated) {
+ internalProperties.put(FederatedEntities.FEDERATED_ENTITY, "true");
+ }
+ return this;
+ }
Review Comment:
I'll update this to be the same as the other
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add support for federated principal and role with block for manual role assignment [polaris]
Posted by "collado-mike (via GitHub)" <gi...@apache.org>.
collado-mike commented on PR #1353:
URL: https://github.com/apache/polaris/pull/1353#issuecomment-2859609442
@snazy can we remove your -1?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add support for federated principal and role with block for manual role assignment [polaris]
Posted by "collado-mike (via GitHub)" <gi...@apache.org>.
collado-mike commented on code in PR #1353:
URL: https://github.com/apache/polaris/pull/1353#discussion_r2073764168
##########
spec/polaris-management-service.yml:
##########
@@ -1105,6 +1105,30 @@ components:
required:
- name
+ Principal:
+ description: A Polaris principal.
+ type: object
+ allOf:
+ - $ref: '#/components/schemas/BasePrincipal'
+ - type: object
+ properties:
+ clientId:
+ type: string
+ description: The output-only OAuth clientId associated with this principal if applicable
+
+ FederatedPrincipal:
+ description: A federated Polaris principal.
+ type: object
+ allOf:
+ - $ref: '#/components/schemas/BasePrincipal'
+ - type: object
+ properties:
+ federated:
Review Comment:
> I'd prefer to just add a simple new federated property to the single Principal type in the API spec.
That's what I had in the first revision âšī¸ It is my preference as well, as it's simple and straightforward to implement. There's really no difference in the backend (especially, as we don't persist the federated principals). I'm happy to revert to the original revision, but I'd like to avoid any more back and forth
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add support for federated principal and role with block for manual role assignment [polaris]
Posted by "eric-maynard (via GitHub)" <gi...@apache.org>.
eric-maynard commented on code in PR #1353:
URL: https://github.com/apache/polaris/pull/1353#discussion_r2038937204
##########
integration-tests/src/main/java/org/apache/polaris/service/it/test/PolarisManagementServiceIntegrationTest.java:
##########
@@ -871,6 +873,42 @@ public void testCreatePrincipalAndRotateCredentials() {
// rotation that makes the old secret fall off retention.
}
+ @Test
+ public void testCreateFederatedPrincipalFails() {
Review Comment:
Given how different the semantics for federated principal (role)s appear to be, I wonder if we should consider a subtype? It looks like most APIs are valid for nonfederated principal (role)s are not valid for federated ones. This is reminiscent of EXTERNAL catalogs, but maybe more extreme
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add support for federated principal and role with block for manual role assignment [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #1353:
URL: https://github.com/apache/polaris/pull/1353#discussion_r2074299008
##########
api/management-model/build.gradle.kts:
##########
@@ -57,6 +57,8 @@ openApiGenerate {
additionalProperties.put("apiNameSuffix", "Api")
additionalProperties.put("metricsPrefix", "polaris")
serverVariables = mapOf("basePath" to "api/v1")
+ openapiNormalizer.put("REFACTOR_ALLOF_WITH_PROPERTIES_ONLY", "true")
Review Comment:
Do we still need this with the latest state of this PR?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add support for federated principal and role with block for manual role assignment [polaris]
Posted by "collado-mike (via GitHub)" <gi...@apache.org>.
collado-mike commented on code in PR #1353:
URL: https://github.com/apache/polaris/pull/1353#discussion_r2045077628
##########
quarkus/service/src/test/java/org/apache/polaris/service/quarkus/admin/ManagementServiceTest.java:
##########
@@ -158,4 +173,150 @@ public void testUpdateCatalogWithDisallowedStorageConfig() {
.isInstanceOf(IllegalArgumentException.class)
.hasMessage("Unsupported storage type: FILE");
}
+
+ private PolarisMetaStoreManager setupMetaStoreManager() {
+ MetaStoreManagerFactory metaStoreManagerFactory = services.metaStoreManagerFactory();
+ RealmContext realmContext = services.realmContext();
+ return metaStoreManagerFactory.getOrCreateMetaStoreManager(realmContext);
+ }
+
+ private PolarisCallContext setupCallContext(PolarisMetaStoreManager metaStoreManager) {
+ MetaStoreManagerFactory metaStoreManagerFactory = services.metaStoreManagerFactory();
+ RealmContext realmContext = services.realmContext();
+ return new PolarisCallContext(
+ metaStoreManagerFactory.getOrCreateSessionSupplier(realmContext).get(),
+ services.polarisDiagnostics());
+ }
+
+ private PolarisAdminService setupPolarisAdminService(
+ PolarisMetaStoreManager metaStoreManager, PolarisCallContext callContext) {
+ RealmContext realmContext = services.realmContext();
+ return new PolarisAdminService(
+ CallContext.of(realmContext, callContext),
+ services.entityManagerFactory().getOrCreateEntityManager(realmContext),
+ metaStoreManager,
+ new SecurityContext() {
+ @Override
+ public Principal getUserPrincipal() {
+ return new AuthenticatedPolarisPrincipal(
+ new PrincipalEntity.Builder().setName("root").build(), Set.of("service_admin"));
+ }
+
+ @Override
+ public boolean isUserInRole(String role) {
+ return true;
+ }
+
+ @Override
+ public boolean isSecure() {
+ return false;
+ }
+
+ @Override
+ public String getAuthenticationScheme() {
+ return "";
+ }
+ },
+ new PolarisAuthorizerImpl(new DefaultConfigurationStore(Map.of())));
+ }
+
+ private PrincipalEntity createPrincipal(
+ PolarisMetaStoreManager metaStoreManager,
+ PolarisCallContext callContext,
+ String name,
+ boolean isFederated) {
+ return new PrincipalEntity.Builder()
+ .setFederated(isFederated)
+ .setName(name)
+ .setCreateTimestamp(Instant.now().toEpochMilli())
+ .setId(metaStoreManager.generateNewEntityId(callContext).getId())
+ .build();
+ }
+
+ private PrincipalRoleEntity createRole(
+ PolarisMetaStoreManager metaStoreManager,
+ PolarisCallContext callContext,
+ String name,
+ boolean isFederated) {
+ return new PrincipalRoleEntity.Builder()
+ .setId(metaStoreManager.generateNewEntityId(callContext).getId())
+ .setName(name)
+ .setFederated(isFederated)
+ .setProperties(Map.of())
+ .setCreateTimestamp(Instant.now().toEpochMilli())
+ .setLastUpdateTimestamp(Instant.now().toEpochMilli())
+ .build();
+ }
+
+ @Test
+ public void testCannotAddFederatedPrincipalToNonFederatedRole() {
+ PolarisMetaStoreManager metaStoreManager = setupMetaStoreManager();
+ PolarisCallContext callContext = setupCallContext(metaStoreManager);
+ PolarisAdminService polarisAdminService =
+ setupPolarisAdminService(metaStoreManager, callContext);
+
+ PrincipalEntity federatedPrincipal =
+ createPrincipal(metaStoreManager, callContext, "federated_id", true);
+ metaStoreManager.createPrincipal(callContext, federatedPrincipal);
+
+ PrincipalRoleEntity nonFederatedRole =
+ createRole(metaStoreManager, callContext, "non_federated_role", false);
+ EntityResult result =
+ metaStoreManager.createEntityIfNotExists(callContext, null, nonFederatedRole);
+ assertThat(result.isSuccess()).isTrue();
+
+ assertThatThrownBy(
+ () ->
+ polarisAdminService.assignPrincipalRole(
+ federatedPrincipal.getName(), nonFederatedRole.getName()))
+ .isInstanceOf(ValidationException.class);
+ }
+
+ @Test
+ public void testCannotAddNonFederatedPrincipalToFederatedRole() {
Review Comment:
I'll skip the first row because it's already covered exhaustively elsewhere. Personally, I tend to shy away from parameterized tests, as I think they're overused and oftentimes obfuscate the test code with a bunch of if/else statements, but I'm ok adding it here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add support for federated principal and role with block for manual role assignment [polaris]
Posted by "collado-mike (via GitHub)" <gi...@apache.org>.
collado-mike commented on code in PR #1353:
URL: https://github.com/apache/polaris/pull/1353#discussion_r2045082533
##########
spec/polaris-management-service.yml:
##########
@@ -1089,6 +1089,10 @@ components:
clientId:
type: string
description: The output-only OAuth clientId associated with this principal if applicable
+ federated:
Review Comment:
The main case is to support adding federated roles so that privileges can be defined. Per the original design doc, federated identities are created on the fly when a user logs in, but if we don't allow creation of federated roles, we can't define any privileges for those users until they've logged in. That makes things hard for the admins, IMO
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add support for federated principal and role with block for manual role assignment [polaris]
Posted by "eric-maynard (via GitHub)" <gi...@apache.org>.
eric-maynard commented on code in PR #1353:
URL: https://github.com/apache/polaris/pull/1353#discussion_r2038934219
##########
quarkus/service/src/test/java/org/apache/polaris/service/quarkus/admin/ManagementServiceTest.java:
##########
@@ -158,4 +173,150 @@ public void testUpdateCatalogWithDisallowedStorageConfig() {
.isInstanceOf(IllegalArgumentException.class)
.hasMessage("Unsupported storage type: FILE");
}
+
+ private PolarisMetaStoreManager setupMetaStoreManager() {
+ MetaStoreManagerFactory metaStoreManagerFactory = services.metaStoreManagerFactory();
+ RealmContext realmContext = services.realmContext();
+ return metaStoreManagerFactory.getOrCreateMetaStoreManager(realmContext);
+ }
+
+ private PolarisCallContext setupCallContext(PolarisMetaStoreManager metaStoreManager) {
+ MetaStoreManagerFactory metaStoreManagerFactory = services.metaStoreManagerFactory();
+ RealmContext realmContext = services.realmContext();
+ return new PolarisCallContext(
+ metaStoreManagerFactory.getOrCreateSessionSupplier(realmContext).get(),
+ services.polarisDiagnostics());
+ }
+
+ private PolarisAdminService setupPolarisAdminService(
+ PolarisMetaStoreManager metaStoreManager, PolarisCallContext callContext) {
+ RealmContext realmContext = services.realmContext();
+ return new PolarisAdminService(
+ CallContext.of(realmContext, callContext),
+ services.entityManagerFactory().getOrCreateEntityManager(realmContext),
+ metaStoreManager,
+ new SecurityContext() {
+ @Override
+ public Principal getUserPrincipal() {
+ return new AuthenticatedPolarisPrincipal(
+ new PrincipalEntity.Builder().setName("root").build(), Set.of("service_admin"));
+ }
+
+ @Override
+ public boolean isUserInRole(String role) {
+ return true;
+ }
+
+ @Override
+ public boolean isSecure() {
+ return false;
+ }
+
+ @Override
+ public String getAuthenticationScheme() {
+ return "";
+ }
+ },
+ new PolarisAuthorizerImpl(new DefaultConfigurationStore(Map.of())));
+ }
+
+ private PrincipalEntity createPrincipal(
+ PolarisMetaStoreManager metaStoreManager,
+ PolarisCallContext callContext,
+ String name,
+ boolean isFederated) {
+ return new PrincipalEntity.Builder()
+ .setFederated(isFederated)
+ .setName(name)
+ .setCreateTimestamp(Instant.now().toEpochMilli())
+ .setId(metaStoreManager.generateNewEntityId(callContext).getId())
+ .build();
+ }
+
+ private PrincipalRoleEntity createRole(
+ PolarisMetaStoreManager metaStoreManager,
+ PolarisCallContext callContext,
+ String name,
+ boolean isFederated) {
+ return new PrincipalRoleEntity.Builder()
+ .setId(metaStoreManager.generateNewEntityId(callContext).getId())
+ .setName(name)
+ .setFederated(isFederated)
+ .setProperties(Map.of())
+ .setCreateTimestamp(Instant.now().toEpochMilli())
+ .setLastUpdateTimestamp(Instant.now().toEpochMilli())
+ .build();
+ }
+
+ @Test
+ public void testCannotAddFederatedPrincipalToNonFederatedRole() {
+ PolarisMetaStoreManager metaStoreManager = setupMetaStoreManager();
+ PolarisCallContext callContext = setupCallContext(metaStoreManager);
+ PolarisAdminService polarisAdminService =
+ setupPolarisAdminService(metaStoreManager, callContext);
+
+ PrincipalEntity federatedPrincipal =
+ createPrincipal(metaStoreManager, callContext, "federated_id", true);
+ metaStoreManager.createPrincipal(callContext, federatedPrincipal);
+
+ PrincipalRoleEntity nonFederatedRole =
+ createRole(metaStoreManager, callContext, "non_federated_role", false);
+ EntityResult result =
+ metaStoreManager.createEntityIfNotExists(callContext, null, nonFederatedRole);
+ assertThat(result.isSuccess()).isTrue();
+
+ assertThatThrownBy(
+ () ->
+ polarisAdminService.assignPrincipalRole(
+ federatedPrincipal.getName(), nonFederatedRole.getName()))
+ .isInstanceOf(ValidationException.class);
+ }
+
+ @Test
+ public void testCannotAddNonFederatedPrincipalToFederatedRole() {
Review Comment:
It could make sense to use a parameterized test here. IIUC there is basically a table like the following:
| Principal Type | Principal Role Type | Can Attach |
| ------------- | ------------- | -- |
| Non-Federated | Non-Federated | Yes |
| Non-Federated | Federated | No |
| Federated | Non-Federated | No |
| Federated | Federated | No |
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add support for federated principal and role with block for manual role assignment [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #1353:
URL: https://github.com/apache/polaris/pull/1353#discussion_r2047914839
##########
service/common/src/main/java/org/apache/polaris/service/admin/PolarisAdminService.java:
##########
@@ -771,6 +771,9 @@ public PrincipalWithCredentials createPrincipal(PolarisEntity entity) {
PolarisAuthorizableOperation op = PolarisAuthorizableOperation.CREATE_PRINCIPAL;
authorizeBasicRootOperationOrThrow(op);
+ if (PolarisEntity.isFederated(entity)) {
Review Comment:
The design doc does talk about this, but I do not recall firm decisions on mentioned alternatives :)
Even if we create entities for federated principals, consider a use case when someone creates a principal called `A` (non-federated) and later someone else adds federation and federated principal with name `A` comes in (or the other way around). Where is this conflict resolved? This is what I mean be "manager".
Another use case: a federated principal `B` is created thought some IdP integration logic and brings in some properties from the IdP. A user updates principal `B` via Admin API. The doc mentions that being able to update principal properties is desirable, but where do we resolve conflicts between updated from the API and from the integration side?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add support for federated principal and role with block for manual role assignment [polaris]
Posted by "collado-mike (via GitHub)" <gi...@apache.org>.
collado-mike merged PR #1353:
URL: https://github.com/apache/polaris/pull/1353
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add support for federated principal and role with block for manual role assignment [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #1353:
URL: https://github.com/apache/polaris/pull/1353#discussion_r2047887344
##########
spec/polaris-management-service.yml:
##########
@@ -1089,6 +1089,10 @@ components:
clientId:
type: string
description: The output-only OAuth clientId associated with this principal if applicable
+ federated:
Review Comment:
Thanks for the clarification! The description of the property LGTM. However, I'm not not sure about exposing federated Principals in the Polaris API.
Granted, we have to expose federated Principal Roles in order to allow assignment to Catalog Roles, but do Principals have to be exposed?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add support for federated principal and role with block for manual role assignment [polaris]
Posted by "collado-mike (via GitHub)" <gi...@apache.org>.
collado-mike commented on code in PR #1353:
URL: https://github.com/apache/polaris/pull/1353#discussion_r2045050829
##########
polaris-core/src/main/java/org/apache/polaris/core/entity/PrincipalRoleEntity.java:
##########
@@ -38,19 +38,19 @@ public static PrincipalRoleEntity of(PolarisBaseEntity sourceEntity) {
public static PrincipalRoleEntity fromPrincipalRole(PrincipalRole principalRole) {
return new Builder()
.setName(principalRole.getName())
+ .setFederated(principalRole.getFederated())
.setProperties(principalRole.getProperties())
.build();
}
public PrincipalRole asPrincipalRole() {
- PrincipalRole principalRole =
- new PrincipalRole(
- getName(),
- getPropertiesAsMap(),
- getCreateTimestamp(),
- getLastUpdateTimestamp(),
- getEntityVersion());
- return principalRole;
+ return new PrincipalRole(
+ getName(),
+ PolarisEntity.isFederated(this),
Review Comment:
Normally, I'd agree, but here I am trying to stick to the convention of keeping the entity-specific args at the front and the general, id/created/updated-at timestamps at the end. That's the convention that every entity type follows.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add support for federated principal and role with block for manual role assignment [polaris]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on code in PR #1353:
URL: https://github.com/apache/polaris/pull/1353#discussion_r2067155086
##########
polaris-core/src/main/java/org/apache/polaris/core/entity/PrincipalRoleEntity.java:
##########
@@ -65,6 +65,13 @@ public Builder(PrincipalRoleEntity original) {
super(original);
}
+ public Builder setFederated(Boolean isFederated) {
+ if (isFederated != null && isFederated) {
Review Comment:
I agree with both @dimas-b and @eric-maynard that this looks error-prone.
Is there a compelling reason to use a "trilean" logic (null/false/true) here? If so, maybe something like this would be less error-prone:
```java
public Builder setFederated(Boolean isFederated) {
if (isFederated != null) {
internalProperties.put(PolarisEntityConstants.FEDERATED_ENTITY, isFederated.toString());
} else {
internalProperties.remove(PolarisEntityConstants.FEDERATED_ENTITY);
}
return this;
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add support for federated principal and role with block for manual role assignment [polaris]
Posted by "collado-mike (via GitHub)" <gi...@apache.org>.
collado-mike commented on code in PR #1353:
URL: https://github.com/apache/polaris/pull/1353#discussion_r2069851856
##########
polaris-core/src/main/java/org/apache/polaris/core/entity/PrincipalRoleEntity.java:
##########
@@ -65,6 +65,13 @@ public Builder(PrincipalRoleEntity original) {
super(original);
}
+ public Builder setFederated(Boolean isFederated) {
+ if (isFederated != null && isFederated) {
Review Comment:
TBH, I don't see a reason, beyond building from an existing entity, to support setting a value in a builder twice. Modifying an existing entity is one reason to allow flexibility, but a builder has no reason for such flexibility. But, it's a minor point and I'm fine to concede if it allows us to move things along.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add support for federated principal and role with block for manual role assignment [polaris]
Posted by "collado-mike (via GitHub)" <gi...@apache.org>.
collado-mike commented on PR #1353:
URL: https://github.com/apache/polaris/pull/1353#issuecomment-2852071767
> I think we have to remove the ability to persist federated principals/roles. I'll elaborate on the dev-ML.
This has already been discussed on the mailing list. There is no persistence of the federated principals - there is a test that explicitly illustrates this. There is also no persistence of the grants between federated principals and roles. Tests exist to illustrate this as well.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add support for federated principal and role with block for manual role assignment [polaris]
Posted by "snazy (via GitHub)" <gi...@apache.org>.
snazy commented on PR #1353:
URL: https://github.com/apache/polaris/pull/1353#issuecomment-2862154954
> > As mentioned on the ML, there's no need to have that "federated" flag on principals, which is still there.
>
> Per Dmitri's comment above
>
> > As discussed, having API-level "federated" flags does not mean that Polaris will necessarily import Principal entities from an external IdP, but it does have certain symmetry with the PrincipalRole's federated flag, so it probably makes sense to add both of them at the same time.
I do not understand this statement. If that "flag" can never be true, what's the reason for having code for that case?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add support for federated principal and role with block for manual role assignment [polaris]
Posted by "collado-mike (via GitHub)" <gi...@apache.org>.
collado-mike commented on PR #1353:
URL: https://github.com/apache/polaris/pull/1353#issuecomment-2887358780
> PR LGTM, but there are CI failures...
Oh - it's running against the latest main. I kept running it locally, but after I rebase on main, I see the error
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add support for federated principal and role with block for manual role assignment [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #1353:
URL: https://github.com/apache/polaris/pull/1353#discussion_r2043021446
##########
polaris-core/src/main/java/org/apache/polaris/core/entity/PolarisEntity.java:
##########
@@ -182,6 +182,12 @@ public static List<NameAndId> toNameAndIdList(List<EntityNameLookupRecord> entit
.orElse(null);
}
+ public static boolean isFederated(PolarisBaseEntity entity) {
Review Comment:
I tend to agree with @eric-maynard 's point here
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add support for federated principal and role with block for manual role assignment [polaris]
Posted by "eric-maynard (via GitHub)" <gi...@apache.org>.
eric-maynard commented on code in PR #1353:
URL: https://github.com/apache/polaris/pull/1353#discussion_r2045148978
##########
polaris-core/src/main/java/org/apache/polaris/core/entity/PrincipalRoleEntity.java:
##########
@@ -65,6 +65,13 @@ public Builder(PrincipalRoleEntity original) {
super(original);
}
+ public Builder setFederated(Boolean isFederated) {
+ if (isFederated != null && isFederated) {
Review Comment:
Why should it not be supported? It sounds like you're saying that `.setFederated(false)` is never a valid call, in which case `setFederated` shouldn't take a boolean arg. Perhaps you are even looking for something like `buildFederated`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add support for federated principal and role with block for manual role assignment [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #1353:
URL: https://github.com/apache/polaris/pull/1353#discussion_r2045315911
##########
service/common/src/main/java/org/apache/polaris/service/admin/PolarisAdminService.java:
##########
@@ -771,6 +771,9 @@ public PrincipalWithCredentials createPrincipal(PolarisEntity entity) {
PolarisAuthorizableOperation op = PolarisAuthorizableOperation.CREATE_PRINCIPAL;
authorizeBasicRootOperationOrThrow(op);
+ if (PolarisEntity.isFederated(entity)) {
Review Comment:
to clarify: My thinking was that the fact that make those entities "not creatable" via the Admin API is not the presence of a specific property (i.e. "federated") but the fact that they are managed in a different way (via IdP integrations, I believe).
Can this be made more explicit in code? I do not have a solid suggestion, but I wonder if some sort of a "manager" class might be suitable here. That "manager" would then make the decision about allowing or not allowing changes via Admin API. WDYT?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add support for federated principal and role with block for manual role assignment [polaris]
Posted by "eric-maynard (via GitHub)" <gi...@apache.org>.
eric-maynard commented on code in PR #1353:
URL: https://github.com/apache/polaris/pull/1353#discussion_r2038929905
##########
polaris-core/src/main/java/org/apache/polaris/core/entity/PrincipalRoleEntity.java:
##########
@@ -38,19 +38,19 @@ public static PrincipalRoleEntity of(PolarisBaseEntity sourceEntity) {
public static PrincipalRoleEntity fromPrincipalRole(PrincipalRole principalRole) {
return new Builder()
.setName(principalRole.getName())
+ .setFederated(principalRole.getFederated())
.setProperties(principalRole.getProperties())
.build();
}
public PrincipalRole asPrincipalRole() {
- PrincipalRole principalRole =
- new PrincipalRole(
- getName(),
- getPropertiesAsMap(),
- getCreateTimestamp(),
- getLastUpdateTimestamp(),
- getEntityVersion());
- return principalRole;
+ return new PrincipalRole(
+ getName(),
+ PolarisEntity.isFederated(this),
Review Comment:
If we have to add an argument, it's best to add it at the end
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add support for federated principal and role with block for manual role assignment [polaris]
Posted by "eric-maynard (via GitHub)" <gi...@apache.org>.
eric-maynard commented on code in PR #1353:
URL: https://github.com/apache/polaris/pull/1353#discussion_r2038929198
##########
polaris-core/src/main/java/org/apache/polaris/core/entity/PolarisEntity.java:
##########
@@ -182,6 +182,12 @@ public static List<NameAndId> toNameAndIdList(List<EntityNameLookupRecord> entit
.orElse(null);
}
+ public static boolean isFederated(PolarisBaseEntity entity) {
Review Comment:
If only a subset of entity types support this, I don't think it should be present in the base type `PolarisEntity`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add support for federated principal and role with block for manual role assignment [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #1353:
URL: https://github.com/apache/polaris/pull/1353#discussion_r2047919467
##########
spec/polaris-management-service.yml:
##########
@@ -1089,6 +1089,10 @@ components:
clientId:
type: string
description: The output-only OAuth clientId associated with this principal if applicable
+ federated:
Review Comment:
As @eric-maynard suggested, it might be more efficient to review the ML / doc discussion. GH comment threads are less convenient for long discussions.
##########
spec/polaris-management-service.yml:
##########
@@ -1089,6 +1089,10 @@ components:
clientId:
type: string
description: The output-only OAuth clientId associated with this principal if applicable
+ federated:
Review Comment:
As @eric-maynard suggested, it might be more efficient to revive the ML / doc discussion. GH comment threads are less convenient for long discussions.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add support for federated principal and role with block for manual role assignment [polaris]
Posted by "collado-mike (via GitHub)" <gi...@apache.org>.
collado-mike commented on code in PR #1353:
URL: https://github.com/apache/polaris/pull/1353#discussion_r2047857076
##########
polaris-core/src/main/java/org/apache/polaris/core/entity/PrincipalRoleEntity.java:
##########
@@ -65,6 +65,13 @@ public Builder(PrincipalRoleEntity original) {
super(original);
}
+ public Builder setFederated(Boolean isFederated) {
+ if (isFederated != null && isFederated) {
Review Comment:
I'm saying the code example you suggested is a programming error. Ideally, that would be handled by the compiler, but an exception would also work.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Add support for federated principal and role with block for manual role assignment [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #1353:
URL: https://github.com/apache/polaris/pull/1353#discussion_r2067146918
##########
polaris-core/src/main/java/org/apache/polaris/core/entity/PrincipalRoleEntity.java:
##########
@@ -65,6 +65,13 @@ public Builder(PrincipalRoleEntity original) {
super(original);
}
+ public Builder setFederated(Boolean isFederated) {
+ if (isFederated != null && isFederated) {
Review Comment:
@collado-mike : this is the only substantial issue with the current state if this PR from my POV.
The other topics got clarified in ML and doc comments, I think the PR is good to merge after we resolve the setter issue.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org