Include user-defined attributes when constructing PolarisPrincipal from PrincipalEntity (#4291)#4292
Include user-defined attributes when constructing PolarisPrincipal from PrincipalEntity (#4291)#4292sneethiraj wants to merge 5 commits into
Conversation
…erUserStore - that can be used in Policy Conditions.
sneethiraj
changed the title
sneethiraj
changed the title
|
Thanks for the fix! The approach of merging both Map<String, String> allProperties = new HashMap<>(principalEntity.getInternalPropertiesAsMap());
allProperties.putAll(principalEntity.getPropertiesAsMap());
With this order, user-defined properties take precedence over internal ones on key conflicts. This means a user could potentially supply a custom attribute like client_id and silently overwrite the system-managed internal value, which could be a security concern.
Would it be safer to reverse the order so internal properties always win?
Map<String, String> allProperties = new HashMap<>(principalEntity.getPropertiesAsMap());
allProperties.putAll(principalEntity.getInternalPropertiesAsMap());
Also noticed the checklist has Fixes # without the issue number — should be Fixes #4291. |
…e internal properties take precedence over user-defined properties during merge
|
@Vishwaspatel2401 Thanks for the suggestion — good catch on the merge order. Also updated the PR description to include Fixes #4291. |
| RangerAccessContext context = new RangerAccessContext(SERVICE_TYPE, serviceName); | ||
| RangerMultiAuthzRequest authzRequest = | ||
| new RangerMultiAuthzRequest(userInfo, accessInfos, context); | ||
| RangerUtils.setUserAttribsInRequestContext(userInfo, context); |
There was a problem hiding this comment.
the context is getting modified after the request is created? Is the request holding a pointer to the context?
|
|
||
| protected String getUserToken(String userName) { | ||
| protected String getUserToken(String userName, Properties userProperties) { | ||
| String token = user2Token.get(userName); |
There was a problem hiding this comment.
Seems odd that we are adding userProperties to this test method but we're not actually using that field when asking the token cache for the token...?
| import io.quarkus.test.junit.QuarkusTest; | ||
| import io.quarkus.test.junit.TestProfile; | ||
| import io.restassured.http.ContentType; | ||
| import java.util.Properties; |
There was a problem hiding this comment.
java.util.Properties is used here (and in the base class) purely as a Map<String, String>. would prefer using Map instead to standardize (since that's what we're using elsewhere in this PR) rather than having Properties's Hashtable causing any little-known side-effects later on.
| public class RangerABACWithUserAttrIT extends RangerIntegrationTestBase { | ||
|
|
||
| @Test | ||
| void getCatalogListsWithoutAuthorization() { |
There was a problem hiding this comment.
Nit: getCatalogListsWithoutAuthorization / getCatalogListsWithAuthorization are ambiguous — they could be read as "without an Authorization header", which is a different scenario. Something like denyAccessForPrincipalWithNonMatchingRegionAttribute / allowAccessForPrincipalWithMatchingRegionAttribute would make the intent immediately clear.
| * roles. | ||
| * | ||
| * <p>The created principal will have the same ID and name as the {@link PrincipalEntity}, and its | ||
| * properties will be derived from the internal properties of the entity. |
There was a problem hiding this comment.
Can we change this Javadoc for both functions that were changed?
| Map.Entry::getKey, e -> e.getValue() == null ? "" : e.getValue().toString())); | ||
| userAttrMapping.put(userInfo.getName(), attributeMap); | ||
| store.setUserAttrMapping(userAttrMapping); | ||
| if (rangerAccessContext.getAdditionalInfo() == null) { |
There was a problem hiding this comment.
nit: this has a side effect on rangerAccessContext, it modifies it's state. We do this to avoid passing null to RangerAccessRequestUtil.setRequestUserStoreInContext?
github-project-automation
Bot
moved this from PRs In Progress
to Done
in Basic Kanban Board
5 participants
This change provides a fix for issue #4291 ; It updates the construction of PolarisPrincipal from PrincipalEntity to include user-defined attributes in addition to internal attributes.
Currently, PolarisPrincipal is initialized using:
principalEntity.getInternalPropertiesAsMap()
This results in only internal attributes (such as client_id) being available in the PolarisPrincipal, while user-defined attributes supplied during principal creation are not propagated.
This PR updates the implementation to use:
principalEntity.getPropertiesAsMap() along with principalEntity.getInternalPropertiesAsMap()
so that both internal and user-defined attributes are available during request processing.
Fixes: #4291
Problem
When a principal is created with user-defined attributes (for example, region=northamerica), those attributes are successfully stored in PrincipalEntity.
However, during authentication and request handling, the constructed PolarisPrincipal only contains internal properties, and user-defined attributes are not available.
This prevents downstream components from accessing user attributes that were explicitly configured during principal creation.
Testing
Checklist
CHANGELOG.md(if needed)site/content/in-dev/unreleased(if needed)