Skip to content

Authorizer: expose the required level of detail #3228

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.

Sign up for GitHub

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

Open
snazy wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Authorizer: expose the required level of detail #3228

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 15 additions & 0 deletions ...th/opa/impl/src/main/java/org/apache/polaris/extension/auth/opa/OpaPolarisAuthorizer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,21 @@ public OpaPolarisAuthorizer(
this.objectMapper = objectMapper;
}

@Override
public boolean requiresPrincipalRoles() {
return false;
}

@Override
public boolean requiresCatalogRoles() {
return false;
}

@Override
public boolean requiresResolvedEntities() {
return false;
}
Comment on lines +98 to +111
Copy link
Contributor

@singhpk234 singhpk234 Dec 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder this comes under the use case Delegated AuthZ, should we have another interface for it entirely to undertand whats required for delegation ?

Copy link
Member Author

@snazy snazy Dec 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nothing to do with delegation. Just to get rid of unnecessary work to get data (principal/catalog roles, grant records) that some implementations just do not need.

Copy link
Contributor

@dimas-b dimas-b Dec 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@singhpk234 : I'm not sure what you mean by "delegated" AuthZ 🤔 The PolarisAuthorizer interface has multiple implementations, but I do not think this is "delegation" per se... Maybe I'm missing some context.

As far as the different (two in OSS) implementations of PolarisAuthorizer are concerns, they have different requirements on the amount of input data they require for making AuthZ decisions. These new interface methods are intended (as far as I understand) to allow other Polaris classes to perform less work, if some input data is not needed by a particular authorizer.

I suppose there will be other related code changes that expose the benefits of the new methods added in this PR.

Copy link
Contributor

@singhpk234 singhpk234 Dec 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure what you mean by "delegated" AuthZ 🤔 The PolarisAuthorizer interface has multiple implementations, but I do not think this is "delegation" per se... Maybe I'm missing some context

I see, my understanding the reason why we don't use the principalRoles / catalogRoles etc as the grants etc are managed by external system which is OPA here which gives us an AuthZ decision (YES | NO) hence the Authorization is delegated here as otherwise PolarisAuthZ would have required these stuff as grants etc are done based on that. This is what i had in mind, in context of above so here is what my thought process was considering the code change :

public interface SupportsDelegatedAuthZ { 
  default boolean requiresPrincipalRoles() {
    return false;
  }

  default boolean requiresCatalogRoles() {
    return false;
  }

   default boolean requiresResolvedEntities() {
    return false;
  } 
 }
OPAAuthZ implements PolarisAuthZ, SupportsDelegateAuthZ {
  // overrides
}

THis way persistence who wanna optimize to not look up prinicipal role and catalog role can first check SupportsDelegateAuthZ and skip the look ups accordingly.

Again i am thinking out loud here !

Copy link
Member Author

@snazy snazy Dec 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Short and precise: instanceof is a no-go in CDI as it doesn't work. It would prevent making PolarisAuthorizer request scoped in the future.

The long-term solution IMO is to have a different API/SPI structure for these things, but I do not have thought about it at all.

Copy link
Contributor

@dimas-b dimas-b Dec 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@singhpk234 : Thanks for clarifying the meaning of "delegated authZ" in this context. I think I understand what you mean, although, I would not personally use the term "delegation" here as in the content of Auth N/Z it has a different meaning usually (acting on behalf of a different identity).

Re: SupportsDelegatedAuthZ the implementation you propose is mostly equivalent to the state of the code in this PR, IMHO.

However, there is a downside - as @snazy mentions, the callers would have to perform instanceof checks on the PolarisAuthorizer instances, which will complicate the code on the caller side. Plus, CDI will become much harder, because injection normally guarantees only the declared type of the variable in runtime. Sub-classing is not guaranteed to be exposed in injected objects in CDI... Sub-classes may be exposes "as is" or they may not be (e.g. proxies may be injected). All in all, this approach increases code complexity.

If you have strong reasons to avoid introducing even byte-code compatible changes to PolarisAuthorizer (as in this PR). I'd proposed to move the methods from SupportsDelegatedAuthZ into a (new) AuthorizationHints interface to be produced from PolarisAuthorizerFactory and used at call sites that can optimize execution based on the information the new interface provides.

@snazy @singhpk234 : WDYT?

Copy link
Contributor

@singhpk234 singhpk234 Dec 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

strong reasons to avoid introducing even byte-code compatible changes

TBH i don't have strong concerns for introducing byte-code compatibility changes, my main consideration were the version upgrade should be smooth and the API should well documented so the downstream users can override it accordingly which i think is already addressed in the recent commit so I am good from that POV as this is definitely an integration point.

I'd proposed to move the methods from SupportsDelegatedAuthZ into a (new) AuthorizationHints interface to be produced from PolarisAuthorizerFactory and used at call sites that can optimize execution based on the information the new interface provides.

I leave this to the your better judgement ! I don't have a strong for or against opinion on this ! since the concerns i had are already addressed.

cc @HonahX can you also please weigh in on this discussion

Copy link
Contributor

@dimas-b dimas-b Dec 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If adding default methods to the existing interface is not a concern, my preference would be to go with the current state of this PR 🙂


/**
* Authorizes a single target and secondary entity for the given principal and operation.
*
Expand Down
36 changes: 36 additions & 0 deletions polaris-core/src/main/java/org/apache/polaris/core/auth/PolarisAuthorizer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,42 @@
/** Interface for invoking authorization checks. */
public interface PolarisAuthorizer {

/**
* Whether the implementation expects Polaris principal roles to be present in the {@code
* activatedEntities} parameters of the {@link #authorizeOrThrow(PolarisPrincipal, Set,
* PolarisAuthorizableOperation, PolarisResolvedPathWrapper, PolarisResolvedPathWrapper)}
* functions.
*
* <p>If {@code false}, call sites may choose to not pass principal roles.
*/
default boolean requiresPrincipalRoles() {
return true;
}

/**
* Whether the implementation expects Polaris catalog roles to be present in the {@code
* activatedEntities} parameters of the {@link #authorizeOrThrow(PolarisPrincipal, Set,
* PolarisAuthorizableOperation, PolarisResolvedPathWrapper, PolarisResolvedPathWrapper)}
* functions.
*
* <p>If {@code false}, call sites may choose to not pass catalog roles.
*/
default boolean requiresCatalogRoles() {
return true;
}

/**
* Whether the implementation expects the {@link
* org.apache.polaris.core.persistence.ResolvedPolarisEntity}s in the {@link
* PolarisResolvedPathWrapper} instances of the {@code target} and {@code secondary} parameters to
* contain grant records information.
*
* <p>If {@code false}, call sites may choose to not pass grant records.
*/
default boolean requiresResolvedEntities() {
return true;
}

void authorizeOrThrow(
@Nonnull PolarisPrincipal polarisPrincipal,
@Nonnull Set<PolarisBaseEntity> activatedEntities,
Expand Down