eveniota opened a new pull request, #13708:
URL: https://github.com/apache/trafficserver/pull/13708

   Fixes #13607 
   
   ### Summary
   The `access_control` plugin previously parsed and exposed a `scope` claim 
and supported the `--invalid-scope-status-code` configuration option, but 
`AccessToken::validate()` never evaluated the scope against incoming request 
paths. Consequently, any valid token authorized access to every resource 
covered by a remap rule, and setting `--invalid-scope-status-code` produced no 
effect.
   This PR finishes the feature by implementing path-prefix scope validation on 
normalized path segments, providing per-resource granularity while remaining 
fully backward compatible with existing tokens.
   
   ### Design Approach (per #13607 suggestion)
   
   This PR implements the minimal backward-compatible approach:
   1. Path-prefix matching on normalized segments
   2. Absent/empty scope = unrestricted (backward compatible)
   3. Scope check in separate function after token validation
   
   These choices are open to feedback; see Design Decisions section below.
   
   ### Key Requirements & Design Decisions
   
   1. **Clean Separation of Concerns (Plumbing):**
        - Kept `AccessToken::validate()` focused purely on token cryptographic 
integrity, semantics, and timing.
        - Implemented scope comparison as a separate helper function, 
`validateScope(requestPath, scope)`, called in `enforceAccessControl()` 
immediately after `token->validate()` succeeds.
        - When out of scope, the transaction state is set to `OUT_OF_SCOPE`, 
correctly triggering `--invalid-scope-status-code`  (default: 403) and 
suppressing subject header extraction.
   
   2. **Matching Semantics (Normalized Path Segments):**
        - Matching is performed as a **path-prefix on normalized segments**.
        - Handles trailing slashes, redundant slashes, and ATS's 
`TSUrlPathGet()` format (which omits leading slashes).
        - Enforces directory segment boundaries so that a token scoped to 
`/reports/` (or `/reports`) authorizes `/reports/2026/` but strictly rejects 
sibling paths like `/reports2/` or `/reports_backup`.
   
   3. **Backward Compatibility (Absent / Empty Scope):**
        - An absent or empty `scope` claim is treated as **unrestricted**. 
Existing tokens in the field continue to function without disruption.
   
   4. **Resource Granularity vs Target Audience (`sub` claim):**
        - Retains the existing model where `sub` represents target audience 
(e.g. `frogs-in-a-well`), while adding the missing per-resource granularity 
(e.g. `scope="/reports/2026/"`).
   
   ### What Changed
   
   * **Core Matching Logic (access_control.cc, access_control.h):**
     - Declared and implemented `validateScope(StringView requestPath, 
StringView scope)`.
     - Added helper `normalizePath()` to ensure leading slashes, collapse 
consecutive slashes, and normalize trailing slashes.
     - Updated `@todo` comments regarding scope validation.
   
   * **Transaction Enforcement (plugin.cc):**
     - Inside `enforceAccessControl()`, extracted the request path via 
`TSUrlPathGet()` and validated it against `token->getScope()`.
     - On failure, sets `data->_vaState = OUT_OF_SCOPE` and invokes 
`handleInvalidToken()`.
   
   * **Unit Tests (test_access_control.cc):**
     - Added Catch2 test cases covering:
       - Empty scope (unrestricted / backward compatibility)
       - Exact match (`/reports` $\rightarrow$ `/reports`)
       - Subpath matches (`/reports` $\rightarrow$ `/reports/2026/annual.pdf`)
       - Sibling segment boundary enforcement (`/reports` vs `/reports2` 
$\rightarrow$ rejected)
       - Inverted hierarchy / parent path requests with child scope 
$\rightarrow$ rejected
       - Normalization edge cases (trailing slashes, redundant slashes, root 
scope `"/"`)
       - Token builder & parser integration with `addScope()` / `getScope()`
   
     * **Documentation (access_control.en.rst):**
       - Replaced the note stating `scope` is "ignored by the current version 
of the plugin" with full documentation of the matching semantics, segment 
boundary rules, and status code behavior.
   
   ### Future Work
   
   - Pattern matching (e.g., `/reports/{year}/`)
   - Scope lists or disjunctions
   - Host-qualified scopes (e.g., `example.com:/reports/`)
   - Query parameter scoping
   
   These can be added in follow-ups once the core prefix matching is proven.
   
   ### Verification
     - [x] Catch2 unit tests pass: 
`./build/plugins/experimental/access_control/unit_tests/test_access_control`
     - [x] Code formatted via `cmake --build build -t format`
   
   
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]

Reply via email to