Skip to content

fix(file-service): 400 on revoke of an unknown email - #8384

Open
gupta-sahil01 wants to merge 1 commit into
apache:mainfrom
gupta-sahil01:fix/revoke-unknown-email
Open

fix(file-service): 400 on revoke of an unknown email#8384
gupta-sahil01 wants to merge 1 commit into
apache:mainfrom
gupta-sahil01:fix/revoke-unknown-email

Conversation

@gupta-sahil01

Copy link
Copy Markdown
Contributor

What changes were proposed in this PR?

ResourceAccess.revoke dereferenced the result of UserDao.fetchOneByEmail(email) without a null check:

val granteeUid = new UserDao(ctx.configuration()).fetchOneByEmail(email).getUid

fetchOneByEmail returns null for an address with no account, so revoking a dataset or model share for an unregistered email threw a NullPointerException. The service registers no ExceptionMapper for it, so it surfaced as an opaque HTTP 500 rather than an actionable 400.

grant, in the same object, already performed the correct check. This PR extracts that check into a private resolveUidByEmail helper and routes both grant and revoke through it, mirroring ComputingUnitAccessResource.resolveUidByEmail added in #6446 for the same defect.

Affected endpoints:

  • DELETE /api/access/dataset/revoke/{did}/{email}
  • DELETE /api/access/model/revoke/{mid}/{email}

Behaviour note: revoke now also rejects placeholder accounts, matching grant and ComputingUnitAccessResource.

Any related issues, documentation, discussions?

Closes #8353.

Same defect as #6445, fixed in #6446 for ComputingUnitAccessResource only. That fix's rationale assumed the dataset/model/project resources already behaved correctly — true of their grant paths, but not their revoke paths.

How was this PR tested?

Added "reject a revoke for an email with no account" to DatasetAccessResourceSpec and ModelAccessResourceSpec.

sbt "FileService/testOnly *DatasetAccessResourceSpec *ModelAccessResourceSpec"

[info] Tests: succeeded 71, failed 0, canceled 0, ignored 0, pending 0
[info] All tests passed.

Originally found on a local bin/local-dev.sh stack, where DELETE /api/access/dataset/revoke/2/nobody@example.com returned HTTP 500 with:

java.lang.NullPointerException: Cannot invoke "...pojos.User.getUid()" because the return value of "...daos.UserDao.fetchOneByEmail(String)" is null
    at org.apache.texera.service.resource.ResourceAccess$.revoke(ResourceAccess.scala:288)
    at org.apache.texera.service.resource.DatasetAccessResource.$anonfun$revokeAccess$1(DatasetAccessResource.scala:138)

Was this PR authored or co-authored using generative AI tooling?

Generated-by: Claude Code

@github-actions github-actions Bot added fix platform Non-amber Scala service paths labels Sep 3, 2026
@Yicong-Huang Yicong-Huang added release/v1.3 back porting to release/v1.3 release/v1.2 back porting to release/v1.2 labels Sep 3, 2026
@github-actions
github-actions Bot requested a review from mengw15 September 3, 2026 18:49
@github-actions

github-actions Bot commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

Automated Reviewer Suggestions

Based on the git blame history of the changed files, we recommend the following reviewers:

  • Contributors with relevant context: @tanishqgandhi1908
    You can notify them by mentioning @tanishqgandhi1908 in a comment.

@github-actions
github-actions Bot requested a review from xuang7 September 3, 2026 18:49
@github-actions

github-actions Bot commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

Backport auto-label report

This fix: PR was checked against each actively-supported release branch. release/* labels drive the post-merge backport, so add or remove one to change where this fix lands.

Release branch Analysis
release/v1.3 Change detected on this branch — label added; this fix is queued to backport here. Requested review from @mengw15.
release/v1.2 Change detected on this branch — label added; this fix is queued to backport here. Requested review from @xuang7.

Auto-label run.

@codecov-commenter

codecov-commenter commented Sep 3, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 94.04%. Comparing base (cd4fd5a) to head (ffc8fac).

Additional details and impacted files
@@            Coverage Diff            @@
##               main    #8384   +/-   ##
=========================================
  Coverage     94.03%   94.04%           
- Complexity     4771     4785   +14     
=========================================
  Files          1196     1196           
  Lines         48757    48778   +21     
  Branches       5916     5918    +2     
=========================================
+ Hits          45850    45871   +21     
  Misses         1458     1458           
  Partials       1449     1449           
Flag Coverage Δ *Carryforward flag
access-control-service 81.00% <ø> (ø)
agent-service 99.32% <ø> (ø) Carriedforward from cd4fd5a
amber 89.89% <ø> (ø) Carriedforward from cd4fd5a
computing-unit-managing-service 73.67% <ø> (ø)
config-service 89.25% <ø> (+2.12%) ⬆️
file-service 87.92% <100.00%> (+0.01%) ⬆️
frontend 96.81% <ø> (ø) Carriedforward from cd4fd5a
notebook-migration-service 79.31% <ø> (ø)
pyamber 98.47% <ø> (ø) Carriedforward from cd4fd5a
workflow-compiling-service 77.19% <ø> (ø)

*This pull request uses carry forward flags. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

fix platform Non-amber Scala service paths release/v1.2 back porting to release/v1.2 release/v1.3 back porting to release/v1.3

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Dataset and model revoke endpoints return HTTP 500 for an unregistered email

3 participants