You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@texera.apache.org by gi...@apache.org on 2026/08/15 21:03:35 UTC
(texera) branch gh-readonly-queue/main/pr-7690-f8912a452b06f0f50c3f99c0318a7480470cc6f5 created (now 0de241723e)
This is an automated email from the ASF dual-hosted git repository.
github-merge-queue[bot] pushed a change to branch gh-readonly-queue/main/pr-7690-f8912a452b06f0f50c3f99c0318a7480470cc6f5
in repository https://gitbox.apache.org/repos/asf/texera.git
at 0de241723e test(access-control): pin token extraction and the privilege guard (#7690)
This branch includes the following new commits:
new 0de241723e test(access-control): pin token extraction and the privilege guard (#7690)
The 1 revisions listed above as "new" are entirely new to this
repository and will be described in separate emails. The revisions
listed as "add" were already present in the repository and have only
been added to this reference.
(texera) 01/01: test(access-control): pin token extraction and the privilege guard (#7690)
Posted by gi...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
github-merge-queue[bot] pushed a commit to branch gh-readonly-queue/main/pr-7690-f8912a452b06f0f50c3f99c0318a7480470cc6f5
in repository https://gitbox.apache.org/repos/asf/texera.git
commit 0de241723eb80ac23cc765d2b22c20bb7321bb9b
Author: Xinyuan Lin <xi...@uci.edu>
AuthorDate: Sat Aug 15 21:03:14 2026 +0000
test(access-control): pin token extraction and the privilege guard (#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)
---
.../apache/texera/AccessControlResourceSpec.scala | 401 ++++++++++++++++++++-
1 file changed, 387 insertions(+), 14 deletions(-)
diff --git a/access-control-service/src/test/scala/org/apache/texera/AccessControlResourceSpec.scala b/access-control-service/src/test/scala/org/apache/texera/AccessControlResourceSpec.scala
index b4f1b89617..10ff44db7f 100644
--- a/access-control-service/src/test/scala/org/apache/texera/AccessControlResourceSpec.scala
+++ b/access-control-service/src/test/scala/org/apache/texera/AccessControlResourceSpec.scala
@@ -42,7 +42,8 @@ import org.scalatest.flatspec.AnyFlatSpec
import org.scalatest.matchers.should.Matchers
import org.scalatest.{BeforeAndAfterAll, BeforeAndAfterEach}
-import java.net.URI
+import java.net.{URI, URLEncoder}
+import java.nio.charset.StandardCharsets
import java.util
class AccessControlResourceSpec
@@ -60,6 +61,18 @@ class AccessControlResourceSpec
private val testRecordedUri: String =
"computing-unit-2.compute-unit-svc.default.svc.cluster.local:8888"
+ // A second, deliberately different recorded URI. Routing tests that target the
+ // read-only unit assert on this value so a Host header that ignored the looked-up
+ // row (or hard-coded the other unit's host) would fail.
+ private val testReadOnlyRecordedUri: String =
+ "computing-unit-5.compute-unit-svc.default.svc.cluster.local:9999"
+
+ // Recorded URI of the routable unit testUser1 has no privilege on. It exists so a
+ // request refused by the privilege check cannot be confused with one refused for
+ // being unroutable.
+ private val testNoAccessRecordedUri: String =
+ "computing-unit-6.compute-unit-svc.default.svc.cluster.local:7777"
+
private val testUser1: User = {
val user = new User()
user.setUid(1)
@@ -112,8 +125,39 @@ class AccessControlResourceSpec
cu
}
+ // A routable computing unit on which testUser1 holds READ (not WRITE). Every other
+ // fixture grants WRITE, so this is the only unit whose privilege header can tell a
+ // real lookup apart from a hard-coded WRITE.
+ private val testCUReadOnly: WorkflowComputingUnit = {
+ val cu = new WorkflowComputingUnit()
+ cu.setUid(2)
+ cu.setType(WorkflowComputingUnitTypeEnum.kubernetes)
+ cu.setCuid(5)
+ cu.setName("test-cu-read-only")
+ cu.setUri(testReadOnlyRecordedUri)
+ cu
+ }
+
+ // A perfectly routable computing unit owned by testUser2 on which testUser1 is
+ // granted nothing at all. It is the only fixture whose refusal can come from the
+ // privilege check itself: every other refusal in this suite is produced either by
+ // the unit not existing or by it having no recorded URI.
+ private val testCUNoAccess: WorkflowComputingUnit = {
+ val cu = new WorkflowComputingUnit()
+ cu.setUid(testUser2.getUid)
+ cu.setType(WorkflowComputingUnitTypeEnum.kubernetes)
+ cu.setCuid(6)
+ cu.setName("test-cu-no-access")
+ cu.setUri(testNoAccessRecordedUri)
+ cu
+ }
+
private var token: String = _
+ // A token for the *other* user. Tests that must tell "the authenticated user" apart
+ // from "user 1" need a second identity to compare against.
+ private var token2: String = _
+
override protected def beforeAll(): Unit = {
initializeDBAndReplaceDSLContext()
val userDao = new UserDao(getDSLContext.configuration())
@@ -126,6 +170,9 @@ class AccessControlResourceSpec
computingUnitDao.insert(testCU)
computingUnitDao.insert(testCUNoUri)
computingUnitDao.insert(testCUBlankUri)
+ computingUnitDao.insert(testCUReadOnly)
+ // Deliberately inserted with NO computing_unit_user_access row for testUser1.
+ computingUnitDao.insert(testCUNoAccess)
// Grant testUser1 WRITE access to every test computing unit so the routing
// logic (not the access check) is what each routing test exercises.
@@ -137,8 +184,14 @@ class AccessControlResourceSpec
computingUnitOfUserDao.insert(cuAccess)
}
- val claims = JwtAuth.jwtClaims(testUser1)
- token = JwtAuth.jwtToken(claims)
+ val readOnlyAccess = new ComputingUnitUserAccess()
+ readOnlyAccess.setUid(testUser1.getUid)
+ readOnlyAccess.setCuid(testCUReadOnly.getCuid)
+ readOnlyAccess.setPrivilege(PrivilegeEnum.READ)
+ computingUnitOfUserDao.insert(readOnlyAccess)
+
+ token = JwtAuth.jwtToken(JwtAuth.jwtClaims(testUser1))
+ token2 = JwtAuth.jwtToken(JwtAuth.jwtClaims(testUser2))
}
override protected def afterAll(): Unit = {
@@ -204,14 +257,16 @@ class AccessControlResourceSpec
response.getStatus shouldBe Response.Status.FORBIDDEN.getStatusCode
}
- "AccessControlResource" should "return FORBIDDEN when user does not have access to the computing unit" in {
+ "AccessControlResource" should "return FORBIDDEN for an unknown computing unit" in {
// Mock the request context
val mockUriInfo = mock(classOf[UriInfo])
val mockHttpHeaders = mock(classOf[HttpHeaders])
- // Prepare query parameters with a computing unit ID (cuid)
+ // cuid 1 is not in the fixture DB at all, so this exercises the "no such unit"
+ // path. The privilege check itself is exercised by the no-privilege test below,
+ // which uses a unit that exists and is routable.
val queryParams = new MultivaluedHashMap[String, String]()
- queryParams.add("cuid", "1") // Assuming user 1 does not have access to cuid 1
+ queryParams.add("cuid", "1")
// Prepare request headers with the generated JWT
val requestHeaders = new MultivaluedHashMap[String, String]()
@@ -271,6 +326,16 @@ class AccessControlResourceSpec
response.getHeaderString("Host") shouldBe testRecordedUri
}
+ it should "return FORBIDDEN when the user has no privilege on a routable computing unit" in {
+ // testCUNoAccess exists, is owned by testUser2, and has a recorded URI — so the
+ // routing check would happily let this through. The only thing that can refuse it
+ // is the PrivilegeEnum.NONE guard.
+ val (uri, headers) = mockRequest("/pve/system", Some(testCUNoAccess.getCuid.toString))
+ val response = new AccessControlResource().authorizeGet(uri, headers)
+
+ response.getStatus shouldBe Response.Status.FORBIDDEN.getStatusCode
+ }
+
it should "refuse the connection when no URI is recorded for the computing unit" in {
val (uri, headers) = mockRequest(testPath, Some(testCUNoUri.getCuid.toString))
val response = new AccessControlResource().authorizeGet(uri, headers)
@@ -285,25 +350,47 @@ class AccessControlResourceSpec
response.getStatus shouldBe Response.Status.FORBIDDEN.getStatusCode
}
+ /**
+ * Builds a mocked request context.
+ *
+ * @param authorizationHeader `None` models a client that sent no Authorization
+ * header at all — JAX-RS returns `null` from
+ * `getRequestHeader` in that case, which is what the
+ * body-token tests need so the body is the only token
+ * source left.
+ * @param accessTokenQueryParam value for the `access-token` query parameter, when
+ * the token travels in the URL instead.
+ */
private def mockRequest(
path: String,
- cuidQueryParam: Option[String]
+ cuidQueryParam: Option[String],
+ authorizationHeader: Option[String] = Some("Bearer " + token),
+ accessTokenQueryParam: Option[String] = None
): (UriInfo, HttpHeaders) = {
val mockUriInfo = mock(classOf[UriInfo])
val mockHttpHeaders = mock(classOf[HttpHeaders])
val queryParams = new MultivaluedHashMap[String, String]()
cuidQueryParam.foreach(queryParams.add("cuid", _))
+ accessTokenQueryParam.foreach(queryParams.add("access-token", _))
val requestHeaders = new MultivaluedHashMap[String, String]()
- requestHeaders.add("Authorization", "Bearer " + token)
when(mockUriInfo.getQueryParameters).thenReturn(queryParams)
when(mockUriInfo.getRequestUri).thenReturn(new URI(testURI))
when(mockUriInfo.getPath).thenReturn(path)
+
+ authorizationHeader match {
+ case Some(header) =>
+ requestHeaders.add("Authorization", header)
+ when(mockHttpHeaders.getRequestHeader("Authorization"))
+ .thenReturn(util.Arrays.asList(header))
+ case None =>
+ // JAX-RS hands back null for a header the client never sent.
+ when(mockHttpHeaders.getRequestHeader("Authorization"))
+ .thenReturn(null.asInstanceOf[util.List[String]])
+ }
when(mockHttpHeaders.getRequestHeaders).thenReturn(requestHeaders)
- when(mockHttpHeaders.getRequestHeader("Authorization"))
- .thenReturn(util.Arrays.asList("Bearer " + token))
(mockUriInfo, mockHttpHeaders)
}
@@ -315,18 +402,34 @@ class AccessControlResourceSpec
response.getStatus shouldBe Response.Status.OK.getStatusCode
}
- it should "return OK for /pve/pves/{cuid} (cuid extracted from path)" in {
- val (uri, headers) = mockRequest(s"/pve/pves/${testCU.getCuid}", None)
+ // The three path shapes below use the form Jersey actually hands to the resource:
+ // UriInfo.getPath returns the path relative to the base URI, so a check for
+ // /auth/api/pve/... arrives as "auth/api/pve/..." — no leading slash, and with the
+ // "auth/" and "api/" segments still attached. That is exactly what the optional
+ // `^/?(?:auth/)?(?:api/|wsapi/)?` prefix groups on each pve regex exist to absorb.
+ it should "return OK for the gateway-relative form of a pve route" in {
+ val (uri, headers) = mockRequest("auth/api/pve/system", Some(testCU.getCuid.toString))
+ val response = new AccessControlResource().authorizeGet(uri, headers)
+
+ response.getStatus shouldBe Response.Status.OK.getStatusCode
+ response.getHeaderString("Host") shouldBe testRecordedUri
+ }
+
+ it should "return OK for pve/pves/{cuid} (cuid extracted from path)" in {
+ val (uri, headers) = mockRequest(s"auth/api/pve/pves/${testCU.getCuid}", None)
val response = new AccessControlResource().authorizeDelete(uri, headers)
response.getStatus shouldBe Response.Status.OK.getStatusCode
+ response.getHeaderString("Host") shouldBe testRecordedUri
}
- it should "return OK for /pve/{cuid}/{pveName}/packages/{packageName} (cuid extracted from path)" in {
- val (uri, headers) = mockRequest(s"/pve/${testCU.getCuid}/myenv/packages/numpy", None)
+ it should "return OK for pve/{cuid}/{pveName}/packages/{packageName} (cuid extracted from path)" in {
+ val (uri, headers) =
+ mockRequest(s"auth/api/pve/${testCU.getCuid}/myenv/packages/numpy", None)
val response = new AccessControlResource().authorizeDelete(uri, headers)
response.getStatus shouldBe Response.Status.OK.getStatusCode
+ response.getHeaderString("Host") shouldBe testRecordedUri
}
it should "return FORBIDDEN for a PVE path with no cuid in query or path" in {
@@ -349,4 +452,274 @@ class AccessControlResourceSpec
response.getStatus shouldBe Response.Status.OK.getStatusCode
}
+
+ it should "report the privilege and the recorded URI of the requested computing unit" in {
+ val (uri, headers) = mockRequest("/pve/system", Some(testCUReadOnly.getCuid.toString))
+ val response = new AccessControlResource().authorizeGet(uri, headers)
+
+ // testUser1 holds READ (not WRITE) on this unit, and this unit's recorded URI
+ // differs from every other fixture's, so both headers are answered from the
+ // row actually looked up rather than from a constant.
+ response.getStatus shouldBe Response.Status.OK.getStatusCode
+ response.getHeaderString(
+ HeaderField.UserComputingUnitAccess
+ ) shouldBe PrivilegeEnum.READ.toString
+ response.getHeaderString("Host") shouldBe testReadOnlyRecordedUri
+ }
+
+ it should "resolve the privilege and identity of the authenticated user, not a fixed one" in {
+ // testUser2 owns testCUReadOnly, so the owner branch of the lookup gives it WRITE,
+ // while testUser1 — the identity every other test authenticates as — only holds
+ // READ on that same unit. Answering for the wrong user is therefore visible in
+ // both the privilege header and the forwarded identity headers.
+ val (uri, headers) = mockRequest(
+ "/pve/system",
+ Some(testCUReadOnly.getCuid.toString),
+ authorizationHeader = Some("Bearer " + token2)
+ )
+ val response = new AccessControlResource().authorizeGet(uri, headers)
+
+ response.getStatus shouldBe Response.Status.OK.getStatusCode
+ response.getHeaderString(
+ HeaderField.UserComputingUnitAccess
+ ) shouldBe PrivilegeEnum.WRITE.toString
+ response.getHeaderString(HeaderField.UserId) shouldBe testUser2.getUid.toString
+ response.getHeaderString(HeaderField.UserName) shouldBe testUser2.getName
+ response.getHeaderString(HeaderField.UserEmail) shouldBe testUser2.getEmail
+ }
+
+ it should "fall back to the cuid in the path when the cuid query parameter is empty" in {
+ // Envoy forwards `?cuid=` verbatim when the client sends the parameter with no
+ // value; an empty parameter must not shadow the cuid embedded in the path.
+ val (uri, headers) = mockRequest(s"/pve/pves/${testCUReadOnly.getCuid}", Some(""))
+ val response = new AccessControlResource().authorizeDelete(uri, headers)
+
+ response.getStatus shouldBe Response.Status.OK.getStatusCode
+ response.getHeaderString("Host") shouldBe testReadOnlyRecordedUri
+ }
+
+ it should "prefer the cuid query parameter over the cuid embedded in the path" in {
+ // Path says unit 5 (READ, its own host), query says unit 2 (WRITE, another host):
+ // both headers show which one the resolution actually used.
+ val (uri, headers) =
+ mockRequest(s"/pve/pves/${testCUReadOnly.getCuid}", Some(testCU.getCuid.toString))
+ val response = new AccessControlResource().authorizeDelete(uri, headers)
+
+ response.getStatus shouldBe Response.Status.OK.getStatusCode
+ response.getHeaderString(
+ HeaderField.UserComputingUnitAccess
+ ) shouldBe PrivilegeEnum.WRITE.toString
+ response.getHeaderString("Host") shouldBe testRecordedUri
+ }
+
+ it should "authorize the /wsapi/workflow-websocket route using the access-token query parameter" in {
+ // The Authorization header carries a token that cannot be parsed, so a 200 is
+ // only reachable if the access-token query parameter is read AND preferred.
+ val (uri, headers) = mockRequest(
+ "/wsapi/workflow-websocket",
+ Some(testCU.getCuid.toString),
+ authorizationHeader = Some("Bearer not-a-real-jwt"),
+ accessTokenQueryParam = Some(token)
+ )
+ val response = new AccessControlResource().authorizeGet(uri, headers)
+
+ response.getStatus shouldBe Response.Status.OK.getStatusCode
+ response.getHeaderString("Host") shouldBe testRecordedUri
+ }
+
+ it should "authorize the /api/executions/result/export route" in {
+ val (uri, headers) =
+ mockRequest("/api/executions/result/export", Some(testCU.getCuid.toString))
+ val response = new AccessControlResource().authorizeGet(uri, headers)
+
+ response.getStatus shouldBe Response.Status.OK.getStatusCode
+ }
+
+ it should "ignore an empty access-token query parameter and fall back to the Authorization header" in {
+ val (uri, headers) = mockRequest(
+ "/pve/system",
+ Some(testCU.getCuid.toString),
+ accessTokenQueryParam = Some("")
+ )
+ val response = new AccessControlResource().authorizeGet(uri, headers)
+
+ response.getStatus shouldBe Response.Status.OK.getStatusCode
+ }
+
+ it should "accept a lower-case bearer scheme" in {
+ // RFC 7235 makes the auth scheme case-insensitive, so a client sending "bearer"
+ // must not be turned away.
+ val (uri, headers) = mockRequest(
+ "/pve/system",
+ Some(testCU.getCuid.toString),
+ authorizationHeader = Some("bearer " + token)
+ )
+ val response = new AccessControlResource().authorizeGet(uri, headers)
+
+ response.getStatus shouldBe Response.Status.OK.getStatusCode
+ response.getHeaderString("Host") shouldBe testRecordedUri
+ }
+
+ it should "accept an Authorization value that is a padded bare token" in {
+ // The scheme is stripped only if present, so a client that sends the raw token is
+ // still understood — and the surrounding whitespace has to be removed before the
+ // JWT parser sees it. (Padding after a "Bearer " scheme would prove nothing: the
+ // strip regex already eats the leading whitespace, and jose4j ignores trailing
+ // whitespace because it lands in the base64url signature segment. Only a
+ // scheme-less value can carry *leading* whitespace this far, and leading
+ // whitespace corrupts the signing input.)
+ val (uri, headers) = mockRequest(
+ "/pve/system",
+ Some(testCU.getCuid.toString),
+ authorizationHeader = Some(" " + token + " ")
+ )
+ val response = new AccessControlResource().authorizeGet(uri, headers)
+
+ response.getStatus shouldBe Response.Status.OK.getStatusCode
+ response.getHeaderString("Host") shouldBe testRecordedUri
+ }
+
+ // ---------------------------------------------------------------------------
+ // Token carried in the request body.
+ //
+ // Each of these sends no Authorization header and no access-token query
+ // parameter, so the body is the only token source left: the 200 can only come
+ // from the body-parsing branch under test.
+ // ---------------------------------------------------------------------------
+
+ private val multipartBoundary: String = "----TexeraTestBoundary"
+ private val crlf: String = "\r\n"
+
+ /**
+ * A realistic two-part CRLF multipart document whose first part is named "token".
+ *
+ * Everything about its shape is load-bearing: the Content-Disposition line carries
+ * a trailing attribute after `name="token"`, the value is padded with spaces, and a
+ * second part follows before the closing boundary — so the capture has to stop at
+ * the *next* boundary rather than running to the last one.
+ */
+ private def multipartTwoParts(value: String): String =
+ s"--$multipartBoundary$crlf" +
+ s"""Content-Disposition: form-data; name="token"; filename="t.txt"$crlf""" +
+ crlf +
+ s" $value $crlf" +
+ s"--$multipartBoundary$crlf" +
+ s"""Content-Disposition: form-data; name="other"$crlf""" +
+ crlf +
+ s"hello$crlf" +
+ s"--$multipartBoundary--"
+
+ /**
+ * A single-part document that stops at the value, so only the run-to-end fallback
+ * can recover the token. Deliberately built unlike [[multipartTwoParts]]: LF-only
+ * line breaks (a client that does not send CRLF), whitespace around the `=` in the
+ * Content-Disposition, a trailing attribute, and a padded value.
+ */
+ private def multipartRunToEnd(value: String): String =
+ s"--$multipartBoundary\n" +
+ "Content-Disposition: form-data; name = \"token\"; filename=\"t.txt\"\n" +
+ "\n" +
+ s" $value "
+
+ private def bodyOnlyRequest(cuid: Int): (UriInfo, HttpHeaders) =
+ mockRequest("/pve/system", Some(cuid.toString), authorizationHeader = None)
+
+ it should "read the token out of an x-www-form-urlencoded body" in {
+ // A single `token=<value>` pair — the shape a form encoder produces for a one-field
+ // form, and the one body with no '&' in it at all.
+ // Encoded the way a form encoder would: the surrounding spaces become '+', so
+ // a reader that skipped URL-decoding would see "+<jwt>+" and reject it.
+ val encoded = URLEncoder.encode(s" $token ", StandardCharsets.UTF_8.name())
+ val (uri, headers) = bodyOnlyRequest(testCU.getCuid)
+
+ val response = new AccessControlResource().authorizePost(uri, headers, s"token=$encoded")
+
+ response.getStatus shouldBe Response.Status.OK.getStatusCode
+ response.getHeaderString("Host") shouldBe testRecordedUri
+ }
+
+ it should "keep scanning an urlencoded body past a valueless token pair" in {
+ val (uri, headers) = bodyOnlyRequest(testCU.getCuid)
+
+ // The leading "token" pair has no '=' and therefore no value; it must not be
+ // accepted as the token, and the scan must continue to the real pair.
+ val response =
+ new AccessControlResource().authorizePost(uri, headers, s"token&other=1&token=$token")
+
+ response.getStatus shouldBe Response.Status.OK.getStatusCode
+ }
+
+ it should "take the first token pair when an urlencoded body carries duplicates" in {
+ val (uri, headers) = bodyOnlyRequest(testCU.getCuid)
+
+ // Duplicate keys are not specified by the form-encoding spec, so the resolution
+ // rule is a deliberate choice: the scan stops at the first usable value. Both
+ // tokens here are valid and testUser2 owns this unit, so both orderings return
+ // 200 — only the forwarded identity says which credential was selected.
+ val response =
+ new AccessControlResource().authorizePost(uri, headers, s"token=$token&token=$token2")
+
+ response.getStatus shouldBe Response.Status.OK.getStatusCode
+ response.getHeaderString(HeaderField.UserId) shouldBe testUser1.getUid.toString
+ }
+
+ it should "read the token out of a JSON body" in {
+ val (uri, headers) = bodyOnlyRequest(testCU.getCuid)
+
+ // Jackson preserves the padding, so the value has to be trimmed before it reaches
+ // the JWT parser.
+ val response =
+ new AccessControlResource().authorizePost(uri, headers, s"""{"token":" $token "}""")
+
+ response.getStatus shouldBe Response.Status.OK.getStatusCode
+ response.getHeaderString("Host") shouldBe testRecordedUri
+ }
+
+ it should "read the token out of a multipart body terminated by a boundary" in {
+ val (uri, headers) = bodyOnlyRequest(testCU.getCuid)
+
+ val response =
+ new AccessControlResource().authorizePost(uri, headers, multipartTwoParts(token))
+
+ response.getStatus shouldBe Response.Status.OK.getStatusCode
+ response.getHeaderString("Host") shouldBe testRecordedUri
+ }
+
+ it should "read the token out of a multipart body that ends with the token" in {
+ val (uri, headers) = bodyOnlyRequest(testCU.getCuid)
+
+ // No closing boundary follows the value, so only the run-to-end fallback can
+ // recover the token.
+ val response =
+ new AccessControlResource().authorizePost(uri, headers, multipartRunToEnd(token))
+
+ response.getStatus shouldBe Response.Status.OK.getStatusCode
+ response.getHeaderString("Host") shouldBe testRecordedUri
+ }
+
+ it should "prefer the Authorization header over a token in the request body" in {
+ // Header and body carry *different* valid identities. testUser2 owns this unit, so
+ // either choice returns 200; only the forwarded identity reveals which token was
+ // used to authenticate.
+ val (uri, headers) = mockRequest("/pve/system", Some(testCU.getCuid.toString))
+ val response =
+ new AccessControlResource().authorizePost(uri, headers, s"token=$token2")
+
+ response.getStatus shouldBe Response.Status.OK.getStatusCode
+ response.getHeaderString(HeaderField.UserId) shouldBe testUser1.getUid.toString
+ }
+
+ it should "fall back to the request body when the Authorization header is a bare bearer scheme" in {
+ // "Bearer " with nothing after it leaves an empty token; it must be discarded so
+ // the next source in the chain still gets a chance.
+ val (uri, headers) = mockRequest(
+ "/pve/system",
+ Some(testCU.getCuid.toString),
+ authorizationHeader = Some("Bearer ")
+ )
+ val response = new AccessControlResource().authorizePost(uri, headers, s"token=$token")
+
+ response.getStatus shouldBe Response.Status.OK.getStatusCode
+ response.getHeaderString("Host") shouldBe testRecordedUri
+ }
}