Core: include missing privilege and target entity in PolarisAuthorizer 403 messages by iprithv · Pull Request #4406 · apache/polaris

iprithv · 2026-05-11T21:56:59Z

PolarisAuthorizerImpl.authorizeOrThrow today produces only:

Principal 'alice' with activated PrincipalRoles '[reader]' and activated grants via '[]' is not authorized for op CREATE_TABLE_DIRECT

the operator has no way to tell which privilege is missing or on which entity, and routinely has to grep the codebase to figure out which grant to add. The explicit TODO at the old short-circuit in isAuthorized ("Collect missing privileges to report all at the end and/or return to code that throws NotAuthorizedException for more useful messages.") calls this out.

and this is a diagnostic improvement, not a behavioural change, every operation that was authorized before is still authorized, every one denied is still denied. Only the text of the 403 changes.

Principal 'alice' with activated PrincipalRoles '[reader]' and activated grants via '[]' is not authorized for op CREATE_TABLE_DIRECT; missing TABLE_CREATE on NAMESPACE 'ns1'

secondary failures are labelled (secondary) so operators can tell which side of a RENAME_TABLE-style op needs grants. Multiple missing privileges are joined with , in one message.

new SPI's AuthorizationDecision.deny(e.getMessage()) in PolarisAuthorizerImpl.authorize() inherits the richer text automatically.

dimas-b

Thanks for your contribution, @iprithv !

operator has no way to tell which privilege is missing or on which entity [...]

I believe a more robust solution would be to log these details and return a matching unique ID of that message to the client. This way the Polaris Admin can correlated the client-side error to AuthZ grants without exposing any extra (potentially sensitive) information to clients.

That said, please open a corresponding discussion on the dev ML. I think all AuthZ changes should be discussed on dev.

dimas-b · 2026-05-12T01:33:41Z

+          findMissingPrivileges(polarisPrincipal, activatedEntities, authzOp, targets, secondaries);
+      if (!missing.isEmpty()) {
+        throw new ForbiddenException(
+            "Principal '%s' with activated PrincipalRoles '%s' and activated grants via '%s' is not authorized for op %s; missing %s",


I do not think exposing authZ details to the client in case of denied responses is a good idea. This information can potentially be used by malicious clients to further attacks.

Agreed with @dimas-b. Deny responses should just tell the caller that they're denied, not the shape of the wall they're hitting. Instead, logging the missing privileges should be good.

ah ok, I get it about not exposing authZ details to clients. updated. thanks!

github-actions · 2026-06-12T02:17:23Z

This PR is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 5 days.

Addresses security review feedback on PR apache#4406: - Detailed missing privilege info (e.g. 'TABLE_CREATE on NAMESPACE ns1') is now logged at INFO level server-side instead of being included in the client-facing ForbiddenException message. - The client-facing 403 response remains generic to avoid leaking authorization metadata to untrusted clients, per SECURITY-THREAT-MODEL.md. - Operators can correlate client errors to server logs via the existing X-Request-ID header already stamped on every request. - findMissingPrivileges() and MissingPrivilege record are retained for server-side logging and debugging. Fixes apache#4406

dimas-b

Functional changes LGTM, but it looks like this PR accumulated some unrelated changes? 🤔

dimas-b · 2026-07-10T01:42:04Z

This file looks odd in current PR 🤔 Would you mind re-applying the functional commits to main from scratch?

Good catch — those were accidental gradle/server-test-runner/.gradle cache artifacts that should never have been committed (they're gitignored).

Re-applied the functional changes onto current main from scratch in a single clean commit (review changes). The PR now only touches:

PolarisAuthorizerImpl.java

PolarisAuthorizerImplTest.java

CHANGELOG.md (under Unreleased)

sure, done. thanks!

dimas-b · 2026-07-10T01:46:01Z

+        LOGGER.info(
+            "Authorization denied for principal '{}' on operation '{}': missing {}",
+            polarisPrincipal.getName(),
+            authzOp,
+            missingDetails);
+        throw new ForbiddenException(
+            "Principal '%s' with activated PrincipalRoles '%s' and activated grants via '%s' is not authorized for op %s",


This approach looks reasonable to me. I think we can merge "as is".

However, as I commented earlier, as more admin-friendly approach might be to add a UUID to this message and also log it on line 942. Whoever needs to investigate the authZ denial, can use the UUID to find related log messages easily (hopefully).

I'll leave it up to your @iprithv to choose which way to go for this PR.

... or use request ID as @sungwy suggested: https://lists.apache.org/thread/0l1x090nmxbbrght3jrgdzqdp24fbbsx

Thanks — going with the existing request-ID correlation path for this PR (as suggested by @sungwy on the ML).

Default logging already includes %X{requestId} via MDC (LoggingMDCFilter + RequestIdFilter), and the client already receives X-Request-ID. Operators can match a client-side 403 to the server-side INFO log that lists the missing privileges without putting authZ details (or a second synthetic UUID) into the response body.

Leaving the client message generic as-is; happy to follow up later if we want the request ID duplicated into the ForbiddenException text as well.

Agreed — using the existing request ID / X-Request-ID is enough here. Default log format already stamps requestId from MDC, so no extra UUID in this PR.

ah okay, went with existing request-ID correlation. default logs include MDC requestId.. client gets X-Request-ID...leaving client message generic...thanks!

Re-apply auth-failure missing-privilege logging onto main from scratch: server-side INFO logs with missing privilege details, generic client 403, and drop accidentally committed gradle/server-test-runner/.gradle cache files.

flyrain

+1 Thanks @iprithv

github-project-automation Bot added this to Basic Kanban Board May 11, 2026

github-project-automation Bot moved this to PRs In Progress in Basic Kanban Board May 11, 2026

dimas-b reviewed May 12, 2026

View reviewed changes

github-actions Bot added the stale label Jun 12, 2026

github-actions Bot closed this Jun 17, 2026

github-project-automation Bot moved this from PRs In Progress to Done in Basic Kanban Board Jun 17, 2026

dimas-b reopened this Jul 7, 2026

github-project-automation Bot moved this from Done to PRs In Progress in Basic Kanban Board Jul 7, 2026

iprithv requested review from dimas-b and flyrain July 8, 2026 02:11

github-actions Bot removed the stale label Jul 8, 2026

dimas-b reviewed Jul 10, 2026

View reviewed changes

iprithv force-pushed the improve/auth-failure-missing-privilege-details branch from a89e0b8 to 801b2af Compare July 10, 2026 05:47

iprithv requested a review from dimas-b July 10, 2026 06:08

review changes

89b5e39

Re-apply auth-failure missing-privilege logging onto main from scratch: server-side INFO logs with missing privilege details, generic client 403, and drop accidentally committed gradle/server-test-runner/.gradle cache files.

iprithv force-pushed the improve/auth-failure-missing-privilege-details branch from 801b2af to 89b5e39 Compare July 10, 2026 06:10

dimas-b approved these changes Jul 10, 2026

View reviewed changes

github-project-automation Bot moved this from PRs In Progress to Ready to merge in Basic Kanban Board Jul 10, 2026

flyrain approved these changes Jul 10, 2026

View reviewed changes

flyrain merged commit 6533382 into apache:main Jul 10, 2026
25 checks passed

github-project-automation Bot moved this from Ready to merge to Done in Basic Kanban Board Jul 10, 2026

shyundev mentioned this pull request Aug 13, 2026

Return 503 instead of 500 when the metastore fails during authentication #5247

Merged

6 tasks done

shyundev mentioned this pull request Aug 24, 2026

Return a generic 503 message when the metastore fails during authentication #5363

Merged

5 of 6 tasks

Conversation

iprithv commented May 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dimas-b left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Jun 12, 2026

Uh oh!

dimas-b left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dimas-b Jul 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

flyrain left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

iprithv commented May 11, 2026 •

edited

Loading

dimas-b Jul 10, 2026 •

edited

Loading