apache / apache/polaris

Migrate all authorizeOrThrow(principal, ...) callers to authorize(state, request) and remove the legacy overloads

Open
#5,171 1 comment 0 reactions 0 assignees View on GitHub
stale
Dominant language
Java
Stars
2.1k
Forks
522
Avg merge
1d 22h
Merged PRs (30d)
137

Description

## Overview

`PolarisAuthorizer` exposes a decision-native entry point, `authorize(AuthorizationState, AuthorizationRequest)`, and a thin throwing convenience wrapper, `authorizeOrThrow(AuthorizationState, AuthorizationRequest)`. However, most service-layer call sites still use the legacy resolved-path overloads:

```java
void authorizeOrThrow(PolarisPrincipal, Set, PolarisAuthorizableOperation,
PolarisResolvedPathWrapper target, PolarisResolvedPathWrapper secondary);
void authorizeOrThrow(PolarisPrincipal, Set, PolarisAuthorizableOperation,
List targets, List secondaries);
```

Per the discussion in #5034, `authorize(state, request)` is intended to be the main method. This issue tracks migrating all callers to the decision-native path and removing the legacy overloads.

## Goal / Scope

- Move every caller of the legacy `authorizeOrThrow(principal, activatedEntities, op, target(s), secondary(ies))` overloads to `authorize(state, request)` / `authorizeOrThrow(state, request)`.
- Keep `authorizeOrThrow(AuthorizationState, AuthorizationRequest)` (the thin throwing wrapper) — most call sites just want to throw on denial.
- Remove the two legacy resolved-path overloads from `PolarisAuthorizer`, `PolarisAuthorizerImpl`, and the Ranger/OPA implementations once no callers remain.

## Known blocker (why this needs prep work)

The two paths resolve entities differently. The legacy overloads take an already-resolved `PolarisResolvedPathWrapper`, whereas `authorize(state, request)` re-derives each securable from the request intents by looking them up in the resolution manifest (`PolarisAuthorizerImpl.getResolvedSecurable()` → `getResolvedTopLevelEntity(name, type)` for top-level entities, `getResolvedPath(...)` for paths).

Call sites that resolved their target the legacy way but never registered the securable's name+type in the manifest will fail re-resolution with:

```
IllegalStateException: never_registered_top_level_name_and_type_for_resolved_entity
```

So this is not a mechanical find-and-replace: each call site must first populate the resolution manifest so re-resolution by name/type succeeds. This is most impactful for top-level entities (catalog / principal / principal-role / catalog-role) and secondary (grant) targets.

## Approximate call-site inventory

- `CatalogHandler` — ~9 sites (namespace + table-like, path-based; lowest risk)
- `PolicyCatalogHandler` — ~3 sites (path-based)
- `PolarisAdminService` — ~10 sites (top-level + grant/secondary targets; highest risk)

## Planned PR series

- [ ] **PR 1 — CatalogHandler, table-like single-target family** (`authorizeResolvedBasicTableLikeOperationOrThrow`): #5170
- [ ] PR 2 — CatalogHandler, namespace + multi-target/rename sites
- [ ] PR 3 — PolicyCatalogHandler sites
- [ ] PR 4–6 — PolarisAdminService, split by securable kind (single top-level, catalog-role/namespace paths, then grant/revoke with secondaries)
- [ ] Final PR — remove the legacy `authorizeOrThrow(principal, …)` overloads

Each non-final PR is behavior-preserving (still throws on denial), so existing authorization tests act as the safety net.

Related: #5034

Contributor guide

Open the contributing guide

Research direction

Start with CatalogHandler and the PR 1 scope in #5170, then inspect PolicyCatalogHandler, PolarisAdminService, PolarisAuthorizer, PolarisAuthorizerImpl, and the Ranger/OPA implementations. Trace how each caller populates the resolution manifest before migrating to authorize(state, request) or authorizeOrThrow(state, request). Run the existing authorization tests; done means all legacy callers are migrated and the two resolved-path overloads can be removed.

Written by the indexing model from the issue text.

Assessment

Tech stack
java
Domain
authorization, backend
Issue type
Refactor
Difficulty
4/5
Estimated time
3-5 days
Activity status
Active
Clarity
Clearly specified
Newbie friendliness
45/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.