Migrate all authorizeOrThrow(principal, ...) callers to authorize(state, request) and remove the legacy overloads
- 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
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