apache / apache/texera

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

Open
#8,353 2 comments 0 reactions 1 assignee Claimed by @gupta-sahil01 View on GitHub
Dominant language
Scala
Stars
314
Forks
187
Avg merge
1d 21h
Merged PRs (30d)
214

Description

### What happened?

Three sharing endpoints return an opaque **HTTP 500** when access is revoked for an email that has no account:

| Endpoint | Service | Unguarded line |
| --- | --- | --- |
| `DELETE /api/access/dataset/revoke/{did}/{email}` | file-service | `ResourceAccess.scala:288` |
| `DELETE /api/access/model/revoke/{mid}/{email}` | file-service | `ResourceAccess.scala:288` (shared helper) |

`UserDao.fetchOneByEmail` returns `null` for an unknown address, and both call sites dereference it directly:

```scala
// ResourceAccess.scala:288 (revoke)
val granteeUid = new UserDao(ctx.configuration()).fetchOneByEmail(email).getUid
```

The **grant** path in the very same files already handles this correctly:

```scala
// ResourceAccess.scala:255-258 (grant)
val grantee = new UserDao(ctx.configuration()).fetchOneByEmail(email)
if (grantee == null || grantee.getIsPlaceholder) {
throw new BadRequestException(s"No registered user with email $email")
}
```

No `ExceptionMapper` is registered for `NullPointerException` in either service — the only mapper in the tree is `UnauthorizedExceptionMapper` (`common/auth/.../AuthFeatures.scala:38`) — so the NPE surfaces as a bare 500.

**Expected:** `400` with `No registered user with email {email}`, matching the grant path and matching `ComputingUnitAccessResource`.

### Current behaviour across the four access resources

| Resource | grant | revoke |
| --- | --- | --- |
| `ComputingUnitAccessResource` | ✅ `resolveUidByEmail` → 400 | ✅ `resolveUidByEmail` → 400 |
| `WorkflowAccessResource` | ✅ explicit check → 400 | ⚠️ `catch { case _: NullPointerException }` → 400 |
| `ResourceAccess` (dataset + model) | ✅ explicit check → 400 | ❌ **500** |

### Relationship to #6445

#6445 fixed exactly this defect for the computing-unit endpoints and introduced `ComputingUnitAccessResource.resolveUidByEmail` as the fix. Its rationale reads:

> This matches how DatasetAccessResource/WorkflowAccessResource/ProjectAccessResource already behave; the computing-unit resource had diverged.

That premise holds only for the **grant** path. On the **revoke** path, the dataset/model resources still perform the unguarded dereference, so #6445 aligned the computing-unit resource to a standard the reference resources do not themselves meet. This issue covers the remaining three endpoints. (the project resource has since been removed in #7464.)

Not a regression: the unguarded revoke predates the `ResourceAccess` extraction in #7760 (`git show 4842e93f2^` shows the same pattern in `DatasetAccessResource.revokeAccess`); the refactor carried the grant-side check across and left revoke as it found it.

### Additional notes

- `ResourceAccess.revoke`'s scaladoc states *"Removes the user's explicit grant; a no-op when they hold none."* That is accurate for a **registered** user holding no grant, but not for an unregistered address, where the call 500s.
- Unlike #6445, this is **not reachable from the UI**: the share dialog only renders a revoke control beside users already present in the access list, so an unregistered email never gets one. The endpoint is reachable only by direct API call, which is likely why it has gone unnoticed.
- The revoke path also does not reject placeholder accounts, which grant rejects via `getIsPlaceholder`. Worth aligning in the same change.

---

### How to reproduce?

### Via the API

Reproduced on a local `bin/local-dev.sh up` stack, signed in as the admin account (the only registered user, `email = texera`):

```js
// browser devtools console on http://localhost:4200
const t = localStorage.getItem("access_token");

// unregistered email -> 500
await fetch(`/api/access/dataset/revoke/2/nobody@example.com`, {
method: "DELETE", headers: { Authorization: `Bearer ${t}` },
}).then(r => r.status); // 500

// same email, same dataset, grant instead of revoke -> 400
await fetch(`/api/access/dataset/grant/2/nobody@example.com/READ`, {
method: "PUT", headers: { Authorization: `Bearer ${t}` },
}).then(r => r.status); // 400
```

The caller needs write access on the resource, so `requireWriteAccess` passes and execution reaches the dereference.

Same result for `/api/access/model/revoke/{mid}/{email}`.

### As a unit test

Added to `file-service/src/test/scala/org/apache/texera/service/resource/DatasetAccessResourceSpec.scala` and `file-service/src/test/scala/org/apache/texera/service/resource/ModelAccessResourceSpec.scala`, this fails with `NullPointerException` instead of `BadRequestException`:

```scala
it should "reject a revoke for an email with no account" in {
assertThrows[BadRequestException] {
accessResource.revokeAccess(
privateDataset.getDid,
"nobody@example.com",
ownerSession
)
}
}
```

```
sbt "FileService/testOnly *DatasetAccessResourceSpec *ModelAccessResourceSpec"
```

The existing neighbouring test, *"succeed as a no-op when the target user has no explicit grant"*, passes a **registered** user who holds no grant, which is why the gap was not caught.

---

### Version/Branch

1.4.0-incubating-SNAPSHOT (main)

### Commit Hash (Optional)

cd4fd5a6d3

### What browsers are you seeing the problem on?

_No response_

### Relevant log output

```shell
! java.lang.NullPointerException: Cannot invoke "org.apache.texera.dao.jooq.generated.tables.pojos.User.getUid()" because the return value of "org.apache.texera.dao.jooq.generated.tables.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)
! at org.apache.texera.dao.SqlServer$.$anonfun$withTransaction$1(SqlServer.scala:101)
! at org.jooq.impl.DefaultDSLContext.lambda$transaction$5(DefaultDSLContext.java:593)
! at org.jooq.impl.DefaultDSLContext.lambda$transactionResult0$3(DefaultDSLContext.java:531)
! at org.jooq.impl.Tools$3$1.block(Tools.java:6416)
! at java.base/java.util.concurrent.ForkJoinPool.unmanagedBlock(ForkJoinPool.java:3463)
! at java.base/java.util.concurrent.ForkJoinPool.managedBlock(ForkJoinPool.java:3434)
! at org.jooq.impl.Tools$3.get(Tools.java:6413)
! at org.jooq.impl.DefaultDSLContext.transactionResult0(DefaultDSLContext.java:579)
! at org.jooq.impl.DefaultDSLContext.transactionResult(DefaultDSLContext.java:502)
! at org.jooq.impl.DefaultDSLContext.transaction(DefaultDSLContext.java:592)
```

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.