eugenegujing opened a new pull request, #8647:
URL: https://github.com/apache/texera/pull/8647
### What changes were proposed in this PR?
`WorkflowResource.mapWorkflowEntries` sets `DashboardWorkflow.isOwner` by
comparing the record's owner uid with the caller's uid. It did so with
`getUid.eq(uid)`. Both operands are `java.lang.Integer`, and Scala's `eq` is
reference identity, so the comparison only succeeded when both boxes were the
same object. Two independently boxed values are the same object only inside the
`java.lang.Integer` cache, which spans -128 to 127. This PR changes the
comparison to `==`, which compares by value and is null-safe on the left
operand.
**Why this is a bug.** `isOwner` answers "is the caller this workflow's
owner". For every uid above 127 the answer came back `false` for the owner's
own request, while the `ownerId` in the same response equalled the caller's
uid. The cutoff is a JVM boxing detail, not a property of the data, so the
field is correct on a fresh database with small uids and silently wrong once a
deployment has more than 127 accounts. `WorkflowSearchQueryBuilder.toEntryImpl`
already computed the same flag with `==`, so the two implementations disagreed
for the same user and the same workflow. `.equals` would also compare by value,
but `baseWorkflowSelect()` LEFT JOINs `WORKFLOW_OF_USER`, so the record's uid
can be null and `.equals` would throw; `==` is the null-safe form and matches
the search builder.
**What the flag reaches today.** Two endpoints go through
`mapWorkflowEntries`: `GET /workflow/list` and the hub's `getTops`, via
`HubResource.fetchDashboardWorkflowsByWids`. No current frontend code reads
`isOwner` from either of them. The dashboard listing that does gate affordances
on `isOwner` is served by `GET /dashboard/search`, which uses the search
builder and was already correct, and the hub landing page renders its own card
that does not read the flag. So this PR changes API responses rather than any
rendered behavior, and no screenshots apply. It is still a defect in a
published REST response and a contradiction between three code paths that
answer the same question.
**One consequence worth stating plainly.** `HubResource.getTops` has no
`@Auth` parameter and takes `uid` as a query parameter, so the uid it reflects
into `isOwner` is caller-asserted. Before this change a caller passing someone
else's uid only got `isOwner: true` back for uids inside the Integer cache;
afterwards it works for any uid. This widens an existing weakness rather than
opening a new one, and it is not privilege escalation: nothing reads that
field, and `deleteWorkflow` independently re-checks ownership against the
authenticated session user. `isOwner` from that endpoint must not be treated as
an authorization signal. The endpoint's missing authentication is out of scope
here and belongs in its own issue.
The test changes add four positive and negative `isOwner` cases to
`HubResourceSpec`, whose uids already sit outside the Integer cache, and
replace a stale `NOTE` comment that documented this bug as deliberately
unasserted:
- `set isOwner for the owner's own request when the uid is outside the
Integer cache` (uid 9001) is the regression proper;
- `set isOwner for the owner's own request when the uid is inside the
Integer cache` (uid 42) pins that small uids, which were already correct, stay
correct;
- `set isOwner for the highest uid inside the Integer cache (127)` and `set
isOwner for the lowest uid outside the Integer cache (128)` localise the break
to a single increment rather than to the gap between 42 and 9001; and
- `clear isOwner when an anonymous caller sees a workflow with no ownership
row` covers both uids being null at once, where Scala's `==` reports two nulls
as equal and only the existing `uid != null` guard keeps the answer `false`.
The pre-existing negatives for a different uid and for an anonymous caller
are unchanged, and a case for a workflow with no ownership row asserts that a
null record uid yields `false` without throwing.
### Any related issues, documentation, discussions?
Closes #8646
Two neighbouring defects were found while working on this and are
deliberately not addressed here, since each is a different function and a
different failure mode: `mapWorkflowEntries` dereferences a null privilege for
a workflow with no `workflow_user_access` row, and
`WorkflowSearchQueryBuilder.toEntryImpl` lacks the `uid != null` guard that
this file has, so an anonymous `publicSearch` caller would report `isOwner:
true` for a workflow with no ownership row.
### How was this PR tested?
Unit tests, run against the embedded PostgreSQL that `MockTexeraDB`
provides, so no external database is needed.
```shell
sbt "WorkflowExecutionService/testOnly
org.apache.texera.web.resource.dashboard.hub.HubResourceSpec"
sbt "WorkflowExecutionService/testOnly \
org.apache.texera.web.resource.dashboard.file.WorkflowResourceSpec \
org.apache.texera.web.resource.dashboard.hub.HubResourceSpec \
org.apache.texera.web.resource.dashboard.WorkflowSearchQueryBuilderSpec \
org.apache.texera.web.resource.dashboard.DashboardResourceSpec \
org.apache.texera.web.resource.dashboard.DatasetSearchQueryBuilderSpec"
sbt scalafmtCheckAll
sbt "scalafixAll --check"
```
Results:
- `HubResourceSpec`: 70/70 passed;
- the five suites that reach `mapWorkflowEntries` or assert on an `isOwner`
flag: 190/190 passed;
- `scalafmtCheckAll`: passed; and
- `scalafixAll --check`: passed.
Both new behaviours were confirmed to fail without the corresponding source
line, not merely to pass with it. Restoring `getUid.eq(uid)` fails exactly two
cases, the uid 9001 one and the uid 128 one, with `false was not equal to
true`, while the uid 127 case still passes, which places the break at one
increment. Separately, deleting the `uid != null` guard while keeping `==`
fails exactly one case, the anonymous caller on a workflow with no ownership
row, with `true was not equal to false`; no pre-existing test catches that,
which is why the case was added.
The response itself was also checked end to end through the real handlers
rather than through test helpers, by calling `persistWorkflow` and then
`retrieveWorkflowsBySessionUser` for one owner on each side of the cache
boundary. Before the change:
```text
caller uid=7 ownerId=7 isOwner=true workflow=wf_of_small_uid_user
caller uid=1001 ownerId=1001 isOwner=false workflow=wf_of_large_uid_user
```
After the change both rows report `isOwner=true`.
### Was this PR authored or co-authored using generative AI tooling?
Generated-by: Claude Code (Claude 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]
