aglinxinyuan opened a new pull request, #7690: URL: https://github.com/apache/texera/pull/7690
### What changes were proposed in this PR? `AccessControlResource` is the ext-authz endpoint the gateway calls on every request. It was at **74.2% of 159 lines** behind a 13-test spec, and the residue was concentrated in `extractTokenFromBody` -- which the entire suite invoked exactly once, with a body carrying no token, so only the "everything returns None" skeleton ran. Tests **13 -> 32**. Measured across the whole module (the file is also touched by sibling specs, so a single-spec figure would not be comparable to the reported one): | | Before | After | |---|---|---| | lines | 74.2% | **152/159 (95.6%)** | | branches | -- | 74/112 (66.1%) | Now covered: all three body parsers (JSON, form-urlencoded, multipart, including both boundary shapes), the `access-token` query parameter, the full token-precedence chain, both missing arms of the route whitelist, cuid resolution from path vs query, and the privilege and identity lookups. ### Verification, and what it found Review proposed 20 mutations. **All 20 survived the 22-test suite** -- every finding was real, none was a false alarm. 19 are now killed, each verified red on its intended test by name from the JUnit XML; the twentieth is an equivalent mutant, explained below. 24 distinct mutations were run over two passes, one at a time, with `git diff --quiet` on the production file asserted after every single run. **The most serious finding: the authorization guard itself was unpinned.** Deleting ```scala if (cuAccess == PrivilegeEnum.NONE) return FORBIDDEN ``` left all 22 tests green. The suite's only negative case used a cuid absent from the fixture DB, so its 403 came from the routing check -- the privilege check was never the reason for any rejection anywhere. Fixed with a fixture computing unit that exists and is routable but on which the test user holds no privilege. Three other axes were constants across the whole suite, so a lookup could be replaced by a literal and nothing noticed: | Surviving mutation | Why nothing noticed | |---|---| | `getComputingUnitAccess(cuidInt, uid)` -> `(cuidInt, 1)` | every request authenticated as uid 1 | | the `x-user-id` header hard-coded to `"1"` | same | | `qToken.orElse(hToken).orElse(bToken)` reordered | only query-vs-header was ever discriminated, never header-vs-body | Also fixed: the multipart regexes were pinned only by fixtures too simple to discriminate them (`(.*?)` vs `(.*)`, the `[^\r\n]*` attribute tail, `\s*=\s*` spacing, and `\r?\n` vs `\r\n`), and three `.trim` / `.map(_.trim)` calls that no fixture ever padded. ### One mutation is deliberately left alive At `:193`, `val key = if (idx >= 0) p.substring(0, idx) else p` -> `else ""` survives, and that is correct rather than a gap. The `idx < 0` leg is the only one reaching that `else`, and in the same leg `raw` is hard-coded to `""` at `:195`, so `v` is always empty and `found` is never assigned. No input can distinguish the two -- killing it would require mutating `:195` simultaneously. The line is executed; it is simply not behaviourally observable. ### Deliberately not included The `catch` at `:136-137` is unreachable: `JwtParser.parseToken` swallows every exception internally and returns `Optional.empty`, and the only other throw source is a Scala `object` with no injection seam whose failure mode would corrupt the shared MockTexeraDB pool. Two observations are reported rather than pinned, so neither is cemented: `extractTokenFromBody` never consults `Content-Type` and accepts the first `token` it finds in any of the three formats (bounded, since the value must still be a validly signed JWT), and one privilege-comparison question I have raised with a maintainer directly rather than here. No production file is touched. ### Any related issues, documentation, discussions? Closes #7689 ### How was this PR tested? ``` sbt "AccessControlService/testOnly org.apache.texera.AccessControlResourceSpec" ``` ``` [info] Total number of tests run: 32 [info] Tests: succeeded 32, failed 0, canceled 0, ignored 0, pending 0 [info] All tests passed. ``` The full module is green too (`AccessControlService/jacoco`, 56 tests), which matters here because `LiteLLMProxyAuthSpec` shares the JVM and resets the logback context. `Test/scalafmtCheck` and `Test/scalafix --check` both pass. ### Was this PR authored or co-authored using generative AI tooling? Generated-by: Claude Code (Opus 5) -- 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]
