From 48dd78064b01adf6f02230868c48f7fa984bba62 Mon Sep 17 00:00:00 2001 From: huyang Date: Thu, 24 Sep 2026 07:30:36 +0000 Subject: [PATCH 1/4] feat: separate deactivating a user from deleting them, and manage administrators Closes #18. `DELETE /scim2/Users/{id}` cleared every grant the principal held and then only marked the account DISABLED: advertised as deletion, behaving as deactivation, and destroying the grants on the way. The row survived, so the email stayed taken and could never be registered again. The two are now separate operations: - Deactivate leaves grants and owned objects untouched, so reactivating gives the account back as it was. Access stops on the next request, since the state is re-read per request. - Purge is the irreversible one, and the only thing that frees the email. It requires the principal typed back, refuses an account that is still active, refuses your own, and requires an heir when the user owns securables -- those objects change hands. Metastore administrators can be appointed and stood down from the Users page, under the invariant that at least one *enabled* administrator always remains. Fixed along the way: - `PATCH /scim2/Users/{id}` carried no authorization annotation (from upstream #500), and the access decorator lets a method without one through, so any signed-in account could deactivate any other -- an administrator included. It now requires metastore OWNER. - The administrator password sign-in never consulted the user table: a deactivated `admin` still received a session, whose every request was then refused. It now checks that the account exists and is enabled, and says which is wrong. - The bootstrap administrator could be deactivated by three separate routes, and stripped of its administrator status, either of which closes the password sign-in -- the way back in when the identity provider is unavailable, and something no restart repairs. All of them are refused. - The PatchOp schema typed `value` as a boolean, so the UI sent a scalar where RFC 7644 3.5.2.1 requires an object; the SCIM library rejected it while parsing and the user saw a JSON parse failure instead of a deactivated account. Both forms the RFC allows are now accepted. - Test servers raced for their port: it was picked, released, and only bound once Hibernate was up, failing whole test classes with `Address already in use` on a loaded runner. Co-Authored-By: Claude Opus 5 (1M context) --- api/control.yaml | 258 +++++++++++- deploy/README.md | 75 +++- deploy/README.zh-CN.md | 65 +++- .../server/UnityCatalogServer.java | 4 + .../server/auth/AllowingAuthorizer.java | 5 + .../server/auth/JCasbinAuthorizer.java | 10 + .../server/auth/MetastoreAdmins.java | 148 +++++++ .../server/auth/UnityCatalogAuthorizer.java | 14 + .../server/persist/UserRepository.java | 216 +++++++++++ .../server/service/AuthService.java | 34 ++ .../server/service/MetastoreAdminService.java | 93 +++++ .../server/service/Scim2UserService.java | 296 ++++++++++++-- .../server/base/BaseServerTest.java | 77 +++- .../access/SdkAccessControlBaseCRUDTest.java | 5 +- .../service/AuthServiceTokenLoginTest.java | 60 +++ .../service/BootstrapAdminProtectionTest.java | 236 +++++++++++ .../server/service/MetastoreAdminsTest.java | 250 ++++++++++++ .../server/service/Scim2UserPatchTest.java | 243 ++++++++++++ .../server/service/Scim2UserPurgeTest.java | 283 ++++++++++++++ ui/README.md | 27 ++ .../components/users/DeleteUserModal.test.tsx | 160 ++++++++ ui/src/components/users/DeleteUserModal.tsx | 224 +++++++++++ .../users/UserActionsDropdown.test.tsx | 260 +++++++++++++ .../components/users/UserActionsDropdown.tsx | 210 +++++++++- ui/src/hooks/users.test.tsx | 109 ++++++ ui/src/hooks/users.ts | 229 +++++++++++ ui/src/pages/UsersList.test.tsx | 107 +++++ ui/src/pages/UsersList.tsx | 73 +++- ui/src/types/api/control.gen.ts | 367 +++++++++++++++++- 29 files changed, 4072 insertions(+), 66 deletions(-) create mode 100644 server/src/main/java/io/unitycatalog/server/auth/MetastoreAdmins.java create mode 100644 server/src/main/java/io/unitycatalog/server/service/MetastoreAdminService.java create mode 100644 server/src/test/java/io/unitycatalog/server/service/BootstrapAdminProtectionTest.java create mode 100644 server/src/test/java/io/unitycatalog/server/service/MetastoreAdminsTest.java create mode 100644 server/src/test/java/io/unitycatalog/server/service/Scim2UserPatchTest.java create mode 100644 server/src/test/java/io/unitycatalog/server/service/Scim2UserPurgeTest.java create mode 100644 ui/src/components/users/DeleteUserModal.test.tsx create mode 100644 ui/src/components/users/DeleteUserModal.tsx create mode 100644 ui/src/components/users/UserActionsDropdown.test.tsx create mode 100644 ui/src/hooks/users.test.tsx create mode 100644 ui/src/pages/UsersList.test.tsx diff --git a/api/control.yaml b/api/control.yaml index cc7a5519..925e95d6 100644 --- a/api/control.yaml +++ b/api/control.yaml @@ -104,19 +104,124 @@ paths: application/scim+json: schema: $ref: '#/components/schemas/UserResource' + patch: + tags: + - Users + operationId: patchUser + summary: Patch a user + description: | + Applies a SCIM patch to the user that matches the supplied id. Only a `replace` operation + with no path is supported, whose value sets the user's `active` flag — this is how a user + is deactivated and reactivated. + + Deactivation takes effect on the next request: the server re-reads the user's state on + every call, so access tokens already issued stop working immediately. Nothing else about + the user changes — their grants, the objects they own and their email are left as they + were, which is what makes it reversible. + requestBody: + content: + application/json: + schema: + $ref: '#/components/schemas/PatchRequest' + responses: + '200': + description: The user was successfully patched. + content: + application/scim+json: + schema: {} + '501': + description: The patch contained no supported operation. delete: tags: - Users operationId: deleteUser - summary: Delete a user + summary: Deactivate or permanently delete a user description: | - Deletes the user that matches the supplied id. + Without `purge`, deactivates the user — the same state change as a SCIM patch setting + `active` to false, and equally reversible. + + With `purge=true`, removes the user record permanently. This is the only way to free the + user's email and externalId for reuse; it does not block access any harder than + deactivation, which already refuses the user on every request. Because it cannot be undone, + the server requires that: + + * the caller is a metastore administrator, and is not the user being deleted; + * the user is already deactivated; + * `confirm_principal` repeats the user's principal exactly; + * another enabled metastore administrator remains afterwards; + * the built-in `admin` account is never the target — it is the account behind the + password sign-in, the way back in when the identity provider is unavailable; + * `reassign_to` names an enabled user, if the target owns anything. + + Ownership transfers in both places it is recorded: the `owner` field of each object, and + the OWNER privileges that actually authorize. `created_by` and `updated_by` are left alone — + they record what happened, which stays true. + parameters: + - name: purge + in: query + description: | + Permanently remove the user record instead of deactivating them. Cannot be undone. + required: false + schema: + type: boolean + default: false + - name: reassign_to + in: query + description: | + Principal (email) taking ownership of everything the user owns. Required with + `purge=true` when the user owns anything; must name an enabled user other than the + target. Prefer the administrator performing the deletion over the built-in `admin` + account, which may itself be deactivated. + required: false + schema: + type: string + - name: confirm_principal + in: query + description: | + The target user's principal, repeated back. Required with `purge=true`. The user list + mixes people with Relyt instance principals, which look alike; this is what catches + the wrong row being selected. + required: false + schema: + type: string responses: '200': - description: The user was successfully deleted. + description: The user was successfully deactivated or deleted. content: application/scim+json: schema: {} + '400': + description: | + A guard refused the purge — missing or mismatched `confirm_principal`, the user is + still active, the target is the caller or the built-in administrator, no other enabled + administrator would remain, or `reassign_to` is unusable. + '409': + description: | + The user owns objects and no `reassign_to` was supplied. + /scim2/Users/{id}/ownedObjects: + parameters: + - name: id + in: path + description: The id of the user. + required: true + schema: + type: string + get: + tags: + - Users + operationId: getUserOwnedObjects + summary: List what a user owns + description: | + Every securable whose owner is this user, so that a caller can see the consequences of + deleting them before anything happens. One indexed query per securable type, unlike the + per-object walk needed to enumerate a user's privileges. + responses: + '200': + description: The owned objects were successfully retrieved. + content: + application/json: + schema: + $ref: '#/components/schemas/OwnedObjectsResponse' /scim2/Me: get: tags: @@ -132,6 +237,71 @@ paths: application/json: schema: $ref: '#/components/schemas/UserResource' + /metastore/admins: + get: + tags: + - Metastore + operationId: listMetastoreAdmins + summary: List the metastore's administrators + description: | + The users holding OWNER on the metastore, deactivated ones included. This is the only way + to see that: OWNER is filtered out of `/permissions` responses, so no other endpoint + reveals who holds it. + responses: + '200': + description: The administrators were successfully retrieved. + content: + application/json: + schema: + $ref: '#/components/schemas/MetastoreAdmins' + /metastore/admins/{email}: + parameters: + - name: email + in: path + description: The user's principal. + required: true + schema: + type: string + put: + tags: + - Metastore + operationId: grantMetastoreAdmin + summary: Make a user an administrator + description: | + Grants the user OWNER on the metastore, which is what being an administrator means: it + authorizes every securable, including ones they do not personally own. + + Any administrator may do this. Metastore OWNER is the top of the privilege lattice, so an + administrator granting one has escalated nobody past themselves — whereas restricting it + to the built-in `admin` account would make the break-glass password sign-in a participant + in routine administration. + + The user must exist and be active; ownership of a deactivated account authorizes nothing + and would only obscure who is really in charge. + responses: + '200': + description: The user is now an administrator. + '400': + description: The user is deactivated. + '404': + description: No such user. + delete: + tags: + - Metastore + operationId: revokeMetastoreAdmin + summary: Withdraw a user's administrator privilege + description: | + Revoking your own is allowed — handing over is a real thing to want — but only while + somebody else still holds it. At least one *enabled* administrator must remain: a + deactivated one cannot sign in, and a metastore left without one has no supported way back, + because the bootstrap re-grants OWNER only when the `admin` user does not exist at all. + responses: + '200': + description: The user is no longer an administrator. + '400': + description: This would leave the metastore without an enabled administrator. + '404': + description: No such user. /auth/capabilities: get: tags: @@ -489,6 +659,88 @@ components: description: | Whether the caller owns the metastore. Metastore owners are authorized for every securable, including ones they do not personally own. + MetastoreAdmins: + type: object + properties: + admins: + type: array + description: Principals holding OWNER on the metastore. + items: + type: string + PatchRequest: + description: | + A SCIM patch. Only a single `replace` operation with no path is honoured, and its value + sets the user's `active` flag. + type: object + properties: + schemas: + type: array + items: + type: string + example: + - urn:ietf:params:scim:api:messages:2.0:PatchOp + Operations: + type: array + items: + $ref: '#/components/schemas/PatchOperation' + PatchOperation: + type: object + properties: + op: + type: string + description: The patch operation; only `replace` is supported. + example: replace + path: + type: string + description: > + The attribute to replace. Omit it to send the attributes as an object in `value` + (the form Okta and this project's UI use); set it to `active` to send the bare value. + `active` is the only attribute this endpoint changes; anything else is answered with + 501. + example: active + value: + type: object + description: > + With no `path`, the object of attributes to replace — RFC 7644 §3.5.2.1 requires an + object here, not a bare value, and the server's SCIM library enforces that while + parsing, so a scalar is refused before any handler runs. With `path` set, this is + instead that attribute's new value (for `active`, a boolean). + properties: + active: + type: boolean + description: The user's new `active` state. + example: + active: false + OwnedObjectsResponse: + type: object + properties: + principal: + type: string + description: The user whose objects these are. + owned: + type: array + description: | + Every securable this user owns. Empty means the user can be deleted without naming + anyone to take ownership. + items: + $ref: '#/components/schemas/OwnedObject' + OwnedObject: + type: object + properties: + securable_type: + type: string + description: | + The kind of securable, named as in /permissions/{securable_type}/{full_name}. + example: table + full_name: + type: string + description: | + The securable's qualified name. Falls back to the bare name when an ancestor has + already been deleted — a catalog's removal does not clear what was under it. + example: sales.raw.orders + securable_id: + type: string + description: The securable's id. UserResource: description: SCIM provides a resource type for "User" resources. type: object diff --git a/deploy/README.md b/deploy/README.md index 103fda65..a0853b0e 100644 --- a/deploy/README.md +++ b/deploy/README.md @@ -294,11 +294,17 @@ friends run server-side); the administrator, as metastore owner, sees everything `deploy-uc.sh` warns when authorization is on but neither entry point is configured, since nobody could sign in. -**Signing in with an access token.** Whoever runs the server can sign in with the token it writes -to `etc/conf/token.txt` at `/login/token`, pasting it into the form or opening +**Signing in with an access token.** Paste a **user's** access token at `/login/token`, or open `/login/token?token=`; the page drops the token from the address bar once it is used. The server verifies the token the way it verifies any request and hands it back as the session -cookie, so the session ends when the token does. +cookie, so the session ends when the token does. You are signed in as the user the token's `sub` +names, with exactly that user's privileges. + +> ⚠️ The token in `etc/conf/token.txt` **cannot sign in.** Its `sub` is `server` — it is the +> server's own service token — and no user with that email is ever created, so +> `/auth/token/login` refuses it with `no enabled user named 'server'`. To get a token that can +> sign in, exchange one at `/auth/tokens`, or sign in at `/login/admin` and take the `UC_TOKEN` +> cookie. Earlier versions had the UI server inject that token into every API call instead, which made the UI's own address a way into an authorized server with no sign-in at all. The UI server now adds no @@ -437,6 +443,69 @@ password is stored in the rendered `server.properties` alongside the other secre constant time; a failed attempt is delayed by a second, which slows but does not stop guessing — use a long random value and keep the UI behind your usual network controls. +#### `admin` is a break-glass account, not an operations account + +Being an administrator is not an account type in UC; it is a fact: the user holds `OWNER` on the +metastore resource. That is exactly what `metastore_admin` from `GET /auth/capabilities` reports. +In principle it can be granted and revoked, and several people can hold it at once. + +On the UI's Users page an administrator can "Make administrator" / "Withdraw administrator" for any +**active** account, and administrators carry an `Admin` tag in the list. The endpoints behind it: + +```bash +# Promote +curl -X PUT -H "Authorization: Bearer $TOKEN" \ + "$UC_URL/api/1.0/unity-control/metastore/admins/ops@corp.com" + +# Withdraw +curl -X DELETE -H "Authorization: Bearer $TOKEN" \ + "$UC_URL/api/1.0/unity-control/metastore/admins/ops@corp.com" + +# See who they are (nothing else shows this: OWNER is filtered out of /permissions) +curl -H "Authorization: Bearer $TOKEN" \ + "$UC_URL/api/1.0/unity-control/metastore/admins" +``` + +**Any administrator may promote and withdraw, not just the built-in `admin`.** Metastore OWNER is +already the top of the privilege lattice, so an administrator granting one has escalated nobody +past themselves; restricting it to `admin` would instead drag the break-glass password account into +routine administration, which is the habit this section warns against. + +The server holds two lines: a deactivated account cannot be made an administrator (it cannot sign +in, so it would only obscure who is really in charge), and **at least one enabled administrator +must remain** after a withdrawal. The second is not ceremony: `UnityAccessUtil#initializeAdmin` +re-grants OWNER only when the `admin` user does not exist *at all*, so an `admin` row that survives +without the privilege is never repaired by a restart. + +> Background: `OWNER` is deliberately absent from the public `Privilege` enum, so it cannot travel +> through `/permissions` — hence the dedicated endpoints above. It is kept out of that enum because +> the enum is shared by every securable: admitting `OWNER` would simultaneously open ownership +> transfer on catalogs, schemas and tables, which is a separate feature needing its own design. + +**Run day-to-day administration from real accounts holding metastore OWNER, not from a shared +`admin` password.** Three reasons: + +1. **It bypasses every Entra control.** `/login/admin` never touches Entra, so MFA, conditional + access and "disabled on termination" do not apply to it. Share the password and someone disabled + in Entra after leaving still signs in to UC with the copy they kept. +2. **It destroys attribution.** In the server log — and in any audit record built later — the actor + is always `admin`, with no way to tell who acted. For deleting users, reassigning ownership or + handing out cloud credentials, that is the same as having no audit trail. +3. **It is the only way back in, and using it routinely is how you lose it.** The `admin` account + itself can be deactivated or have its privileges reduced (see the comment on + `UnityAccessUtil.initializeAdmin`), and the password sign-in **does not check that the account + still exists or is still ENABLED**. Once it is disabled or deleted, `/login/admin` still + returns success and sets the cookie, but every subsequent API call is rejected with 403 — a + dead end that looks like a working session. Keeping it for emergencies avoids ever getting there. + +**In production, leave `UC_ADMIN_PASSWORD` empty.** With it empty, `GET /auth/providers` does not +report `admin_login` and the `/login/admin` entry point exists neither on the login page nor on +the server. Set it and restart only when you actually need it (Entra outage, every administrator +locked out), then clear it again. + +> `/login/token` is a break-glass entry point of the same kind and should likewise not be a daily +> habit; the `etc/conf/token.txt` token it uses never expires — see [Security notes](#security-notes). + ### How programs reach UC Customer programs do not sign in with Microsoft identities. They go through Relyt, whose coordinator diff --git a/deploy/README.zh-CN.md b/deploy/README.zh-CN.md index db103edb..4683d432 100644 --- a/deploy/README.zh-CN.md +++ b/deploy/README.zh-CN.md @@ -248,9 +248,15 @@ UI 必须登录。**每个入口都有自己的地址，应用地址不是入口 `deploy-uc.sh` 会在"授权开着但一个登录入口都没配"时告警，因为那样没人能登录。 -**用访问令牌登录**：运维方可以拿服务端写在 `etc/conf/token.txt` 里的令牌，在 `/login/token` 粘贴， -或直接打开 `/login/token?token=<令牌>`，页面用完会立即把令牌从地址栏抹掉。服务端按校验普通请求的 -同一套规则校验它，然后**原样**作为会话 cookie 返回，所以会话不会比令牌活得久。 +**用访问令牌登录**：在 `/login/token` 粘贴一个**用户**的访问令牌，或直接打开 +`/login/token?token=<令牌>`，页面用完会立即把令牌从地址栏抹掉。服务端按校验普通请求的同一套规则 +校验它，然后**原样**作为会话 cookie 返回，所以会话不会比令牌活得久。 + +> ⚠️ `etc/conf/token.txt` 里那个令牌**不能用于登录**。它的 `sub` 是 `server`（服务令牌，供服务端自用）， +> 而 UC 里从来不存在 email 为 `server` 的用户——`/auth/token/login` 会以 +> `no enabled user named 'server'` 拒绝。这个入口登录成的是**令牌 `sub` 所指的那个用户**，权限也就是 +> 那个用户的权限；要拿一个可登录的令牌，走 `/auth/tokens` 交换，或用 `/login/admin` 登录后从 +> `UC_TOKEN` cookie 里取。 早期版本是由 UI server 把这个令牌注入到每个接口请求里，等于让 UI 的地址本身成为一条无需登录的入口。 现在 UI server 不再携带任何凭据。 @@ -370,6 +376,59 @@ issue 跟踪，不要在配置项里找。 `server.properties` 里，比对是常量时间的，失败会延迟一秒 —— 这能减缓但挡不住暴力猜测，请用足够长的随机 值，并把 UI 放在既有的网络管控之后。 +#### `admin` 是应急通道，不是日常运维账号 + +管理员权限在 UC 里不是一种账号类型，而是**一个事实**：某个用户在授权表里持有 metastore 的 `OWNER`。 +`GET /auth/capabilities` 返回的 `metastore_admin` 就是在报告这件事。原则上它可以授予和收回，也可以同时 +有多个持有者。 + +在 UI 的 Users 页面，管理员对任意**在用**账号可以「Make administrator」/「Withdraw administrator」， +列表里管理员带 `Admin` 标签。对应接口： + +```bash +# 提为管理员 +curl -X PUT -H "Authorization: Bearer $TOKEN" \ + "$UC_URL/api/1.0/unity-control/metastore/admins/ops@corp.com" + +# 收回 +curl -X DELETE -H "Authorization: Bearer $TOKEN" \ + "$UC_URL/api/1.0/unity-control/metastore/admins/ops@corp.com" + +# 看当前有哪些管理员（其它接口都看不到：OWNER 会被 /permissions 过滤掉） +curl -H "Authorization: Bearer $TOKEN" \ + "$UC_URL/api/1.0/unity-control/metastore/admins" +``` + +**任何管理员都能提拔和收回，不限于内置 `admin`。** metastore OWNER 已经是权限格的顶点——管理员提拔别人 +并没有把谁提到自己之上；反过来，只让 `admin` 有这个权力，等于把应急口令账号拉进日常运维，正是下面要 +避免的习惯。 + +服务端守着两条线：不能把停用账号提为管理员（它签不进来，只会让"谁在管"变得模糊），以及**收回后必须 +至少还剩一个在用的管理员**。后一条不是形式主义：`UnityAccessUtil.initializeAdmin` 只在 `admin` 用户 +**完全不存在**时才补授 OWNER，所以一个还在、但丢了 OWNER 的 `admin` 行，重启也修不回来。 + +> 历史说明：`OWNER` 不在公开的 `Privilege` 枚举里，所以它不能走 `/permissions` 通道，上面这组接口是 +> 专门为此开的。之所以不把 `OWNER` 塞进那个枚举：该枚举被所有 securable 共用，放进去等于同时给 +> catalog/schema/table 开了转移属主的口子，那是另一个需要单独设计的特性。 + +**日常运维请用被授予 metastore OWNER 的真人 M365 账号，不要共用 `admin` 口令。** 三个原因： + +1. **它绕过 Entra 的全部控制。** `/login/admin` 不经过 Entra，因此 MFA、条件访问、以及"员工离职即禁用" + 都对它无效。共用口令的后果是：某人离职后在 Entra 里被禁用，他照样能用记在本地的口令登进 UC。 +2. **它让操作无法归因。** 服务端日志和后续的审计记录里，操作者永远是 `admin`，分不出是谁做的。对于 + 删用户、改所有权、发放云凭证这类动作，这等于没有审计。 +3. **它是唯一的救命通道，用得越多越容易废掉。** `admin` 这个账号本身可以被停用或降权（见 + `UnityAccessUtil.initializeAdmin` 的注释），而口令登录**不检查该账号是否还存在、是否还是 ENABLED** + —— 账号被停用或删除后，`/login/admin` 仍然返回登录成功并种下 cookie，但随后每个接口调用都会被 + 403 拒绝，表现为"登录成功却什么都做不了"的死局。把它留着只做应急，就不会走到这一步。 + +**生产建议**：把 `UC_ADMIN_PASSWORD` 留空。留空时 `GET /auth/providers` 不会报告 `admin_login`， +`/login/admin` 这个入口在登录页和服务端都不存在。真需要应急时（Entra 故障、所有管理员都进不来） +再填上并重启，用完清空。 + +> `/login/token` 是同性质的应急入口，同样不要作为日常方式；它使用的 `etc/conf/token.txt` 令牌永不过期， +> 见[安全要点](#安全要点)。 + ### 程序怎么访问 UC 客户程序不以 M365 身份访问 UC。它们经 Relyt：coordinator 用登记在本地 JWKS 文件里的实例密钥向 UC 认证 diff --git a/server/src/main/java/io/unitycatalog/server/UnityCatalogServer.java b/server/src/main/java/io/unitycatalog/server/UnityCatalogServer.java index 0479de8f..a180d8c1 100644 --- a/server/src/main/java/io/unitycatalog/server/UnityCatalogServer.java +++ b/server/src/main/java/io/unitycatalog/server/UnityCatalogServer.java @@ -39,6 +39,7 @@ import io.unitycatalog.server.service.ExternalLocationService; import io.unitycatalog.server.service.FunctionService; import io.unitycatalog.server.service.IcebergRestCatalogService; +import io.unitycatalog.server.service.MetastoreAdminService; import io.unitycatalog.server.service.MetastoreService; import io.unitycatalog.server.service.ModelService; import io.unitycatalog.server.service.PermissionService; @@ -182,6 +183,8 @@ private void addApiServices( SelfCapabilitiesService selfCapabilitiesService = new SelfCapabilitiesService( authorizer, repositories, unityCatalogServerBuilder.serverProperties); + MetastoreAdminService metastoreAdminService = + new MetastoreAdminService(authorizer, repositories); CatalogService catalogService = new CatalogService(authorizer, repositories); SchemaService schemaService = new SchemaService(authorizer, repositories); VolumeService volumeService = new VolumeService(authorizer, repositories); @@ -229,6 +232,7 @@ private void addApiServices( requestConverterFunction, scimResponseConverterFunction) .annotatedService(CONTROL_PATH + "auth/capabilities", selfCapabilitiesService) + .annotatedService(CONTROL_PATH + "metastore/admins", metastoreAdminService) .annotatedService(BASE_PATH + "permissions", permissionService) .annotatedService(BASE_PATH + "catalogs", catalogService, requestConverterFunction) .annotatedService(BASE_PATH + "schemas", schemaService, requestConverterFunction) diff --git a/server/src/main/java/io/unitycatalog/server/auth/AllowingAuthorizer.java b/server/src/main/java/io/unitycatalog/server/auth/AllowingAuthorizer.java index 9780cdd7..794eef3f 100644 --- a/server/src/main/java/io/unitycatalog/server/auth/AllowingAuthorizer.java +++ b/server/src/main/java/io/unitycatalog/server/auth/AllowingAuthorizer.java @@ -77,4 +77,9 @@ public List listAuthorizations(UUID principal, UUID resource) { public Map> listAuthorizations(UUID resource) { return Map.of(); } + + @Override + public Map> listAuthorizationsForPrincipal(UUID principal) { + return Map.of(); + } } diff --git a/server/src/main/java/io/unitycatalog/server/auth/JCasbinAuthorizer.java b/server/src/main/java/io/unitycatalog/server/auth/JCasbinAuthorizer.java index addfd5ce..709ef2c3 100644 --- a/server/src/main/java/io/unitycatalog/server/auth/JCasbinAuthorizer.java +++ b/server/src/main/java/io/unitycatalog/server/auth/JCasbinAuthorizer.java @@ -139,4 +139,14 @@ public Map> listAuthorizations(UUID resource) { Collectors.mapping( l -> Privileges.fromValue(l.get(PRIVILEGE_INDEX)), Collectors.toList()))); } + + @Override + public Map> listAuthorizationsForPrincipal(UUID principal) { + return enforcer.getFilteredPolicy(PRINCIPAL_INDEX, principal.toString()).stream() + .collect( + Collectors.groupingBy( + l -> UUID.fromString(l.get(RESOURCE_INDEX)), + Collectors.mapping( + l -> Privileges.fromValue(l.get(PRIVILEGE_INDEX)), Collectors.toList()))); + } } diff --git a/server/src/main/java/io/unitycatalog/server/auth/MetastoreAdmins.java b/server/src/main/java/io/unitycatalog/server/auth/MetastoreAdmins.java new file mode 100644 index 00000000..61d04e32 --- /dev/null +++ b/server/src/main/java/io/unitycatalog/server/auth/MetastoreAdmins.java @@ -0,0 +1,148 @@ +package io.unitycatalog.server.auth; + +import io.unitycatalog.control.model.User; +import io.unitycatalog.server.exception.BaseException; +import io.unitycatalog.server.exception.ErrorCode; +import io.unitycatalog.server.persist.MetastoreRepository; +import io.unitycatalog.server.persist.Repositories; +import io.unitycatalog.server.persist.UserRepository; +import io.unitycatalog.server.persist.model.Privileges; +import java.util.List; +import java.util.Map; +import java.util.Objects; +import java.util.UUID; +import java.util.stream.Collectors; + +/** + * Who administers this metastore. + * + *

Being an administrator is not an account type: it is holding OWNER on the metastore resource, + * which is what {@code GET /auth/capabilities} reports and what every {@code #authorize(#principal, + * #metastore, OWNER)} rule tests. This class is the one place that reads and changes that fact, so + * the invariant below cannot be enforced in one caller and forgotten in another. + * + *

The invariant: at least one enabled administrator always remains. Without it a + * metastore can be left with nobody able to administer it and no supported way back, because + * UnityAccessUtil#initializeAdmin re-grants OWNER only when the {@code admin} user does not exist + * at all — a surviving {@code admin} row that has lost the privilege is never repaired by a + * restart. + */ +public class MetastoreAdmins { + + /** The account behind the password sign-in; see AuthService#adminLogin. */ + private static final String BOOTSTRAP_ADMIN_EMAIL = "admin"; + + private final UnityCatalogAuthorizer authorizer; + private final UserRepository userRepository; + private final MetastoreRepository metastoreRepository; + + public MetastoreAdmins(UnityCatalogAuthorizer authorizer, Repositories repositories) { + this.authorizer = authorizer; + this.userRepository = repositories.getUserRepository(); + this.metastoreRepository = repositories.getMetastoreRepository(); + } + + /** + * Administrator principals, including deactivated ones, so a caller can see the whole picture. + */ + public List list() { + return holders().stream().map(this::emailOrNull).filter(Objects::nonNull).sorted().toList(); + } + + /** + * Makes the user an administrator. + * + *

Any administrator may do this. Restricting it to the built-in {@code admin} account would + * buy nothing — metastore OWNER is already the top of the privilege lattice, so an administrator + * granting one has escalated nobody past themselves — while making the break-glass password + * account a participant in routine administration, which is the habit the deployment guide warns + * against. + */ + public void grant(String email) { + User user = requireUser(email); + if (user.getState() != User.StateEnum.ENABLED) { + throw new BaseException( + ErrorCode.INVALID_ARGUMENT, "Cannot make a deactivated user an administrator: " + email); + } + authorizer.grantAuthorization( + UUID.fromString(Objects.requireNonNull(user.getId())), + metastoreRepository.getMetastoreId(), + Privileges.OWNER); + } + + /** Takes the privilege away, provided somebody is left holding it. */ + public void revoke(String email) { + // The bootstrap administrator keeps it. It is the account behind the password sign-in, and a + // sign-in that works but administers nothing is no way back in. Worse, nothing repairs it: + // UnityAccessUtil#initializeAdmin re-grants OWNER only when the account does not exist, so a + // surviving row that has lost the privilege stays that way across restarts. + if (BOOTSTRAP_ADMIN_EMAIL.equals(email)) { + throw new BaseException( + ErrorCode.INVALID_ARGUMENT, + "The bootstrap administrator cannot lose administrator status; it is the way back in" + + " when the identity provider is unavailable, and no restart restores it."); + } + User user = requireUser(email); + UUID principalId = UUID.fromString(Objects.requireNonNull(user.getId())); + requireAnotherAdminRemains(principalId); + authorizer.revokeAuthorization( + principalId, metastoreRepository.getMetastoreId(), Privileges.OWNER); + } + + public boolean isAdmin(UUID principalId) { + return authorizer.authorize( + principalId, metastoreRepository.getMetastoreId(), Privileges.OWNER); + } + + /** + * Refuses an operation that would leave no enabled administrator once {@code losingAdmin} is no + * longer one — whether that is a revocation or the deletion of the account itself. + * + *

A policy naming a user who is deactivated, or who no longer exists, does not count: it + * cannot be used to sign in, so it would not keep the metastore administrable. + */ + public void requireAnotherAdminRemains(UUID losingAdmin) { + boolean remains = + holders().stream().filter(id -> !id.equals(losingAdmin)).anyMatch(this::isEnabled); + if (!remains) { + throw new BaseException( + ErrorCode.FAILED_PRECONDITION, + "Refusing to leave the metastore without an enabled administrator; make another user an" + + " administrator first."); + } + } + + private List holders() { + return authorizer.listAuthorizations(metastoreRepository.getMetastoreId()).entrySet().stream() + .filter(entry -> entry.getValue().contains(Privileges.OWNER)) + .map(Map.Entry::getKey) + .collect(Collectors.toList()); + } + + private User requireUser(String email) { + try { + return userRepository.getUserByEmail(email); + } catch (BaseException e) { + throw new BaseException(ErrorCode.NOT_FOUND, "User not found: " + email); + } + } + + private boolean isEnabled(UUID principalId) { + User user = userOrNull(principalId); + return user != null && user.getState() == User.StateEnum.ENABLED; + } + + private String emailOrNull(UUID principalId) { + User user = userOrNull(principalId); + return user == null ? null : user.getEmail(); + } + + private User userOrNull(UUID principalId) { + try { + return userRepository.getUser(principalId.toString()); + } catch (Exception e) { + // A policy left behind by a user that no longer exists names nobody. + return null; + } + } +} diff --git a/server/src/main/java/io/unitycatalog/server/auth/UnityCatalogAuthorizer.java b/server/src/main/java/io/unitycatalog/server/auth/UnityCatalogAuthorizer.java index 8e4e859e..5687f191 100644 --- a/server/src/main/java/io/unitycatalog/server/auth/UnityCatalogAuthorizer.java +++ b/server/src/main/java/io/unitycatalog/server/auth/UnityCatalogAuthorizer.java @@ -41,4 +41,18 @@ public interface UnityCatalogAuthorizer { List listAuthorizations(UUID principal, UUID resource); Map> listAuthorizations(UUID resource); + + /** + * Every authorization held by one principal, keyed by the resource it applies to. + * + *

The inverse of {@link #listAuthorizations(UUID)}, and the only way to answer "what does this + * user hold?" without walking the whole catalog tree and asking per object. Deleting a user needs + * exactly this: the resources they own have to be handed to someone else before their policies go + * away. + * + *

The resources are whatever the policy store still names, which is not the same as what still + * exists — deleting a catalog does not clear the policies of the schemas and tables under it (see + * CatalogService#deleteCatalog), so callers that resolve these ids must tolerate misses. + */ + Map> listAuthorizationsForPrincipal(UUID principal); } diff --git a/server/src/main/java/io/unitycatalog/server/persist/UserRepository.java b/server/src/main/java/io/unitycatalog/server/persist/UserRepository.java index 847e53c6..8695ac20 100644 --- a/server/src/main/java/io/unitycatalog/server/persist/UserRepository.java +++ b/server/src/main/java/io/unitycatalog/server/persist/UserRepository.java @@ -1,9 +1,18 @@ package io.unitycatalog.server.persist; +import com.fasterxml.jackson.annotation.JsonProperty; import io.unitycatalog.control.model.User; import io.unitycatalog.server.exception.BaseException; import io.unitycatalog.server.exception.ErrorCode; +import io.unitycatalog.server.persist.dao.CatalogInfoDAO; +import io.unitycatalog.server.persist.dao.CredentialDAO; +import io.unitycatalog.server.persist.dao.ExternalLocationDAO; +import io.unitycatalog.server.persist.dao.FunctionInfoDAO; +import io.unitycatalog.server.persist.dao.RegisteredModelInfoDAO; +import io.unitycatalog.server.persist.dao.SchemaInfoDAO; +import io.unitycatalog.server.persist.dao.TableInfoDAO; import io.unitycatalog.server.persist.dao.UserDAO; +import io.unitycatalog.server.persist.dao.VolumeInfoDAO; import io.unitycatalog.server.persist.model.CreateUser; import io.unitycatalog.server.persist.model.UpdateUser; import io.unitycatalog.server.persist.utils.PagedListingHelper; @@ -11,7 +20,9 @@ import io.unitycatalog.server.utils.IdentityUtils; import io.unitycatalog.server.utils.ValidationUtils; import java.util.ArrayList; +import java.util.HashMap; import java.util.List; +import java.util.Map; import java.util.Optional; import java.util.UUID; import java.util.function.Predicate; @@ -198,6 +209,211 @@ public void deleteUser(String id) { /* readOnly = */ false); } + /** + * Every entity carrying an {@code owner} column that names a user by email. + * + *

Ownership lives in two places that have to move together: this column, which is metadata (no + * server-side authorization reads it — the UI does, to decide whether to enable a button), and + * the OWNER rows in the policy store, which are what actually authorize. Reassigning one without + * the other leaves an object whose displayed owner and effective owner disagree. + * + *

{@code ModelVersionInfoDAO} is here but absent from {@link #listOwnedSecurables}: it has an + * owner column, yet MODEL_VERSION is not a SecurableType — a version is governed by its + * registered model — so a version follows a reassignment without ever blocking one. + */ + private static final List OWNER_BEARING_ENTITIES = + List.of( + "CatalogInfoDAO", + "SchemaInfoDAO", + "TableInfoDAO", + "VolumeInfoDAO", + "FunctionInfoDAO", + "RegisteredModelInfoDAO", + "ModelVersionInfoDAO", + "ExternalLocationDAO", + "CredentialDAO"); + + /** + * A securable whose {@code owner} column names a particular user. + * + *

Serialized with the snake_case names the rest of the catalog API uses for securables, so a + * caller can feed {@code securable_type} and {@code full_name} straight back into + * /permissions/{securable_type}/{full_name}. + */ + public record OwnedSecurable( + @JsonProperty("securable_type") String securableType, + @JsonProperty("full_name") String fullName, + @JsonProperty("securable_id") String securableId) {} + + /** + * Everything the given user owns, named the way an administrator would recognise it. + * + *

One indexed query per entity type, not the per-object walk the UI's access view has to do: + * this has to be cheap enough to run before every delete. + */ + public List listOwnedSecurables(String ownerEmail) { + return TransactionManager.executeWithTransaction( + sessionFactory, + session -> { + List owned = new ArrayList<>(); + AncestorNames names = new AncestorNames(session); + + // Top-level securables: the name is already the full name. + for (CatalogInfoDAO dao : ownedBy(session, CatalogInfoDAO.class, ownerEmail)) { + owned.add(securable("catalog", dao.getName(), dao.getId())); + } + for (ExternalLocationDAO dao : ownedBy(session, ExternalLocationDAO.class, ownerEmail)) { + owned.add(securable("external_location", dao.getName(), dao.getId())); + } + for (CredentialDAO dao : ownedBy(session, CredentialDAO.class, ownerEmail)) { + owned.add(securable("credential", dao.getName(), dao.getId())); + } + + for (SchemaInfoDAO dao : ownedBy(session, SchemaInfoDAO.class, ownerEmail)) { + String parent = names.catalog(dao.getCatalogId()); + owned.add(securable("schema", qualify(parent, dao.getName()), dao.getId())); + } + for (TableInfoDAO dao : ownedBy(session, TableInfoDAO.class, ownerEmail)) { + String parent = names.schema(dao.getSchemaId()); + owned.add(securable("table", qualify(parent, dao.getName()), dao.getId())); + } + for (VolumeInfoDAO dao : ownedBy(session, VolumeInfoDAO.class, ownerEmail)) { + String parent = names.schema(dao.getSchemaId()); + owned.add(securable("volume", qualify(parent, dao.getName()), dao.getId())); + } + for (FunctionInfoDAO dao : ownedBy(session, FunctionInfoDAO.class, ownerEmail)) { + String parent = names.schema(dao.getSchemaId()); + owned.add(securable("function", qualify(parent, dao.getName()), dao.getId())); + } + for (RegisteredModelInfoDAO dao : + ownedBy(session, RegisteredModelInfoDAO.class, ownerEmail)) { + String parent = names.schema(dao.getSchemaId()); + owned.add(securable("registered_model", qualify(parent, dao.getName()), dao.getId())); + } + + return owned; + }, + "Failed to list owned securables", + /* readOnly = */ true); + } + + /** + * Moves every {@code owner} column naming {@code fromEmail} to {@code toEmail}. + * + *

{@code created_by} and {@code updated_by} are deliberately left alone: they record who did + * something, which stays true after the account is gone. Only ownership — a live responsibility — + * transfers. + * + *

This covers the metadata half of ownership only. The caller is responsible for granting the + * new owner the corresponding OWNER policies; see {@link #OWNER_BEARING_ENTITIES}. + */ + public void reassignOwnership(String fromEmail, String toEmail) { + TransactionManager.executeWithTransaction( + sessionFactory, + session -> { + int reassigned = 0; + for (String entity : OWNER_BEARING_ENTITIES) { + reassigned += + session + .createMutationQuery( + "UPDATE " + entity + " SET owner = :newOwner WHERE owner = :oldOwner") + .setParameter("newOwner", toEmail) + .setParameter("oldOwner", fromEmail) + .executeUpdate(); + } + LOGGER.info("Reassigned {} object(s) from {} to {}", reassigned, fromEmail, toEmail); + return null; + }, + "Failed to reassign ownership", + /* readOnly = */ false); + } + + /** + * Removes the user row outright, which is what frees the email and externalId for reuse. + * + *

Unlike {@link #deleteUser}, this cannot be undone and is not what stops the user reaching + * the server — a DISABLED state already does that on every request. Callers must have dealt with + * the user's ownership and policies first. + */ + public void purgeUser(String id) { + TransactionManager.executeWithTransaction( + sessionFactory, + session -> { + UserDAO userDAO = getUserById(session, id); + if (userDAO == null) { + throw new BaseException(ErrorCode.NOT_FOUND, "User not found: " + id); + } + session.remove(userDAO); + LOGGER.info("Purged user: {} ({})", id, userDAO.getEmail()); + return null; + }, + "Failed to purge user", + /* readOnly = */ false); + } + + private static List ownedBy(Session session, Class entity, String ownerEmail) { + return session + .createQuery("FROM " + entity.getSimpleName() + " WHERE owner = :owner", entity) + .setParameter("owner", ownerEmail) + .getResultList(); + } + + private static OwnedSecurable securable(String type, String fullName, UUID id) { + return new OwnedSecurable(type, fullName, id == null ? null : id.toString()); + } + + /** {@code parent.name}, or the bare name when the parent could not be resolved. */ + private static String qualify(String parent, String name) { + return parent == null ? name : parent + "." + name; + } + + /** + * Resolves the qualified names of the catalogs and schemas an owned object sits under, looking + * each one up at most once. + * + *

Every lookup returns null rather than throwing when the ancestor is already gone: deleting a + * catalog does not remove what was under it from the policy store (CatalogService#deleteCatalog + * clears only the catalog's own), so a user's owned set can name objects whose parents no longer + * exist — and a delete must not be blocked by one of those. + */ + private static final class AncestorNames { + private final Session session; + private final Map catalogs = new HashMap<>(); + private final Map schemas = new HashMap<>(); + + AncestorNames(Session session) { + this.session = session; + } + + /** The catalog's name, or null. */ + String catalog(UUID catalogId) { + if (catalogId == null) { + return null; + } + if (catalogs.containsKey(catalogId)) { + return catalogs.get(catalogId); + } + CatalogInfoDAO dao = session.get(CatalogInfoDAO.class, catalogId); + String name = dao == null ? null : dao.getName(); + catalogs.put(catalogId, name); + return name; + } + + /** {@code catalog.schema}, or null. */ + String schema(UUID schemaId) { + if (schemaId == null) { + return null; + } + if (schemas.containsKey(schemaId)) { + return schemas.get(schemaId); + } + SchemaInfoDAO dao = session.get(SchemaInfoDAO.class, schemaId); + String name = dao == null ? null : qualify(catalog(dao.getCatalogId()), dao.getName()); + schemas.put(schemaId, name); + return name; + } + } + public UUID findPrincipalId() { String principalEmailAddress = IdentityUtils.findPrincipalEmailAddress(); if (principalEmailAddress != null) { diff --git a/server/src/main/java/io/unitycatalog/server/service/AuthService.java b/server/src/main/java/io/unitycatalog/server/service/AuthService.java index 0d69ce88..5eba1fb2 100644 --- a/server/src/main/java/io/unitycatalog/server/service/AuthService.java +++ b/server/src/main/java/io/unitycatalog/server/service/AuthService.java @@ -36,6 +36,7 @@ import io.unitycatalog.control.model.TokenEndpointExtensionType; import io.unitycatalog.control.model.TokenType; import io.unitycatalog.control.model.User; +import io.unitycatalog.server.exception.BaseException; import io.unitycatalog.server.exception.ErrorCode; import io.unitycatalog.server.exception.GlobalExceptionHandler; import io.unitycatalog.server.exception.OAuthInvalidRequestException; @@ -556,6 +557,12 @@ public HttpResponse adminLogin(ServiceRequestContext ctx, AggregatedHttpRequest ErrorCode.UNAUTHENTICATED, "Wrong username or password"); } + // The password alone does not make a session. The account it names must exist and be enabled, + // exactly as AuthDecorator requires of every request made with the result. Without this a + // deactivated administrator still got a session and then had every request refused — a sign-in + // that looked like it worked, with nothing saying why nothing else did. + requireTheAdminAccountCanSignIn(); + String accessToken = securityContext.createAccessToken(ADMIN_USERNAME, serverProperties.getAccessTokenTtl()); OAuthTokenExchangeInfo info = @@ -572,6 +579,33 @@ public HttpResponse adminLogin(ServiceRequestContext ctx, AggregatedHttpRequest info); } + /** + * Refuses the password sign-in when the {@code admin} account cannot actually be used. + * + *

The password is checked against configuration, not against the user table, so on its own it + * says nothing about whether the account still exists or is still enabled. Both are required of + * every request the resulting session makes, so letting the sign-in succeed regardless produced a + * session that could do nothing at all. + */ + private void requireTheAdminAccountCanSignIn() { + User admin; + try { + admin = userRepository.getUserByEmail(ADMIN_USERNAME); + } catch (BaseException e) { + LOGGER.warn( + "Administrator sign-in rejected: no '{}' account on this metastore", ADMIN_USERNAME); + throw new OAuthInvalidRequestException( + ErrorCode.UNAUTHENTICATED, "The administrator account does not exist on this metastore."); + } + if (admin.getState() != User.StateEnum.ENABLED) { + LOGGER.warn( + "Administrator sign-in rejected: the '{}' account is deactivated", ADMIN_USERNAME); + throw new OAuthInvalidRequestException( + ErrorCode.UNAUTHENTICATED, + "The administrator account is deactivated; another administrator can reactivate it."); + } + } + /** * Signs a browser in with a Unity Catalog access token the operator already holds -- the one in * etc/conf/token.txt, typically. It replaces the proxy's blanket token injection, which made the diff --git a/server/src/main/java/io/unitycatalog/server/service/MetastoreAdminService.java b/server/src/main/java/io/unitycatalog/server/service/MetastoreAdminService.java new file mode 100644 index 00000000..593b0ed0 --- /dev/null +++ b/server/src/main/java/io/unitycatalog/server/service/MetastoreAdminService.java @@ -0,0 +1,93 @@ +package io.unitycatalog.server.service; + +import static io.unitycatalog.server.model.SecurableType.METASTORE; + +import com.linecorp.armeria.common.HttpResponse; +import com.linecorp.armeria.common.HttpStatus; +import com.linecorp.armeria.server.annotation.Delete; +import com.linecorp.armeria.server.annotation.ExceptionHandler; +import com.linecorp.armeria.server.annotation.Get; +import com.linecorp.armeria.server.annotation.Param; +import com.linecorp.armeria.server.annotation.ProducesJson; +import com.linecorp.armeria.server.annotation.Put; +import com.linecorp.armeria.server.annotation.StatusCode; +import io.unitycatalog.server.auth.MetastoreAdmins; +import io.unitycatalog.server.auth.UnityCatalogAuthorizer; +import io.unitycatalog.server.auth.annotation.AuthorizeExpression; +import io.unitycatalog.server.auth.annotation.AuthorizeResourceKey; +import io.unitycatalog.server.exception.GlobalExceptionHandler; +import io.unitycatalog.server.persist.Repositories; +import io.unitycatalog.server.utils.IdentityUtils; +import java.util.List; +import java.util.Map; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +/** + * Managing who administers the metastore. + * + *

An administrator is a user holding OWNER on the metastore, and until this service there was no + * way to make one: OWNER is deliberately absent from the public Privilege enum, so it cannot travel + * through /permissions, and the only grant in the codebase was the bootstrap that runs once on an + * empty database. A deployment therefore had exactly one administrator — the built-in {@code admin} + * account reached with a shared password — and no supported way to promote a real person, which is + * the opposite of what the deployment guide asks for. + * + *

A dedicated endpoint rather than adding OWNER to Privilege: that enum is shared by every + * securable, so admitting OWNER would simultaneously open ownership transfer on catalogs, schemas + * and tables through /permissions — a larger feature that needs its own design. Keeping this narrow + * also keeps the "one enabled administrator always remains" invariant in one place. + */ +@ExceptionHandler(GlobalExceptionHandler.class) +public class MetastoreAdminService { + private static final Logger LOGGER = LoggerFactory.getLogger(MetastoreAdminService.class); + + private final MetastoreAdmins metastoreAdmins; + + public MetastoreAdminService(UnityCatalogAuthorizer authorizer, Repositories repositories) { + this.metastoreAdmins = new MetastoreAdmins(authorizer, repositories); + } + + /** + * The metastore's administrators. + * + *

Readable by any administrator, and the only way to see this at all: OWNER is filtered out of + * /permissions responses, so no other endpoint reveals who holds it. + */ + @Get("") + @ProducesJson + @StatusCode(200) + @AuthorizeExpression("#authorizeAny(#principal, #metastore, OWNER)") + @AuthorizeResourceKey(METASTORE) + public Map> listAdmins() { + return Map.of("admins", metastoreAdmins.list()); + } + + @Put("/{email}") + @AuthorizeExpression("#authorizeAny(#principal, #metastore, OWNER)") + @AuthorizeResourceKey(METASTORE) + public HttpResponse grantAdmin(@Param("email") String email) { + metastoreAdmins.grant(email); + LOGGER.warn( + "{} made {} a metastore administrator", IdentityUtils.findPrincipalEmailAddress(), email); + return HttpResponse.of(HttpStatus.OK); + } + + /** + * Takes the privilege away. + * + *

Revoking your own is allowed — handing over is a real thing to want — but only while someone + * else still holds it, which {@link MetastoreAdmins} enforces for every caller. + */ + @Delete("/{email}") + @AuthorizeExpression("#authorizeAny(#principal, #metastore, OWNER)") + @AuthorizeResourceKey(METASTORE) + public HttpResponse revokeAdmin(@Param("email") String email) { + metastoreAdmins.revoke(email); + LOGGER.warn( + "{} revoked the metastore administrator privilege from {}", + IdentityUtils.findPrincipalEmailAddress(), + email); + return HttpResponse.of(HttpStatus.OK); + } +} diff --git a/server/src/main/java/io/unitycatalog/server/service/Scim2UserService.java b/server/src/main/java/io/unitycatalog/server/service/Scim2UserService.java index ca254392..0fa31b15 100644 --- a/server/src/main/java/io/unitycatalog/server/service/Scim2UserService.java +++ b/server/src/main/java/io/unitycatalog/server/service/Scim2UserService.java @@ -3,7 +3,7 @@ import static io.unitycatalog.server.model.SecurableType.METASTORE; import static io.unitycatalog.server.utils.Scim2Utils.asUserResource; -import com.fasterxml.jackson.core.JsonProcessingException; +import com.fasterxml.jackson.databind.JsonNode; import com.linecorp.armeria.common.HttpResponse; import com.linecorp.armeria.common.HttpStatus; import com.linecorp.armeria.server.annotation.Delete; @@ -13,13 +13,13 @@ import com.linecorp.armeria.server.annotation.Patch; import com.linecorp.armeria.server.annotation.Post; import com.linecorp.armeria.server.annotation.Produces; +import com.linecorp.armeria.server.annotation.ProducesJson; import com.linecorp.armeria.server.annotation.Put; import com.linecorp.armeria.server.annotation.StatusCode; import com.unboundid.scim2.common.exceptions.BadRequestException; import com.unboundid.scim2.common.exceptions.PreconditionFailedException; import com.unboundid.scim2.common.exceptions.ResourceConflictException; import com.unboundid.scim2.common.exceptions.ScimException; -import com.unboundid.scim2.common.exceptions.ServerErrorException; import com.unboundid.scim2.common.filters.Filter; import com.unboundid.scim2.common.messages.ListResponse; import com.unboundid.scim2.common.messages.PatchOpType; @@ -31,6 +31,7 @@ import com.unboundid.scim2.common.utils.FilterEvaluator; import com.unboundid.scim2.common.utils.Parser; import io.unitycatalog.control.model.User; +import io.unitycatalog.server.auth.MetastoreAdmins; import io.unitycatalog.server.auth.UnityCatalogAuthorizer; import io.unitycatalog.server.auth.annotation.AuthorizeExpression; import io.unitycatalog.server.auth.annotation.AuthorizeResourceKey; @@ -41,13 +42,19 @@ import io.unitycatalog.server.persist.Repositories; import io.unitycatalog.server.persist.UserRepository; import io.unitycatalog.server.persist.model.CreateUser; +import io.unitycatalog.server.persist.model.Privileges; import io.unitycatalog.server.persist.model.UpdateUser; +import io.unitycatalog.server.utils.IdentityUtils; import io.unitycatalog.server.utils.Scim2Utils; import java.util.Calendar; +import java.util.Iterator; import java.util.List; +import java.util.Map; import java.util.Objects; import java.util.Optional; import java.util.UUID; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; /** * SCIM2-compliant user management. @@ -65,12 +72,22 @@ */ @ExceptionHandler(GlobalExceptionHandler.class) public class Scim2UserService { + private static final Logger LOGGER = LoggerFactory.getLogger(Scim2UserService.class); + + /** The account behind the password sign-in; see AuthService#adminLogin. */ + private static final String BOOTSTRAP_ADMIN_EMAIL = "admin"; + + /** The only attribute a PATCH may change here. */ + private static final String ACTIVE_ATTRIBUTE = "active"; + private final UserRepository userRepository; + private final MetastoreAdmins metastoreAdmins; private final UnityCatalogAuthorizer authorizer; public Scim2UserService(UnityCatalogAuthorizer authorizer, Repositories repositories) { this.authorizer = authorizer; this.userRepository = repositories.getUserRepository(); + this.metastoreAdmins = new MetastoreAdmins(authorizer, repositories); } @Get("") @@ -172,6 +189,12 @@ public UserResource updateUser(@Param("id") String id, UserResource userResource throw new Scim2RuntimeException(new ResourceConflictException("User id mismatch.")); } + // A PUT writes `active` back like any other field, so it is a third way to deactivate an + // account and needs the same guard as PATCH and DELETE. + if (Boolean.FALSE.equals(userResource.getActive())) { + requireNotTheBootstrapAdministrator(userRepository.getUser(id)); + } + UpdateUser updateUser = UpdateUser.builder() .name(userResource.getDisplayName()) @@ -182,46 +205,269 @@ public UserResource updateUser(@Param("id") String id, UserResource userResource return asUserResource(userRepository.updateUser(id, updateUser)); } + /** + * Everything the user owns, for a caller about to decide whether it is safe to delete them. + * + *

Read-only and cheap: one indexed query per securable type, not the per-object walk the UI's + * access view has to perform. A delete can rely on the same scan (see {@link #deleteUser}); this + * endpoint exists so the confirmation dialog can show the consequences before anything happens + * rather than discovering them from a rejection. + */ + @Get("/{id}/ownedObjects") + @ProducesJson + @StatusCode(200) + @AuthorizeExpression("#authorizeAny(#principal, #metastore, OWNER)") + @AuthorizeResourceKey(METASTORE) + public Map getOwnedObjects(@Param("id") String id) { + User user = userRepository.getUser(id); + return Map.of( + "principal", user.getEmail(), + "owned", userRepository.listOwnedSecurables(user.getEmail())); + } + + /** + * Deactivates the user, or — with {@code purge=true} — removes them permanently. + * + *

Deactivation is the default and is a pure state change: the user is refused on every request + * from that moment (AuthDecorator re-reads the state per request, so tokens already issued stop + * working immediately), while their grants, the objects they own and their email stay exactly as + * they were. That is what makes it reversible. + * + *

Purging is the irreversible one, and the only reason to reach for it: it is what frees the + * email and externalId for reuse. It does not block access any harder than deactivation already + * does. Everything it needs to be safe is checked here rather than trusted to the caller. + */ @Delete("/{id}") @AuthorizeExpression("#authorizeAny(#principal, #metastore, OWNER)") @AuthorizeResourceKey(METASTORE) - public HttpResponse deleteUser(@Param("id") String id) { + public HttpResponse deleteUser( + @Param("id") String id, + @Param("purge") Optional purge, + @Param("reassign_to") Optional reassignTo, + @Param("confirm_principal") Optional confirmPrincipal) { User user = userRepository.getUser(id); - authorizer.clearAuthorizationsForPrincipal( - UUID.fromString(Objects.requireNonNull(user.getId()))); - userRepository.deleteUser(user.getId()); + + if (!purge.map(Boolean::parseBoolean).orElse(false)) { + // Deactivate. The user's authorizations are deliberately left untouched: a deactivation that + // silently erased them would be advertised as reversible while being anything but -- on + // reactivation the user would find they no longer own what they created, with nothing in the + // UI to explain it. + requireNotTheBootstrapAdministrator(user); + userRepository.deleteUser(user.getId()); + return HttpResponse.of(HttpStatus.OK); + } + + purgeUser(user, reassignTo, confirmPrincipal); return HttpResponse.of(HttpStatus.OK); } + private void purgeUser( + User user, Optional reassignTo, Optional confirmPrincipal) { + String targetEmail = user.getEmail(); + UUID targetId = UUID.fromString(Objects.requireNonNull(user.getId())); + + // These refusals are reported as BaseException rather than the SCIM errors the rest of this + // service raises, because a Scim2RuntimeException is answered with HTTP 500 whatever status + // the SCIM error carries (see GlobalExceptionHandler) -- a guard that fired deliberately would + // reach the caller as a server fault, with the explanation stripped out on the way. A purge is + // a Unity Catalog extension rather than a SCIM operation, so a Unity Catalog error fits it. + + // Typing the principal back is the only check that catches "right button, wrong row". It + // matters more here than it looks: the user list mixes people with Relyt instance principals + // (whose email column holds an instance identifier), and deleting one of those takes a whole + // compute instance off UC. + if (confirmPrincipal.filter(targetEmail::equals).isEmpty()) { + throw new BaseException( + ErrorCode.INVALID_ARGUMENT, + "confirm_principal must be set to '" + + targetEmail + + "' to permanently delete this user."); + } + + if (targetEmail.equals(IdentityUtils.findPrincipalEmailAddress())) { + throw new BaseException( + ErrorCode.INVALID_ARGUMENT, + "A user cannot permanently delete their own account. Ask another administrator."); + } + + // The bootstrap administrator is the account behind the password sign-in, which is the way back + // in when the identity provider is unavailable. Worse, that sign-in does not check whether the + // account still exists: delete it and /auth/admin/login still returns a session, but every + // request made with it is rejected -- a dead end that looks like it worked. + if (BOOTSTRAP_ADMIN_EMAIL.equals(targetEmail)) { + throw new BaseException( + ErrorCode.INVALID_ARGUMENT, + "The bootstrap administrator cannot be permanently deleted. Deactivate it instead."); + } + + // Requiring deactivation first is not ceremony: it puts the irreversible step behind a state + // the administrator can observe. If this principal turns out to still be in use, deactivation + // surfaces that within minutes and is undone with one click. + if (user.getState() != User.StateEnum.DISABLED) { + throw new BaseException( + ErrorCode.FAILED_PRECONDITION, + "User must be deactivated before being permanently deleted: " + targetEmail); + } + + // Deleting an administrator is one of the ways a metastore can be left without one. + metastoreAdmins.requireAnotherAdminRemains(targetId); + + List owned = userRepository.listOwnedSecurables(targetEmail); + String reassignedTo = null; + if (!owned.isEmpty()) { + reassignedTo = + reassignTo.orElseThrow( + () -> + new BaseException( + ErrorCode.ABORTED, + "User owns " + + owned.size() + + " object(s); set reassign_to to the principal that should take" + + " ownership. See GET .../scim2/Users/" + + user.getId() + + "/ownedObjects for the list.")); + reassignOwnership(targetId, targetEmail, reassignedTo); + } + + authorizer.clearAuthorizationsForPrincipal(targetId); + userRepository.purgeUser(user.getId()); + // The only record that this happened: UC has no audit log, and a permanent delete that also + // moves ownership is not something to have to reconstruct from its effects. + LOGGER.warn( + "Permanently deleted user {} ({}) by {}; {} object(s) reassigned to {}", + targetEmail, + targetId, + IdentityUtils.findPrincipalEmailAddress(), + owned.size(), + reassignedTo == null ? "nobody" : reassignedTo); + } + + /** + * Hands both halves of ownership to the new owner: the {@code owner} columns, which is what gets + * displayed, and the OWNER policies, which is what actually authorizes. + * + *

The policies are granted before the old principal's are cleared, so no object is left + * without an owner even if the purge fails partway. + */ + private void reassignOwnership(UUID targetId, String targetEmail, String newOwnerEmail) { + if (newOwnerEmail.equals(targetEmail)) { + throw new BaseException( + ErrorCode.INVALID_ARGUMENT, "reassign_to cannot be the user being deleted."); + } + User newOwner; + try { + newOwner = userRepository.getUserByEmail(newOwnerEmail); + } catch (BaseException e) { + throw new BaseException( + ErrorCode.INVALID_ARGUMENT, "reassign_to names an unknown user: " + newOwnerEmail); + } + if (newOwner.getState() != User.StateEnum.ENABLED) { + throw new BaseException( + ErrorCode.INVALID_ARGUMENT, + "reassign_to names a deactivated user: " + + newOwnerEmail + + ". Ownership would be handed to an account that cannot act on it."); + } + + UUID newOwnerId = UUID.fromString(Objects.requireNonNull(newOwner.getId())); + userRepository.reassignOwnership(targetEmail, newOwnerEmail); + authorizer + .listAuthorizationsForPrincipal(targetId) + .forEach( + (resource, privileges) -> { + if (privileges.contains(Privileges.OWNER)) { + authorizer.grantAuthorization(newOwnerId, resource, Privileges.OWNER); + } + }); + } + + /** + * Changes a user's {@code active} state. + * + *

Both forms RFC 7644 §3.5.2.1 allows are accepted, because both turn up in practice: a + * pathless {@code replace} carries the attributes as an object ({@code {"active": false}}, which + * is what Okta and this project's own UI send), while a {@code replace} that names {@code active} + * in its path carries the bare value. An operation that asks for neither is answered with 501 + * rather than guessed at. + * + *

Authorization matches DELETE without {@code purge}, which flips the same flag. Until this + * was added the endpoint carried no authorization annotation at all, and UnityAccessDecorator + * lets a method without an expression through — so any signed-in user could deactivate an + * administrator. + */ @Patch("/{id}") + @AuthorizeExpression("#authorizeAny(#principal, #metastore, OWNER)") + @AuthorizeResourceKey(METASTORE) public HttpResponse patchUser(@Param("id") String id, PatchRequest patchRequest) { return patchRequest.getOperations().stream() - .filter( - op -> - op.getOpType() == PatchOpType.REPLACE - && op.getPath() == null) // Only support patch for okta + .filter(operation -> operation.getOpType() == PatchOpType.REPLACE) + .map(Scim2UserService::activeFrom) + .flatMap(Optional::stream) .findFirst() - .map(op -> handleUserUpdate(id, op)) + .map(active -> applyActive(id, active)) .orElse(HttpResponse.of(HttpStatus.NOT_IMPLEMENTED)); } - private HttpResponse handleUserUpdate(String id, PatchOperation operation) { - try { - Boolean value = operation.getValues(Boolean.class).get(0); - UpdateUser updateUser = UpdateUser.builder().active(value).build(); - userRepository.updateUser(id, updateUser); - return HttpResponse.of(HttpStatus.OK); - } catch (ScimException | JsonProcessingException e) { - return handleExceptionDuringPatch(e); + /** The new {@code active} value a replace asks for, if it asks for one at all. */ + private static Optional activeFrom(PatchOperation operation) { + JsonNode value = operation.getJsonNode(); + if (operation.getPath() == null) { + // Pathless: the value is the object of attributes to replace. + return booleanMember(value, ACTIVE_ATTRIBUTE); + } + // A named path carries the attribute's new value directly. SCIM attribute names are + // case-insensitive, so "active" and "Active" name the same attribute. + return ACTIVE_ATTRIBUTE.equalsIgnoreCase(operation.getPath().toString()) + && value != null + && value.isBoolean() + ? Optional.of(value.booleanValue()) + : Optional.empty(); + } + + /** Reads one boolean member by name, matching case-insensitively as SCIM requires. */ + private static Optional booleanMember(JsonNode object, String name) { + if (object == null || !object.isObject()) { + return Optional.empty(); } + Iterator members = object.fieldNames(); + while (members.hasNext()) { + String member = members.next(); + if (member.equalsIgnoreCase(name)) { + JsonNode value = object.get(member); + return value != null && value.isBoolean() + ? Optional.of(value.booleanValue()) + : Optional.empty(); + } + } + return Optional.empty(); + } + + private HttpResponse applyActive(String id, boolean active) { + if (!active) { + requireNotTheBootstrapAdministrator(userRepository.getUser(id)); + } + userRepository.updateUser(id, UpdateUser.builder().active(active).build()); + return HttpResponse.of(HttpStatus.OK); } - private HttpResponse handleExceptionDuringPatch(Exception ex) { - if (ex instanceof ScimException) { - throw new Scim2RuntimeException((ScimException) ex); - } else { - throw new Scim2RuntimeException( - new ServerErrorException("Problem with patch operation", ex.getMessage(), ex)); + /** + * Refuses to deactivate the bootstrap administrator, by either route. + * + *

It is the account behind the password sign-in — the way back in when the identity provider + * is unavailable. Deactivating it does not simply disable one user: it closes that door, and + * opening it again needs an administrator who can still sign in, which is exactly what may not + * exist when the provider is the reason anyone reached for the password. + * + *

Upstream lets a deployment deactivate its bootstrap account once real accounts exist (see + * the note in UnityAccessUtil#initializeAdmin). That predates the password sign-in this fork + * added, which gave the same account a second job, so the two cannot both hold here. + */ + private void requireNotTheBootstrapAdministrator(User user) { + if (BOOTSTRAP_ADMIN_EMAIL.equals(user.getEmail())) { + throw new BaseException( + ErrorCode.INVALID_ARGUMENT, + "The bootstrap administrator cannot be deactivated; it is the way back in when the" + + " identity provider is unavailable."); } } diff --git a/server/src/test/java/io/unitycatalog/server/base/BaseServerTest.java b/server/src/test/java/io/unitycatalog/server/base/BaseServerTest.java index 0c49204d..a961cae4 100644 --- a/server/src/test/java/io/unitycatalog/server/base/BaseServerTest.java +++ b/server/src/test/java/io/unitycatalog/server/base/BaseServerTest.java @@ -6,6 +6,7 @@ import io.unitycatalog.server.utils.ServerProperties; import io.unitycatalog.server.utils.ServerProperties.Property; import java.io.IOException; +import java.net.BindException; import java.net.ServerSocket; import java.nio.file.Files; import java.nio.file.Path; @@ -86,21 +87,83 @@ public void setUp() { ServerProperties initServerProperties = new ServerProperties(serverProperties); setUpCredentialOperations(initServerProperties); hibernateConfigurator = new HibernateConfigurator(initServerProperties); - unityCatalogServer = + startOnAFreePort(port, initServerProperties); + } + } + + /** + * Starts the server, retrying on a fresh port if the chosen one was taken in the meantime. + * + *

Picking a port and binding it are separate steps: {@link #findAvailablePort()} closes its + * socket before returning, and the server only binds after Hibernate and the credential vendor + * are built. On a loaded CI runner — several test JVMs at once — that window is wide enough for + * something else to take the port, and the server needs two of them, since it also listens on + * {@code port + 1}. The failure surfaced as {@code bind(..) failed: Address already in use} + * thrown from setUp, which fails every test in the class for a reason that has nothing to do with + * them. + */ + private void startOnAFreePort(int firstAttemptPort, ServerProperties properties) + throws IOException { + int port = firstAttemptPort; + for (int attempt = 1; ; attempt++) { + UnityCatalogServer server = UnityCatalogServer.builder() .port(port) - .serverProperties(initServerProperties) + .serverProperties(properties) .credentialOperations(cloudCredentialVendor) .build(); - unityCatalogServer.start(); - serverConfig.setServerUrl("http://localhost:" + port); + try { + server.start(); + unityCatalogServer = server; + serverConfig.setServerUrl("http://localhost:" + port); + return; + } catch (RuntimeException e) { + if (attempt == START_ATTEMPTS || !isAddressInUse(e)) { + throw e; + } + System.out.println("Port " + port + " was taken before the server bound it; retrying"); + port = findAvailablePort(); + } } } - /** Finds an available port for the UC server. */ + /** How many times to re-pick a port before giving up; the race is rare, so a few is plenty. */ + private static final int START_ATTEMPTS = 5; + + /** Whether the failure is the port race rather than something worth reporting. */ + private static boolean isAddressInUse(Throwable failure) { + for (Throwable cause = failure; cause != null; cause = cause.getCause()) { + if (cause instanceof BindException) { + return true; + } + String message = cause.getMessage(); + if (message != null && message.contains("Address already in use")) { + return true; + } + if (cause.getCause() == cause) { + break; + } + } + return false; + } + + /** + * Finds an available port for the UC server. Both this port and the next one must be free: the + * given port serves the URL transcoder and {@code port + 1} the API itself. + */ private int findAvailablePort() throws IOException { - try (ServerSocket socket = new ServerSocket(0)) { - return socket.getLocalPort(); + for (int attempt = 1; ; attempt++) { + int candidate; + try (ServerSocket socket = new ServerSocket(0)) { + candidate = socket.getLocalPort(); + } + try (ServerSocket ignored = new ServerSocket(candidate + 1)) { + return candidate; + } catch (IOException neighbourTaken) { + if (attempt == START_ATTEMPTS) { + return candidate; // Let the start attempt report it. + } + } } } diff --git a/server/src/test/java/io/unitycatalog/server/sdk/access/SdkAccessControlBaseCRUDTest.java b/server/src/test/java/io/unitycatalog/server/sdk/access/SdkAccessControlBaseCRUDTest.java index 326ad234..103dbec6 100644 --- a/server/src/test/java/io/unitycatalog/server/sdk/access/SdkAccessControlBaseCRUDTest.java +++ b/server/src/test/java/io/unitycatalog/server/sdk/access/SdkAccessControlBaseCRUDTest.java @@ -163,7 +163,10 @@ public void tearDown() { for (String userId : createdUserIds) { try { LOGGER.debug("Deleting test user: {}", userId); - usersApi.deleteUser(userId); + // Deactivation, as before purge existed: the extra arguments are purge / reassign_to / + // confirm_principal, and a purge would additionally require the user to be deactivated + // first, which this cleanup has no reason to do. + usersApi.deleteUser(userId, null, null, null); } catch (Exception e) { LOGGER.warn("Failed to delete test user {}: {}", userId, e.getMessage()); } diff --git a/server/src/test/java/io/unitycatalog/server/service/AuthServiceTokenLoginTest.java b/server/src/test/java/io/unitycatalog/server/service/AuthServiceTokenLoginTest.java index b0a8892c..de97511f 100644 --- a/server/src/test/java/io/unitycatalog/server/service/AuthServiceTokenLoginTest.java +++ b/server/src/test/java/io/unitycatalog/server/service/AuthServiceTokenLoginTest.java @@ -122,6 +122,66 @@ public void anEmptyTokenIsRefused() { assertThat(response.status()).isEqualTo(HttpStatus.BAD_REQUEST); } + /** + * The session belongs to whoever the token names, not to whoever pasted it. + * + *

Worth stating outright because this address is reached by pasting a credential: if the + * identity ever collapsed to the administrator, anyone holding any valid token would arrive as + * one. The reason the operator's own sign-in shows "admin" is simply that etc/conf/token.txt + * names admin — not that this endpoint confers it. + */ + @Test + public void theSessionIsWhoeverTheTokenNames() throws IOException { + createUser("someone@example.com"); + String theirToken = securityContext.createAccessToken("someone@example.com", null); + + AggregatedHttpResponse response = signIn(theirToken); + + assertThat(response.status()).isEqualTo(HttpStatus.OK); + AggregatedHttpResponse me = + client + .execute( + RequestHeaders.builder(HttpMethod.GET, "/api/1.0/unity-control/scim2/Me") + .add(HttpHeaderNames.COOKIE, AuthDecorator.UC_TOKEN_KEY + "=" + theirToken) + .build()) + .aggregate() + .join(); + assertThat(me.status()).isEqualTo(HttpStatus.OK); + assertThat(MAPPER.readTree(me.contentUtf8()).get("userName").asText()) + .isEqualTo("someone@example.com"); + } + + /** A token naming somebody the metastore does not know opens nothing. */ + @Test + public void tokenNamingNobodyIsRejected() { + String orphan = securityContext.createAccessToken("ghost@example.com", null); + + AggregatedHttpResponse response = signIn(orphan); + + assertThat(response.status()).isEqualTo(HttpStatus.UNAUTHORIZED); + } + + private void createUser(String email) throws IOException { + AggregatedHttpResponse response = + client + .execute( + RequestHeaders.builder() + .method(HttpMethod.POST) + .path("/api/1.0/unity-control/scim2/Users") + .contentType(MediaType.JSON) + .add(HttpHeaderNames.AUTHORIZATION, "Bearer " + anAdminToken()) + .build(), + HttpData.ofUtf8( + "{\"displayName\":\"" + + email + + "\",\"emails\":[{\"primary\":true,\"value\":\"" + + email + + "\"}]}")) + .aggregate() + .join(); + assertThat(response.status()).isEqualTo(HttpStatus.CREATED); + } + /** The administrator password sign-in is only a convenient source of a real token here. */ private String anAdminToken() throws IOException { String form = "username=admin&password=" + URLEncoder.encode(PASSWORD, StandardCharsets.UTF_8); diff --git a/server/src/test/java/io/unitycatalog/server/service/BootstrapAdminProtectionTest.java b/server/src/test/java/io/unitycatalog/server/service/BootstrapAdminProtectionTest.java new file mode 100644 index 00000000..3018f8f4 --- /dev/null +++ b/server/src/test/java/io/unitycatalog/server/service/BootstrapAdminProtectionTest.java @@ -0,0 +1,236 @@ +package io.unitycatalog.server.service; + +import static org.assertj.core.api.Assertions.assertThat; + +import com.fasterxml.jackson.databind.JsonNode; +import com.fasterxml.jackson.databind.ObjectMapper; +import com.linecorp.armeria.client.WebClient; +import com.linecorp.armeria.common.AggregatedHttpResponse; +import com.linecorp.armeria.common.HttpData; +import com.linecorp.armeria.common.HttpHeaderNames; +import com.linecorp.armeria.common.HttpMethod; +import com.linecorp.armeria.common.HttpStatus; +import com.linecorp.armeria.common.MediaType; +import com.linecorp.armeria.common.RequestHeaders; +import io.unitycatalog.server.base.auth.BaseAuthCRUDTest; +import io.unitycatalog.server.persist.dao.UserDAO; +import java.io.IOException; +import java.net.URLEncoder; +import java.nio.charset.StandardCharsets; +import org.hibernate.Session; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; + +/** + * What may not be done to the bootstrap administrator. + * + *

The {@code admin} account is the one the password sign-in accepts, which is the way back in + * when the identity provider is unavailable. Upstream treats it as a throwaway bootstrap account + * and says so in UnityAccessUtil#initializeAdmin — that note predates the password sign-in this + * fork added, which gave the same account a second job. + * + *

Purging it was already refused. Deactivating it was not, by any of the three routes that can, + * and neither was taking its administrator privilege away — which no restart repairs. A second + * administrator could therefore close the door behind everyone, which is what prompted these tests. + */ +public class BootstrapAdminProtectionTest extends BaseAuthCRUDTest { + + private static final String CONTROL_PATH = "/api/1.0/unity-control"; + private static final String PASSWORD = "correct horse battery staple"; + private static final String ADMIN = "admin"; + private static final ObjectMapper MAPPER = new ObjectMapper(); + + private WebClient client; + private String adminToken; + private String adminUserId; + + @Override + protected void setUpProperties() { + super.setUpProperties(); + serverProperties.setProperty("server.admin-password", PASSWORD); + } + + @BeforeEach + @Override + public void setUp() { + super.setUp(); + client = WebClient.builder(serverConfig.getServerUrl()).build(); + adminToken = signIn(); + adminUserId = idOf(ADMIN); + } + + /** The route the UI takes. */ + @Test + public void patchCannotDeactivateTheBootstrapAdministrator() throws IOException { + AggregatedHttpResponse response = + send( + HttpMethod.PATCH, + CONTROL_PATH + "/scim2/Users/" + adminUserId, + "{\"schemas\":[\"urn:ietf:params:scim:api:messages:2.0:PatchOp\"]," + + "\"Operations\":[{\"op\":\"replace\",\"value\":{\"active\":false}}]}"); + + assertThat(response.status().isSuccess()).isFalse(); + assertThat(isActive(adminUserId)).isTrue(); + } + + /** DELETE without purge means deactivate, so it needs the same guard. */ + @Test + public void deleteCannotDeactivateTheBootstrapAdministrator() throws IOException { + AggregatedHttpResponse response = + send(HttpMethod.DELETE, CONTROL_PATH + "/scim2/Users/" + adminUserId, ""); + + assertThat(response.status().isSuccess()).isFalse(); + assertThat(isActive(adminUserId)).isTrue(); + } + + /** A PUT writes `active` back like any other field; it is the third route in. */ + @Test + public void putCannotDeactivateTheBootstrapAdministrator() throws IOException { + AggregatedHttpResponse response = + send( + HttpMethod.PUT, + CONTROL_PATH + "/scim2/Users/" + adminUserId, + "{\"schemas\":[\"urn:ietf:params:scim:schemas:core:2.0:User\"]," + + "\"id\":\"" + + adminUserId + + "\",\"displayName\":\"Admin\",\"active\":false," + + "\"emails\":[{\"primary\":true,\"value\":\"" + + ADMIN + + "\"}]}"); + + assertThat(response.status().isSuccess()).isFalse(); + assertThat(isActive(adminUserId)).isTrue(); + } + + /** Purging was already refused; this keeps it that way. */ + @Test + public void purgeCannotRemoveTheBootstrapAdministrator() throws IOException { + AggregatedHttpResponse response = + send( + HttpMethod.DELETE, + CONTROL_PATH + "/scim2/Users/" + adminUserId + "?purge=true&confirm_principal=" + ADMIN, + ""); + + assertThat(response.status().isSuccess()).isFalse(); + assertThat(isActive(adminUserId)).isTrue(); + } + + /** + * Taking the privilege away leaves an account that signs in and administers nothing, and + * UnityAccessUtil#initializeAdmin only re-grants OWNER when the account is missing entirely — so + * a surviving row that has lost it never gets it back. + */ + @Test + public void theBootstrapAdministratorCannotLoseAdministratorStatus() { + AggregatedHttpResponse response = + send(HttpMethod.DELETE, CONTROL_PATH + "/metastore/admins/" + ADMIN, ""); + + assertThat(response.status().isSuccess()).isFalse(); + assertThat(admins()).contains(ADMIN); + } + + /** + * Defence in depth for the guards above: if the account is deactivated anyway — by an older + * build, or by something reaching the database directly — the password sign-in must say so. + * + *

It used to hand out a session regardless, because it checks the password against + * configuration and never consults the user table. Every request made with that session was then + * refused by AuthDecorator, so the sign-in looked like it worked and nothing worked afterwards. + */ + @Test + public void signingInIsRefusedWhenTheAdminAccountIsDeactivated() { + deactivateAdminBehindTheApi(); + + AggregatedHttpResponse response = attemptSignIn(); + + assertThat(response.status().isSuccess()).isFalse(); + assertThat(response.contentUtf8()).contains("deactivated"); + } + + // ---------------------------------------------------------------- helpers + + /** Reaches past the service guards, the way an older build or a DBA could. */ + private void deactivateAdminBehindTheApi() { + try (Session session = hibernateConfigurator.getSessionFactory().openSession()) { + session.beginTransaction(); + UserDAO dao = + session + .createQuery("FROM UserDAO WHERE email = :email", UserDAO.class) + .setParameter("email", ADMIN) + .uniqueResult(); + dao.setState("DISABLED"); + session.merge(dao); + session.getTransaction().commit(); + } + } + + private AggregatedHttpResponse attemptSignIn() { + String form = "username=admin&password=" + URLEncoder.encode(PASSWORD, StandardCharsets.UTF_8); + return client + .execute( + RequestHeaders.builder() + .method(HttpMethod.POST) + .path(CONTROL_PATH + "/auth/admin/login") + .contentType(MediaType.FORM_DATA) + .build(), + HttpData.ofUtf8(form)) + .aggregate() + .join(); + } + + private String signIn() { + AggregatedHttpResponse response = attemptSignIn(); + assertThat(response.status()).isEqualTo(HttpStatus.OK); + try { + return MAPPER.readTree(response.contentUtf8()).get("access_token").asText(); + } catch (IOException e) { + throw new RuntimeException("admin sign-in did not return an access token", e); + } + } + + private String idOf(String principal) { + AggregatedHttpResponse response = send(HttpMethod.GET, CONTROL_PATH + "/scim2/Users", ""); + assertThat(response.status()).isEqualTo(HttpStatus.OK); + try { + for (JsonNode user : MAPPER.readTree(response.contentUtf8()).get("Resources")) { + if (principal.equals(user.path("userName").asText())) { + return user.get("id").asText(); + } + } + } catch (IOException e) { + throw new RuntimeException("could not list users", e); + } + throw new IllegalStateException("no user named " + principal); + } + + private boolean isActive(String userId) throws IOException { + AggregatedHttpResponse response = + send(HttpMethod.GET, CONTROL_PATH + "/scim2/Users/" + userId, ""); + assertThat(response.status()).isEqualTo(HttpStatus.OK); + JsonNode active = MAPPER.readTree(response.contentUtf8()).get("active"); + return active == null || active.asBoolean(); + } + + private java.util.List admins() { + AggregatedHttpResponse response = send(HttpMethod.GET, CONTROL_PATH + "/metastore/admins", ""); + assertThat(response.status()).isEqualTo(HttpStatus.OK); + try { + java.util.List names = new java.util.ArrayList<>(); + MAPPER.readTree(response.contentUtf8()).get("admins").forEach(n -> names.add(n.asText())); + return names; + } catch (IOException e) { + throw new RuntimeException("could not list administrators", e); + } + } + + private AggregatedHttpResponse send(HttpMethod method, String path, String body) { + RequestHeaders headers = + RequestHeaders.builder() + .method(method) + .path(path) + .contentType(MediaType.JSON) + .add(HttpHeaderNames.AUTHORIZATION, "Bearer " + adminToken) + .build(); + return client.execute(headers, HttpData.ofUtf8(body)).aggregate().join(); + } +} diff --git a/server/src/test/java/io/unitycatalog/server/service/MetastoreAdminsTest.java b/server/src/test/java/io/unitycatalog/server/service/MetastoreAdminsTest.java new file mode 100644 index 00000000..d59444c7 --- /dev/null +++ b/server/src/test/java/io/unitycatalog/server/service/MetastoreAdminsTest.java @@ -0,0 +1,250 @@ +package io.unitycatalog.server.service; + +import static org.assertj.core.api.Assertions.assertThat; + +import com.fasterxml.jackson.databind.JsonNode; +import com.fasterxml.jackson.databind.ObjectMapper; +import com.linecorp.armeria.client.WebClient; +import com.linecorp.armeria.common.AggregatedHttpResponse; +import com.linecorp.armeria.common.HttpData; +import com.linecorp.armeria.common.HttpHeaderNames; +import com.linecorp.armeria.common.HttpMethod; +import com.linecorp.armeria.common.HttpStatus; +import com.linecorp.armeria.common.MediaType; +import com.linecorp.armeria.common.RequestHeaders; +import io.unitycatalog.server.base.auth.BaseAuthCRUDTest; +import io.unitycatalog.server.persist.dao.UserDAO; +import java.io.IOException; +import java.net.URLEncoder; +import java.nio.charset.StandardCharsets; +import java.util.ArrayList; +import java.util.List; +import org.hibernate.Session; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; + +/** + * Who administers the metastore, and the one rule that governs changing it. + * + *

Being an administrator is holding OWNER on the metastore, and the invariant is that at least + * one enabled account always holds it. Without that a metastore can be left with nobody + * able to administer it and no supported way back: UnityAccessUtil#initializeAdmin re-grants OWNER + * only when the {@code admin} account is missing entirely, so a surviving row that lost the + * privilege is never repaired by a restart. + */ +public class MetastoreAdminsTest extends BaseAuthCRUDTest { + + private static final String CONTROL_PATH = "/api/1.0/unity-control"; + private static final String ADMINS_PATH = CONTROL_PATH + "/metastore/admins"; + private static final String PASSWORD = "correct horse battery staple"; + private static final String ALICE = "alice@example.com"; + private static final String BOB = "bob@example.com"; + private static final ObjectMapper MAPPER = new ObjectMapper(); + + private WebClient client; + private String adminToken; + + @Override + protected void setUpProperties() { + super.setUpProperties(); + serverProperties.setProperty("server.admin-password", PASSWORD); + } + + @BeforeEach + @Override + public void setUp() { + super.setUp(); + client = WebClient.builder(serverConfig.getServerUrl()).build(); + adminToken = signIn(); + } + + @Test + public void administratorsCanBeAppointedAndStoodDown() throws IOException { + createUser(ALICE); + assertThat(admins()).containsExactly("admin"); + + assertThat(grant(ALICE, adminToken).status()).isEqualTo(HttpStatus.OK); + assertThat(admins()).containsExactlyInAnyOrder("admin", ALICE); + + assertThat(revoke(ALICE, adminToken).status()).isEqualTo(HttpStatus.OK); + assertThat(admins()).containsExactly("admin"); + } + + /** + * The server refuses to grant it to a deactivated account: the privilege would be held by + * somebody who cannot sign in, which counts towards nothing and only looks like cover. + */ + @Test + public void appointingADeactivatedAccountIsRefused() throws IOException { + String bobId = createUser(BOB); + deactivate(bobId); + + AggregatedHttpResponse response = grant(BOB, adminToken); + + assertThat(response.status().isSuccess()).isFalse(); + assertThat(admins()).doesNotContain(BOB); + } + + /** + * The invariant itself. The bootstrap administrator is deactivated here behind the API — which + * the service now refuses, and which is exactly why this has to be set up directly — to reach the + * state where one ordinary administrator is all that is left. + */ + @Test + public void theLastEnabledAdministratorCannotStandDown() throws IOException { + createUser(ALICE); + assertThat(grant(ALICE, adminToken).status()).isEqualTo(HttpStatus.OK); + String aliceToken = securityContext.createAccessToken(ALICE, null); + + deactivateBehindTheApi("admin"); + + // admin still holds OWNER, but a deactivated holder does not keep the metastore administrable, + // so Alice is the last one that counts. + AggregatedHttpResponse response = revoke(ALICE, aliceToken); + + assertThat(response.status().isSuccess()).isFalse(); + assertThat(response.contentUtf8()).contains("administrator"); + // Asked as Alice: the admin account is deactivated, so its own token no longer opens anything. + assertThat(admins(aliceToken)).contains(ALICE); + } + + /** + * Handing over is a real thing to want, so revoking your own is allowed while another remains. + */ + @Test + public void anAdministratorMayStandThemselvesDown() throws IOException { + createUser(ALICE); + assertThat(grant(ALICE, adminToken).status()).isEqualTo(HttpStatus.OK); + String aliceToken = securityContext.createAccessToken(ALICE, null); + + assertThat(revoke(ALICE, aliceToken).status()).isEqualTo(HttpStatus.OK); + assertThat(admins()).containsExactly("admin"); + } + + /** Appointing administrators is itself an administrator's privilege. */ + @Test + public void anOrdinaryUserCannotAppointAdministrators() throws IOException { + createUser(ALICE); + createUser(BOB); + String bobToken = securityContext.createAccessToken(BOB, null); + + assertThat(grant(ALICE, bobToken).status().isSuccess()).isFalse(); + assertThat(admins()).doesNotContain(ALICE); + } + + /** And so is seeing the list: OWNER is filtered out of /permissions, so this is the only view. */ + @Test + public void anOrdinaryUserCannotSeeTheAdministrators() throws IOException { + createUser(BOB); + String bobToken = securityContext.createAccessToken(BOB, null); + + AggregatedHttpResponse response = send(HttpMethod.GET, ADMINS_PATH, "", bobToken); + + assertThat(response.status().isSuccess()).isFalse(); + } + + // ---------------------------------------------------------------- helpers + + private AggregatedHttpResponse grant(String email, String token) { + return send(HttpMethod.PUT, ADMINS_PATH + "/" + email, "", token); + } + + private AggregatedHttpResponse revoke(String email, String token) { + return send(HttpMethod.DELETE, ADMINS_PATH + "/" + email, "", token); + } + + private List admins() throws IOException { + return admins(adminToken); + } + + /** + * Who is listed, as seen by a given administrator — the caller matters once admin is disabled. + */ + private List admins(String token) throws IOException { + AggregatedHttpResponse response = send(HttpMethod.GET, ADMINS_PATH, "", token); + assertThat(response.status()).isEqualTo(HttpStatus.OK); + List names = new ArrayList<>(); + for (JsonNode name : MAPPER.readTree(response.contentUtf8()).get("admins")) { + names.add(name.asText()); + } + return names; + } + + private void deactivate(String id) { + AggregatedHttpResponse response = + send( + HttpMethod.PATCH, + CONTROL_PATH + "/scim2/Users/" + id, + "{\"schemas\":[\"urn:ietf:params:scim:api:messages:2.0:PatchOp\"]," + + "\"Operations\":[{\"op\":\"replace\",\"value\":{\"active\":false}}]}", + adminToken); + assertThat(response.status()).isEqualTo(HttpStatus.OK); + } + + /** The service refuses to deactivate the bootstrap administrator, so this goes around it. */ + private void deactivateBehindTheApi(String email) { + try (Session session = hibernateConfigurator.getSessionFactory().openSession()) { + session.beginTransaction(); + UserDAO dao = + session + .createQuery("FROM UserDAO WHERE email = :email", UserDAO.class) + .setParameter("email", email) + .uniqueResult(); + dao.setState("DISABLED"); + session.merge(dao); + session.getTransaction().commit(); + } + } + + private String signIn() { + String form = "username=admin&password=" + URLEncoder.encode(PASSWORD, StandardCharsets.UTF_8); + AggregatedHttpResponse response = + client + .execute( + RequestHeaders.builder() + .method(HttpMethod.POST) + .path(CONTROL_PATH + "/auth/admin/login") + .contentType(MediaType.FORM_DATA) + .build(), + HttpData.ofUtf8(form)) + .aggregate() + .join(); + assertThat(response.status()).isEqualTo(HttpStatus.OK); + try { + return MAPPER.readTree(response.contentUtf8()).get("access_token").asText(); + } catch (IOException e) { + throw new IllegalStateException("admin sign-in did not return an access token", e); + } + } + + private String createUser(String email) { + AggregatedHttpResponse response = + send( + HttpMethod.POST, + CONTROL_PATH + "/scim2/Users", + "{\"displayName\":\"" + + email + + "\",\"emails\":[{\"primary\":true,\"value\":\"" + + email + + "\"}]}", + adminToken); + assertThat(response.status()).isEqualTo(HttpStatus.CREATED); + try { + return MAPPER.readTree(response.contentUtf8()).get("id").asText(); + } catch (IOException e) { + throw new IllegalStateException("user creation did not return an id", e); + } + } + + private AggregatedHttpResponse send( + HttpMethod method, String path, String body, String bearerToken) { + RequestHeaders headers = + RequestHeaders.builder() + .method(method) + .path(path) + .contentType(MediaType.JSON) + .add(HttpHeaderNames.AUTHORIZATION, "Bearer " + bearerToken) + .build(); + return client.execute(headers, HttpData.ofUtf8(body)).aggregate().join(); + } +} diff --git a/server/src/test/java/io/unitycatalog/server/service/Scim2UserPatchTest.java b/server/src/test/java/io/unitycatalog/server/service/Scim2UserPatchTest.java new file mode 100644 index 00000000..91bd1695 --- /dev/null +++ b/server/src/test/java/io/unitycatalog/server/service/Scim2UserPatchTest.java @@ -0,0 +1,243 @@ +package io.unitycatalog.server.service; + +import static org.assertj.core.api.Assertions.assertThat; + +import com.fasterxml.jackson.databind.JsonNode; +import com.fasterxml.jackson.databind.ObjectMapper; +import com.linecorp.armeria.client.WebClient; +import com.linecorp.armeria.common.AggregatedHttpResponse; +import com.linecorp.armeria.common.HttpData; +import com.linecorp.armeria.common.HttpHeaderNames; +import com.linecorp.armeria.common.HttpMethod; +import com.linecorp.armeria.common.HttpStatus; +import com.linecorp.armeria.common.MediaType; +import com.linecorp.armeria.common.RequestHeaders; +import io.unitycatalog.server.base.auth.BaseAuthCRUDTest; +import java.io.IOException; +import java.net.URLEncoder; +import java.nio.charset.StandardCharsets; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; + +/** + * Changing a user's {@code active} state over SCIM PATCH. + * + *

RFC 7644 §3.5.2.1 allows a {@code replace} to be written two ways, and both reach this server + * in practice: without a path, carrying the attributes as an object, and with {@code active} named + * in the path, carrying the bare value. The UI and Okta send the first; other identity providers + * send the second. Neither had a test, and the UI shipped a third shape — pathless with a scalar — + * which the SCIM library refuses while parsing, so the user saw a JSON parse failure instead of a + * deactivated account. + */ +public class Scim2UserPatchTest extends BaseAuthCRUDTest { + + private static final String CONTROL_PATH = "/api/1.0/unity-control"; + private static final String PASSWORD = "correct horse battery staple"; + private static final String TARGET_EMAIL = "patch-target@example.com"; + private static final String OUTSIDER_EMAIL = "outsider@example.com"; + private static final ObjectMapper MAPPER = new ObjectMapper(); + + private static final String PATHLESS_DEACTIVATE = + "{\"schemas\":[\"urn:ietf:params:scim:api:messages:2.0:PatchOp\"]," + + "\"Operations\":[{\"op\":\"replace\",\"value\":{\"active\":false}}]}"; + private static final String PATHLESS_REACTIVATE = + "{\"schemas\":[\"urn:ietf:params:scim:api:messages:2.0:PatchOp\"]," + + "\"Operations\":[{\"op\":\"replace\",\"value\":{\"active\":true}}]}"; + + private WebClient client; + private String adminToken; + private String userId; + + @Override + protected void setUpProperties() { + super.setUpProperties(); + serverProperties.setProperty("server.admin-password", PASSWORD); + } + + @BeforeEach + @Override + public void setUp() { + super.setUp(); + client = WebClient.builder(serverConfig.getServerUrl()).build(); + adminToken = signInAsAdmin(); + userId = createUser(TARGET_EMAIL); + } + + /** The form the UI and Okta send. */ + @Test + public void pathlessReplaceWithObjectValueDeactivatesTheUser() throws IOException { + assertThat(patch(PATHLESS_DEACTIVATE).status()).isEqualTo(HttpStatus.OK); + assertThat(isActive()).isFalse(); + } + + /** And it is reversible, which is the whole point of deactivating rather than purging. */ + @Test + public void theSameFormReactivates() throws IOException { + patch(PATHLESS_DEACTIVATE); + assertThat(isActive()).isFalse(); + + assertThat(patch(PATHLESS_REACTIVATE).status()).isEqualTo(HttpStatus.OK); + assertThat(isActive()).isTrue(); + } + + /** The other form RFC 7644 allows: the attribute named in the path, the value bare. */ + @Test + public void replaceWithActiveInThePathDeactivatesTheUser() throws IOException { + AggregatedHttpResponse response = + patch( + "{\"schemas\":[\"urn:ietf:params:scim:api:messages:2.0:PatchOp\"]," + + "\"Operations\":[{\"op\":\"replace\",\"path\":\"active\",\"value\":false}]}"); + + assertThat(response.status()).isEqualTo(HttpStatus.OK); + assertThat(isActive()).isFalse(); + } + + /** SCIM attribute names are case-insensitive, in the path and in a pathless object alike. */ + @Test + public void theAttributeNameIsMatchedWithoutRegardToCase() throws IOException { + assertThat( + patch( + "{\"schemas\":[\"urn:ietf:params:scim:api:messages:2.0:PatchOp\"]," + + "\"Operations\":[{\"op\":\"replace\",\"path\":\"Active\"," + + "\"value\":false}]}") + .status()) + .isEqualTo(HttpStatus.OK); + assertThat(isActive()).isFalse(); + + assertThat( + patch( + "{\"schemas\":[\"urn:ietf:params:scim:api:messages:2.0:PatchOp\"]," + + "\"Operations\":[{\"op\":\"replace\"," + + "\"value\":{\"ACTIVE\":true}}]}") + .status()) + .isEqualTo(HttpStatus.OK); + assertThat(isActive()).isTrue(); + } + + /** + * The regression that reached a user: a pathless replace carrying a scalar. The status is only + * asserted as "not success" on purpose — the refusal currently surfaces as a 500 from the + * library's deserialiser, and turning that into a 400 would be an improvement this test should + * not stand in the way of. What must not change is that the account is left alone. + */ + @Test + public void pathlessReplaceWithScalarValueIsRefusedAndChangesNothing() throws IOException { + AggregatedHttpResponse response = + patch( + "{\"schemas\":[\"urn:ietf:params:scim:api:messages:2.0:PatchOp\"]," + + "\"Operations\":[{\"op\":\"replace\",\"value\":false}]}"); + + assertThat(response.status().isSuccess()).isFalse(); + assertThat(isActive()).isTrue(); + } + + /** An attribute this endpoint does not manage is declined rather than silently ignored. */ + @Test + public void replaceOfSomeOtherAttributeIsNotImplemented() throws IOException { + AggregatedHttpResponse response = + patch( + "{\"schemas\":[\"urn:ietf:params:scim:api:messages:2.0:PatchOp\"]," + + "\"Operations\":[{\"op\":\"replace\",\"path\":\"displayName\"," + + "\"value\":\"Renamed\"}]}"); + + assertThat(response.status()).isEqualTo(HttpStatus.NOT_IMPLEMENTED); + assertThat(isActive()).isTrue(); + } + + /** + * The endpoint carried no authorization annotation until this was fixed, and UnityAccessDecorator + * lets a method without one through — so any signed-in account could deactivate any other, an + * administrator included. + */ + @Test + public void anOrdinaryUserCannotDeactivateAnyone() throws IOException { + createUser(OUTSIDER_EMAIL); + String outsiderToken = securityContext.createAccessToken(OUTSIDER_EMAIL, null); + + AggregatedHttpResponse response = + send( + HttpMethod.PATCH, + CONTROL_PATH + "/scim2/Users/" + userId, + PATHLESS_DEACTIVATE, + outsiderToken); + + assertThat(response.status().isSuccess()).isFalse(); + assertThat(isActive()).isTrue(); + } + + // ---------------------------------------------------------------- helpers + + private String signInAsAdmin() { + String form = "username=admin&password=" + URLEncoder.encode(PASSWORD, StandardCharsets.UTF_8); + AggregatedHttpResponse response = + client + .execute( + RequestHeaders.builder() + .method(HttpMethod.POST) + .path(CONTROL_PATH + "/auth/admin/login") + .contentType(MediaType.FORM_DATA) + .build(), + HttpData.ofUtf8(form)) + .aggregate() + .join(); + assertThat(response.status()).isEqualTo(HttpStatus.OK); + try { + return MAPPER.readTree(response.contentUtf8()).get("access_token").asText(); + } catch (IOException e) { + throw new RuntimeException("admin sign-in did not return an access token", e); + } + } + + private String createUser(String email) { + AggregatedHttpResponse response = + send( + HttpMethod.POST, + CONTROL_PATH + "/scim2/Users", + "{\"displayName\":\"" + + email + + "\",\"emails\":[{\"primary\":true,\"value\":\"" + + email + + "\"}]}", + adminToken); + assertThat(response.status()).isEqualTo(HttpStatus.CREATED); + try { + return MAPPER.readTree(response.contentUtf8()).get("id").asText(); + } catch (IOException e) { + throw new RuntimeException("user creation did not return an id", e); + } + } + + private AggregatedHttpResponse patch(String body) { + return send(HttpMethod.PATCH, CONTROL_PATH + "/scim2/Users/" + userId, body, adminToken); + } + + private boolean isActive() throws IOException { + AggregatedHttpResponse response = + client + .execute( + RequestHeaders.builder(HttpMethod.GET, CONTROL_PATH + "/scim2/Users/" + userId) + .add(HttpHeaderNames.AUTHORIZATION, "Bearer " + adminToken) + .build()) + .aggregate() + .join(); + assertThat(response.status()).isEqualTo(HttpStatus.OK); + JsonNode active = MAPPER.readTree(response.contentUtf8()).get("active"); + // SCIM may omit `active` when it is true; absent means not deactivated. + return active == null || active.asBoolean(); + } + + private AggregatedHttpResponse send( + HttpMethod method, String path, String body, String bearerToken) { + return client + .execute( + RequestHeaders.builder() + .method(method) + .path(path) + .contentType(MediaType.JSON) + .add(HttpHeaderNames.AUTHORIZATION, "Bearer " + bearerToken) + .build(), + HttpData.ofUtf8(body)) + .aggregate() + .join(); + } +} diff --git a/server/src/test/java/io/unitycatalog/server/service/Scim2UserPurgeTest.java b/server/src/test/java/io/unitycatalog/server/service/Scim2UserPurgeTest.java new file mode 100644 index 00000000..70828b23 --- /dev/null +++ b/server/src/test/java/io/unitycatalog/server/service/Scim2UserPurgeTest.java @@ -0,0 +1,283 @@ +package io.unitycatalog.server.service; + +import static org.assertj.core.api.Assertions.assertThat; + +import com.fasterxml.jackson.databind.JsonNode; +import com.fasterxml.jackson.databind.ObjectMapper; +import com.linecorp.armeria.client.WebClient; +import com.linecorp.armeria.common.AggregatedHttpResponse; +import com.linecorp.armeria.common.HttpData; +import com.linecorp.armeria.common.HttpHeaderNames; +import com.linecorp.armeria.common.HttpMethod; +import com.linecorp.armeria.common.HttpStatus; +import com.linecorp.armeria.common.MediaType; +import com.linecorp.armeria.common.RequestHeaders; +import io.unitycatalog.server.base.auth.BaseAuthCRUDTest; +import io.unitycatalog.server.persist.dao.CatalogInfoDAO; +import java.io.IOException; +import java.net.URLEncoder; +import java.nio.charset.StandardCharsets; +import org.hibernate.Session; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; + +/** + * Permanently deleting a user. + * + *

Purging is the one irreversible thing this service does, and the only thing that frees an + * email for reuse — deactivating leaves the row, so the address stays taken. Everything that makes + * it safe is checked in the service rather than trusted to the caller, and none of it had a test. + */ +public class Scim2UserPurgeTest extends BaseAuthCRUDTest { + + private static final String CONTROL_PATH = "/api/1.0/unity-control"; + private static final String CATALOG_PATH = "/api/2.1/unity-catalog"; + private static final String PASSWORD = "correct horse battery staple"; + private static final String TARGET = "target@example.com"; + private static final ObjectMapper MAPPER = new ObjectMapper(); + + private WebClient client; + private String adminToken; + private String targetId; + + @Override + protected void setUpProperties() { + super.setUpProperties(); + serverProperties.setProperty("server.admin-password", PASSWORD); + } + + @BeforeEach + @Override + public void setUp() { + super.setUp(); + client = WebClient.builder(serverConfig.getServerUrl()).build(); + adminToken = signIn(); + targetId = createUser(TARGET); + } + + /** + * Typing the principal back is the only check that catches "right button, wrong row" — and the + * user list mixes people with Relyt instance principals, where the wrong row takes a whole + * compute instance off Unity Catalog. + */ + @Test + public void purgingWithoutConfirmingThePrincipalIsRefused() throws IOException { + deactivate(targetId); + + AggregatedHttpResponse response = + send(HttpMethod.DELETE, CONTROL_PATH + "/scim2/Users/" + targetId + "?purge=true", ""); + + assertThat(response.status().isSuccess()).isFalse(); + assertThat(response.contentUtf8()).contains("confirm_principal"); + assertThat(exists(TARGET)).isTrue(); + } + + @Test + public void purgingWithTheWrongPrincipalIsRefused() throws IOException { + deactivate(targetId); + + AggregatedHttpResponse response = purge(targetId, "someone.else@example.com", null); + + assertThat(response.status().isSuccess()).isFalse(); + assertThat(exists(TARGET)).isTrue(); + } + + /** + * Deactivation first is not ceremony: it puts the irreversible step behind a state an + * administrator can observe, so a principal that turns out to still be in use says so while the + * change is still one click from being undone. + */ + @Test + public void purgingAnActiveUserIsRefused() throws IOException { + AggregatedHttpResponse response = purge(targetId, TARGET, null); + + assertThat(response.status().isSuccess()).isFalse(); + assertThat(response.contentUtf8()).contains("deactivated"); + assertThat(exists(TARGET)).isTrue(); + } + + @Test + public void purgingYourOwnAccountIsRefused() throws IOException { + // The administrator signs in as "admin", so that is the account it may not purge. + String adminId = idOf("admin"); + + AggregatedHttpResponse response = purge(adminId, "admin", null); + + assertThat(response.status().isSuccess()).isFalse(); + assertThat(exists("admin")).isTrue(); + } + + /** What purging is for: the address becomes available again, which deactivating never does. */ + @Test + public void purgingFreesTheEmailForReuse() throws IOException { + deactivate(targetId); + + assertThat(purge(targetId, TARGET, null).status()).isEqualTo(HttpStatus.OK); + + assertThat(exists(TARGET)).isFalse(); + // Creating the same address again is the proof: before the purge this answers 409. + assertThat(createUser(TARGET)).isNotEqualTo(targetId); + } + + /** + * Objects outlive the account that made them. Purging without saying who takes them would leave + * securables owned by a principal that no longer exists — nobody could then administer them, and + * nothing in the UI would explain why. + */ + @Test + public void purgingSomebodyWhoOwnsThingsNeedsAnHeir() throws IOException { + createCatalog("inherited_catalog"); + giveOwnership("inherited_catalog", TARGET); + deactivate(targetId); + + AggregatedHttpResponse refused = purge(targetId, TARGET, null); + assertThat(refused.status().isSuccess()).isFalse(); + assertThat(refused.contentUtf8()).contains("reassign_to"); + assertThat(exists(TARGET)).isTrue(); + + AggregatedHttpResponse accepted = purge(targetId, TARGET, "admin"); + assertThat(accepted.status()).isEqualTo(HttpStatus.OK); + assertThat(exists(TARGET)).isFalse(); + assertThat(ownerOf("inherited_catalog")).isEqualTo("admin"); + } + + /** The list an administrator is meant to consult before deciding. */ + @Test + public void ownedObjectsAreListedBeforeTheDecision() throws IOException { + createCatalog("inherited_catalog"); + giveOwnership("inherited_catalog", TARGET); + + AggregatedHttpResponse response = + send(HttpMethod.GET, CONTROL_PATH + "/scim2/Users/" + targetId + "/ownedObjects", ""); + + assertThat(response.status()).isEqualTo(HttpStatus.OK); + JsonNode body = MAPPER.readTree(response.contentUtf8()); + assertThat(body.get("principal").asText()).isEqualTo(TARGET); + assertThat(body.get("owned").toString()).contains("inherited_catalog"); + } + + // ---------------------------------------------------------------- helpers + + private AggregatedHttpResponse purge(String id, String confirm, String reassignTo) { + String query = + "?purge=true&confirm_principal=" + URLEncoder.encode(confirm, StandardCharsets.UTF_8); + if (reassignTo != null) { + query += "&reassign_to=" + URLEncoder.encode(reassignTo, StandardCharsets.UTF_8); + } + return send(HttpMethod.DELETE, CONTROL_PATH + "/scim2/Users/" + id + query, ""); + } + + private void deactivate(String id) { + AggregatedHttpResponse response = + send( + HttpMethod.PATCH, + CONTROL_PATH + "/scim2/Users/" + id, + "{\"schemas\":[\"urn:ietf:params:scim:api:messages:2.0:PatchOp\"]," + + "\"Operations\":[{\"op\":\"replace\",\"value\":{\"active\":false}}]}"); + assertThat(response.status()).isEqualTo(HttpStatus.OK); + } + + private void createCatalog(String name) { + AggregatedHttpResponse response = + send(HttpMethod.POST, CATALOG_PATH + "/catalogs", "{\"name\":\"" + name + "\"}"); + assertThat(response.status().code()).isIn(200, 201); + } + + /** Ownership is an email on the row, so this is all it takes to stand one up. */ + private void giveOwnership(String catalogName, String email) { + try (Session session = hibernateConfigurator.getSessionFactory().openSession()) { + session.beginTransaction(); + CatalogInfoDAO dao = + session + .createQuery("FROM CatalogInfoDAO WHERE name = :name", CatalogInfoDAO.class) + .setParameter("name", catalogName) + .uniqueResult(); + dao.setOwner(email); + session.merge(dao); + session.getTransaction().commit(); + } + } + + private String ownerOf(String catalogName) { + try (Session session = hibernateConfigurator.getSessionFactory().openSession()) { + return session + .createQuery("FROM CatalogInfoDAO WHERE name = :name", CatalogInfoDAO.class) + .setParameter("name", catalogName) + .uniqueResult() + .getOwner(); + } + } + + private String signIn() { + String form = "username=admin&password=" + URLEncoder.encode(PASSWORD, StandardCharsets.UTF_8); + AggregatedHttpResponse response = + client + .execute( + RequestHeaders.builder() + .method(HttpMethod.POST) + .path(CONTROL_PATH + "/auth/admin/login") + .contentType(MediaType.FORM_DATA) + .build(), + HttpData.ofUtf8(form)) + .aggregate() + .join(); + assertThat(response.status()).isEqualTo(HttpStatus.OK); + try { + return MAPPER.readTree(response.contentUtf8()).get("access_token").asText(); + } catch (IOException e) { + throw new IllegalStateException("admin sign-in did not return an access token", e); + } + } + + private String createUser(String email) { + AggregatedHttpResponse response = + send( + HttpMethod.POST, + CONTROL_PATH + "/scim2/Users", + "{\"displayName\":\"" + + email + + "\",\"emails\":[{\"primary\":true,\"value\":\"" + + email + + "\"}]}"); + assertThat(response.status()).isEqualTo(HttpStatus.CREATED); + try { + return MAPPER.readTree(response.contentUtf8()).get("id").asText(); + } catch (IOException e) { + throw new IllegalStateException("user creation did not return an id", e); + } + } + + private boolean exists(String principal) throws IOException { + return findUser(principal) != null; + } + + private String idOf(String principal) throws IOException { + JsonNode user = findUser(principal); + if (user == null) { + throw new IllegalStateException("no user named " + principal); + } + return user.get("id").asText(); + } + + private JsonNode findUser(String principal) throws IOException { + AggregatedHttpResponse response = send(HttpMethod.GET, CONTROL_PATH + "/scim2/Users", ""); + assertThat(response.status()).isEqualTo(HttpStatus.OK); + for (JsonNode user : MAPPER.readTree(response.contentUtf8()).get("Resources")) { + if (principal.equals(user.path("userName").asText())) { + return user; + } + } + return null; + } + + private AggregatedHttpResponse send(HttpMethod method, String path, String body) { + RequestHeaders headers = + RequestHeaders.builder() + .method(method) + .path(path) + .contentType(MediaType.JSON) + .add(HttpHeaderNames.AUTHORIZATION, "Bearer " + adminToken) + .build(); + return client.execute(headers, HttpData.ofUtf8(body)).aggregate().join(); + } +} diff --git a/ui/README.md b/ui/README.md index 399a870a..dd27375a 100644 --- a/ui/README.md +++ b/ui/README.md @@ -87,6 +87,33 @@ only to owners/admins. Consequently: permissions" button) and the same warning is shown in the drawer. See `src/hooks/userAccess.ts`. +**Ownership is the exception.** `GET /scim2/Users/{id}/ownedObjects` answers +"what does this user own?" with one indexed query per securable type, because +ownership is a column on each object rather than something that has to be +inferred from the permission graph. The delete dialog uses it freely +(`src/hooks/users.ts`, `useUserOwnedObjects`); it is not the expensive scan +above and carries none of its warnings. + +### Deactivating and deleting users + +Two separate actions in the row menu, and the difference matters: + +- **Deactivate** (`PATCH /scim2/Users/{id}`) flips one flag. The user is + refused on every request from that moment — the server re-reads their state + per call, so tokens already issued stop working at once — while their grants, + the objects they own and their email are left exactly as they were. One click + undoes it. A `PUT` would also work but writes back the whole resource, so the + patch is used to guarantee nothing else changes. +- **Delete permanently** (`DELETE /scim2/Users/{id}?purge=true`) removes the + record. Its only effect beyond deactivation is freeing the email and + externalId for reuse, so it is offered only for an already-deactivated user. + The dialog shows what the user owns and requires a principal to inherit it, + plus the user's principal typed back — the list mixes people with Relyt + instance principals, which look alike. + +Every guard is enforced by the server (see `Scim2UserService`); the gating here +is guidance, exactly as in "Button gating" below. + ### Button gating (positive signals only) Action buttons render enabled only on a *positive* authorization signal diff --git a/ui/src/components/users/DeleteUserModal.test.tsx b/ui/src/components/users/DeleteUserModal.test.tsx new file mode 100644 index 00000000..d33806df --- /dev/null +++ b/ui/src/components/users/DeleteUserModal.test.tsx @@ -0,0 +1,160 @@ +import { fireEvent, render, screen } from '@testing-library/react'; +import { QueryClient, QueryClientProvider } from '@tanstack/react-query'; +import DeleteUserModal from './DeleteUserModal'; +import { + usePurgeScimUser, + useScimUserOptions, + useUserOwnedObjects, +} from '../../hooks/users'; +import { useCurrentPrincipal } from '../../hooks/authz'; + +jest.mock('../../hooks/users', () => ({ + ...jest.requireActual('../../hooks/users'), + useUserOwnedObjects: jest.fn(), + useScimUserOptions: jest.fn(), + usePurgeScimUser: jest.fn(), +})); +jest.mock('../../hooks/authz'); +jest.mock('../../utils/NotificationContext', () => ({ + useNotification: () => ({ setNotification: jest.fn() }), +})); + +const mockUseUserOwnedObjects = useUserOwnedObjects as jest.Mock; +const mockUseScimUserOptions = useScimUserOptions as jest.Mock; +const mockUsePurgeScimUser = usePurgeScimUser as jest.Mock; +const mockUseCurrentPrincipal = useCurrentPrincipal as jest.Mock; + +const mutate = jest.fn(); +const PRINCIPAL = 'alice@example.com'; + +function renderModal() { + const client = new QueryClient({ + defaultOptions: { queries: { retry: false } }, + }); + return render( + + + , + ); +} + +function deleteButton() { + return screen.getByRole('button', { name: 'Delete permanently' }); +} + +function ownsNothing() { + mockUseUserOwnedObjects.mockReturnValue({ + data: { principal: PRINCIPAL, owned: [] }, + isPending: false, + error: null, + }); +} + +beforeEach(() => { + jest.clearAllMocks(); + mockUsePurgeScimUser.mockReturnValue({ mutate, isPending: false }); + mockUseCurrentPrincipal.mockReturnValue({ data: 'admin@example.com' }); + mockUseScimUserOptions.mockReturnValue([ + { value: 'admin@example.com', label: 'Admin (admin@example.com)' }, + { value: PRINCIPAL, label: `Alice (${PRINCIPAL})` }, + ]); + ownsNothing(); +}); + +describe('DeleteUserModal', () => { + it('stays disabled until the principal is typed back exactly', () => { + renderModal(); + expect(deleteButton()).toBeDisabled(); + + fireEvent.change(screen.getByLabelText('Confirm principal'), { + target: { value: 'alice@example.co' }, + }); + expect(deleteButton()).toBeDisabled(); + + fireEvent.change(screen.getByLabelText('Confirm principal'), { + target: { value: PRINCIPAL }, + }); + expect(deleteButton()).toBeEnabled(); + }); + + it('deletes without a reassignment when the user owns nothing', () => { + renderModal(); + fireEvent.change(screen.getByLabelText('Confirm principal'), { + target: { value: PRINCIPAL }, + }); + fireEvent.click(deleteButton()); + + expect(mutate).toHaveBeenCalledWith( + { id: 'user-1', principal: PRINCIPAL, reassignTo: undefined }, + expect.anything(), + ); + }); + + it('lists what the user owns and hands it to the acting administrator', () => { + mockUseUserOwnedObjects.mockReturnValue({ + data: { + principal: PRINCIPAL, + owned: [ + { + securable_type: 'catalog', + full_name: 'sales', + securable_id: 'c1', + }, + { + securable_type: 'table', + full_name: 'sales.raw.orders', + securable_id: 't1', + }, + ], + }, + isPending: false, + error: null, + }); + renderModal(); + + expect(screen.getByText('sales.raw.orders')).toBeInTheDocument(); + // The acting administrator is the default target -- the built-in `admin` + // account may itself be deactivated, so it is a poor place to park assets. + expect(screen.getByText('Admin (admin@example.com)')).toBeInTheDocument(); + + fireEvent.change(screen.getByLabelText('Confirm principal'), { + target: { value: PRINCIPAL }, + }); + fireEvent.click(deleteButton()); + + expect(mutate).toHaveBeenCalledWith( + { + id: 'user-1', + principal: PRINCIPAL, + reassignTo: 'admin@example.com', + }, + expect.anything(), + ); + }); + + it('never offers the user being deleted as the new owner', () => { + mockUseUserOwnedObjects.mockReturnValue({ + data: { + principal: PRINCIPAL, + owned: [ + { + securable_type: 'catalog', + full_name: 'sales', + securable_id: 'c1', + }, + ], + }, + isPending: false, + error: null, + }); + renderModal(); + + expect(screen.queryByText(`Alice (${PRINCIPAL})`)).not.toBeInTheDocument(); + }); +}); diff --git a/ui/src/components/users/DeleteUserModal.tsx b/ui/src/components/users/DeleteUserModal.tsx new file mode 100644 index 00000000..a0aee039 --- /dev/null +++ b/ui/src/components/users/DeleteUserModal.tsx @@ -0,0 +1,224 @@ +import { useMemo, useState } from 'react'; +import { + Alert, + Modal, + Select, + Spin, + Table, + Typography, + Input, + Form, +} from 'antd'; +import { useNotification } from '../../utils/NotificationContext'; +import { + OwnedObjectInterface, + usePurgeScimUser, + useScimUserOptions, + useUserOwnedObjects, +} from '../../hooks/users'; +import { useCurrentPrincipal } from '../../hooks/authz'; + +interface DeleteUserModalProps { + open: boolean; + closeModal: () => void; + /** SCIM id of the user being deleted. */ + userId: string; + /** Their principal (primary email) — what the caller has to type back. */ + principal: string; + displayName?: string; +} + +/** + * Permanent deletion of a user, with its consequences shown first. + * + * Two inputs that are easy to confuse and must not be: the **reassignment + * target** decides who inherits the user's objects, while the **typed + * principal** only confirms which user is being deleted. They can never be the + * same value — the server rejects handing ownership to the account being + * removed. + * + * Ownership is transferred rather than offering to delete the objects: a + * cascade behind a confirmation dialog is how an entire catalog disappears by + * accident, and the objects can still be deleted individually afterwards. + */ +export default function DeleteUserModal({ + open, + closeModal, + userId, + principal, + displayName, +}: DeleteUserModalProps) { + const { data, isPending, error } = useUserOwnedObjects(userId, open); + const { data: currentPrincipal } = useCurrentPrincipal(); + const userOptions = useScimUserOptions(); + const mutation = usePurgeScimUser(); + const { setNotification } = useNotification(); + + const [reassignTo, setReassignTo] = useState(undefined); + const [typedPrincipal, setTypedPrincipal] = useState(''); + + const owned: OwnedObjectInterface[] = useMemo( + () => data?.owned ?? [], + [data], + ); + + // Anyone but the user being deleted. The current administrator is offered + // first: they are demonstrably able to act on what they inherit, whereas the + // built-in `admin` account may itself be deactivated. + const reassignOptions = useMemo( + () => userOptions.filter((option) => option.value !== principal), + [userOptions, principal], + ); + + // Only meaningful when there is something to hand over: sending a + // reassignment target for a user who owns nothing would claim a transfer + // that never happens. + const effectiveReassignTo = + owned.length === 0 + ? undefined + : (reassignTo ?? + (currentPrincipal && currentPrincipal !== principal + ? currentPrincipal + : undefined)); + + const confirmed = typedPrincipal === principal; + const reassignmentSatisfied = owned.length === 0 || !!effectiveReassignTo; + + const close = () => { + setReassignTo(undefined); + setTypedPrincipal(''); + closeModal(); + }; + + return ( + + Delete {displayName ?? principal} permanently + + } + okText="Delete permanently" + cancelText="Cancel" + open={open} + destroyOnClose + onCancel={close} + okButtonProps={{ + danger: true, + loading: mutation.isPending, + disabled: !confirmed || !reassignmentSatisfied || isPending, + }} + onOk={() => + mutation.mutate( + { id: userId, principal, reassignTo: effectiveReassignTo }, + { + onError: (err: Error) => setNotification(err.message, 'error'), + onSuccess: () => { + setNotification(`User ${principal} deleted`, 'success'); + close(); + }, + }, + ) + } + > + + This removes the user record and frees {principal} to be + registered again. It cannot be undone. Access is already blocked by the + deactivation — delete only to clean up the directory. + + + {error && ( + + )} + + {isPending && !error && } + + {!isPending && !error && owned.length === 0 && ( + + )} + + {owned.length > 0 && ( + <> + + + size="small" + pagination={owned.length > 10 ? { pageSize: 10 } : false} + dataSource={owned} + rowKey={(record) => + `${record.securable_type}-${record.securable_id}` + } + style={{ marginBottom: 16 }} + columns={[ + { + title: 'Type', + dataIndex: 'securable_type', + key: 'securable_type', + width: '30%', + }, + { title: 'Name', dataIndex: 'full_name', key: 'full_name' }, + ]} + /> +

+ Reassign ownership to + } + // Credentials and external locations carry access to cloud + // storage; whoever inherits them inherits that responsibility. + extra={ + owned.some((object) => + ['credential', 'external_location'].includes( + object.securable_type ?? '', + ), + ) + ? 'Includes credentials or external locations — the new owner becomes responsible for the cloud access they grant.' + : undefined + } + > + setTypedPrincipal(event.target.value)} + placeholder={principal} + aria-label="Confirm principal" + /> + +
+ + ); +} diff --git a/ui/src/components/users/UserActionsDropdown.test.tsx b/ui/src/components/users/UserActionsDropdown.test.tsx new file mode 100644 index 00000000..ebdf6943 --- /dev/null +++ b/ui/src/components/users/UserActionsDropdown.test.tsx @@ -0,0 +1,260 @@ +import { fireEvent, render, screen } from '@testing-library/react'; +import { QueryClient, QueryClientProvider } from '@tanstack/react-query'; +import UserActionsDropdown from './UserActionsDropdown'; +import { useSetMetastoreAdmin, useSetScimUserActive } from '../../hooks/users'; +import { useCurrentPrincipal, useIsMetastoreAdmin } from '../../hooks/authz'; + +jest.mock('../../hooks/users', () => ({ + ...jest.requireActual('../../hooks/users'), + useSetScimUserActive: jest.fn(), + useSetMetastoreAdmin: jest.fn(), +})); +jest.mock('../../hooks/authz'); +// Both modals open in a portal and pull in their own data; the dropdown's job +// is only to offer the right entries. +jest.mock('./GrantToUserModal', () => () => null); +// Stands in for the confirmation dialog with just enough to click inside: what matters here is +// whether such a click escapes to the row, not what the real dialog renders. +jest.mock('./DeleteUserModal', () => { + const react = jest.requireActual('react'); + return ({ open }: { open: boolean }) => + open + ? react.createElement('input', { 'aria-label': 'type the principal' }) + : null; +}); +jest.mock('../../utils/NotificationContext', () => ({ + useNotification: () => ({ setNotification: jest.fn() }), +})); + +const mockUseSetScimUserActive = useSetScimUserActive as jest.Mock; +const mockUseSetMetastoreAdmin = useSetMetastoreAdmin as jest.Mock; +const mockUseIsMetastoreAdmin = useIsMetastoreAdmin as jest.Mock; +const mockUseCurrentPrincipal = useCurrentPrincipal as jest.Mock; + +const mutate = jest.fn(); +const setAdminMutate = jest.fn(); + +function openMenu({ + principal = 'alice@example.com', + active = true, + isAdmin = false, +}: { principal?: string; active?: boolean; isAdmin?: boolean } = {}) { + const client = new QueryClient({ + defaultOptions: { queries: { retry: false } }, + }); + render( + + + , + ); + fireEvent.click(screen.getByLabelText(`Actions for ${principal}`)); +} + +beforeEach(() => { + jest.clearAllMocks(); + mockUseSetScimUserActive.mockReturnValue({ mutate, isPending: false }); + mockUseSetMetastoreAdmin.mockReturnValue({ + mutate: setAdminMutate, + isPending: false, + }); + mockUseIsMetastoreAdmin.mockReturnValue({ data: true }); + mockUseCurrentPrincipal.mockReturnValue({ data: 'admin@example.com' }); +}); + +describe('UserActionsDropdown', () => { + it('offers deactivation, but not deletion, for an active user', async () => { + openMenu({ active: true }); + + expect(await screen.findByText('Deactivate')).toBeInTheDocument(); + expect(screen.queryByText('Reactivate')).not.toBeInTheDocument(); + // Deleting is only reachable once the account is already deactivated, so + // the irreversible step sits behind a state the admin has seen. + expect(screen.queryByText('Delete permanently')).not.toBeInTheDocument(); + }); + + it('offers reactivation and permanent deletion for an inactive user', async () => { + openMenu({ active: false }); + + expect(await screen.findByText('Reactivate')).toBeInTheDocument(); + expect(screen.getByText('Delete permanently')).toBeInTheDocument(); + expect(screen.queryByText('Deactivate')).not.toBeInTheDocument(); + }); + + it('deactivates through a SCIM patch rather than a delete', async () => { + openMenu({ active: true }); + + fireEvent.click(await screen.findByText('Deactivate')); + + expect(mutate).toHaveBeenCalledWith( + { id: 'user-1', active: false }, + expect.anything(), + ); + }); + + it('offers to promote an ordinary active user', async () => { + openMenu({ active: true, isAdmin: false }); + + fireEvent.click(await screen.findByText('Make administrator')); + + expect(setAdminMutate).toHaveBeenCalledWith( + { email: 'alice@example.com', admin: true }, + expect.anything(), + ); + }); + + it('offers to withdraw the privilege from an administrator', async () => { + openMenu({ active: true, isAdmin: true }); + + fireEvent.click(await screen.findByText('Withdraw administrator')); + + expect(setAdminMutate).toHaveBeenCalledWith( + { email: 'alice@example.com', admin: false }, + expect.anything(), + ); + }); + + it('does not offer administrator status for a deactivated user', async () => { + openMenu({ active: false }); + + expect(await screen.findByText('Reactivate')).toBeInTheDocument(); + expect(screen.queryByText('Make administrator')).not.toBeInTheDocument(); + expect( + screen.queryByText('Withdraw administrator'), + ).not.toBeInTheDocument(); + }); + + it('hides state changes from a caller who is not a metastore admin', async () => { + mockUseIsMetastoreAdmin.mockReturnValue({ data: false }); + openMenu({ active: true }); + + expect(await screen.findByText('Grant access')).toBeInTheDocument(); + expect(screen.queryByText('Deactivate')).not.toBeInTheDocument(); + expect(screen.queryByText('Delete permanently')).not.toBeInTheDocument(); + }); + + it('will not let an admin deactivate their own account', async () => { + mockUseCurrentPrincipal.mockReturnValue({ data: 'alice@example.com' }); + openMenu({ principal: 'alice@example.com', active: true }); + + fireEvent.click(await screen.findByText('Deactivate')); + + expect(mutate).not.toHaveBeenCalled(); + }); + + it('will not let an admin delete their own deactivated account', async () => { + mockUseCurrentPrincipal.mockReturnValue({ data: 'alice@example.com' }); + openMenu({ principal: 'alice@example.com', active: false }); + + const item = await screen.findByText('Delete permanently'); + // eslint-disable-next-line testing-library/no-node-access -- the disabled state lives on the antd menu item wrapping this label + expect(item.closest('.ant-dropdown-menu-item-disabled')).not.toBeNull(); + }); + + it('will not offer to delete the bootstrap administrator', async () => { + openMenu({ principal: 'admin', active: false }); + + const item = await screen.findByText('Delete permanently'); + // eslint-disable-next-line testing-library/no-node-access -- the disabled state lives on the antd menu item wrapping this label + expect(item.closest('.ant-dropdown-menu-item-disabled')).not.toBeNull(); + }); + + /** + * The bootstrap administrator is the account the password sign-in accepts — the way back in when + * the identity provider is unavailable. Deactivating it closes that door, and taking its + * administrator status away leaves an account that signs in and administers nothing, which no + * restart repairs. The server refuses both; the menu should not offer them either. + */ + it('will not offer to deactivate the bootstrap administrator', async () => { + openMenu({ principal: 'admin', active: true }); + + const item = await screen.findByText('Deactivate'); + // eslint-disable-next-line testing-library/no-node-access -- the disabled state lives on the antd menu item wrapping this label + expect(item.closest('.ant-dropdown-menu-item-disabled')).not.toBeNull(); + }); + + it('will not offer to withdraw the bootstrap administrator’s status', async () => { + openMenu({ principal: 'admin', active: true, isAdmin: true }); + + const item = await screen.findByText('Withdraw administrator'); + // eslint-disable-next-line testing-library/no-node-access -- the disabled state lives on the antd menu item wrapping this label + expect(item.closest('.ant-dropdown-menu-item-disabled')).not.toBeNull(); + }); + + /** + * The dropdown sits inside a table row whose own click opens the user drawer. Ant Design puts + * the menu in a portal, but React events travel the React tree rather than the DOM, so without + * this every action also opened the drawer — including Access details, which opens a panel of + * its own and so opened two. + */ + it('does not let a menu click reach the row underneath', async () => { + const rowClick = jest.fn(); + const client = new QueryClient({ + defaultOptions: { queries: { retry: false } }, + }); + render( + // eslint-disable-next-line jsx-a11y/click-events-have-key-events, jsx-a11y/no-static-element-interactions -- stands in for the table row, which antd makes clickable the same way +
+ + + +
, + ); + + // Opening the menu must not reach the row... + fireEvent.click(screen.getByLabelText('Actions for alice@example.com')); + expect(rowClick).not.toHaveBeenCalled(); + + // ...nor must choosing an action, which still does its own job. + fireEvent.click(await screen.findByText('Deactivate')); + expect(mutate).toHaveBeenCalledWith( + { id: 'user-1', active: false }, + expect.anything(), + ); + expect(rowClick).not.toHaveBeenCalled(); + }); + + /** + * And the same for a dialog an action opened. Typing the principal into the delete confirmation + * was opening the drawer behind the dialog on every click in the field — the modal is a portal, + * but on the React tree it is still rendered from inside the row. + */ + it('does not let a click inside a dialog reach the row underneath', async () => { + const rowClick = jest.fn(); + const client = new QueryClient({ + defaultOptions: { queries: { retry: false } }, + }); + render( + // eslint-disable-next-line jsx-a11y/click-events-have-key-events, jsx-a11y/no-static-element-interactions -- stands in for the table row, which antd makes clickable the same way +
+ + + +
, + ); + + fireEvent.click(screen.getByLabelText('Actions for alice@example.com')); + fireEvent.click(await screen.findByText('Delete permanently')); + + fireEvent.click(screen.getByLabelText('type the principal')); + + expect(rowClick).not.toHaveBeenCalled(); + }); +}); diff --git a/ui/src/components/users/UserActionsDropdown.tsx b/ui/src/components/users/UserActionsDropdown.tsx index 527fb81e..315ff4ce 100644 --- a/ui/src/components/users/UserActionsDropdown.tsx +++ b/ui/src/components/users/UserActionsDropdown.tsx @@ -1,11 +1,33 @@ -import { MoreOutlined, SafetyOutlined, TableOutlined } from '@ant-design/icons'; -import { Button, Dropdown, MenuProps } from 'antd'; +import { + CrownOutlined, + DeleteOutlined, + MoreOutlined, + SafetyOutlined, + StopOutlined, + TableOutlined, + UndoOutlined, +} from '@ant-design/icons'; +import { Button, Dropdown, MenuProps, Tooltip } from 'antd'; import { useMemo, useState } from 'react'; import GrantToUserModal from './GrantToUserModal'; +import DeleteUserModal from './DeleteUserModal'; +import { useSetMetastoreAdmin, useSetScimUserActive } from '../../hooks/users'; +import { useCurrentPrincipal, useIsMetastoreAdmin } from '../../hooks/authz'; +import { useNotification } from '../../utils/NotificationContext'; + +/** The account behind the password sign-in; see deploy/README "Administrator sign-in". */ +const BOOTSTRAP_ADMIN = 'admin'; interface UserActionsDropdownProps { /** The user's primary email; Unity Catalog principals are emails. */ principal: string; + /** SCIM id, needed by everything that modifies the user. */ + userId?: string; + /** Whether the user is currently active. */ + active?: boolean; + /** Whether the user holds OWNER on the metastore. */ + isAdmin?: boolean; + displayName?: string; /** Opens the (expensive) full permission scan for this user. */ onShowAccessDetails: () => void; } @@ -18,16 +40,39 @@ interface UserActionsDropdownProps { * per-user permissions lookup, and it only starts on an explicit click. Putting * the grant entry here means an admin can hand out access without ever paying * for that scan. + * + * Deactivation and deletion are two different things and are presented as such. + * Deactivating blocks the user immediately and completely — the server checks + * their state on every request — and is undone with one click. Deleting only + * frees the principal for reuse, and cannot be undone; it is offered solely for + * an already-deactivated user, so the irreversible step sits behind a state the + * administrator has seen. + * + * The gates here are guidance, not enforcement: the server re-checks all of + * them and a 403 remains possible (see hooks/authz.ts). */ export default function UserActionsDropdown({ principal, + userId, + active, + isAdmin, + displayName, onShowAccessDetails, }: UserActionsDropdownProps) { const [dropdownVisible, setDropdownVisible] = useState(false); const [grantOpen, setGrantOpen] = useState(false); + const [deleteOpen, setDeleteOpen] = useState(false); + const { data: isMetastoreAdmin } = useIsMetastoreAdmin(); + const { data: currentPrincipal } = useCurrentPrincipal(); + const setActive = useSetScimUserActive(); + const setAdmin = useSetMetastoreAdmin(); + const { setNotification } = useNotification(); - const menuItems = useMemo( - (): MenuProps['items'] => [ + const isSelf = !!currentPrincipal && currentPrincipal === principal; + const isBootstrapAdmin = principal === BOOTSTRAP_ADMIN; + + const menuItems = useMemo((): MenuProps['items'] => { + const items: MenuProps['items'] = [ { key: 'grantAccess', label: 'Grant access', @@ -40,12 +85,148 @@ export default function UserActionsDropdown({ onClick: onShowAccessDetails, icon: , }, - ], - [onShowAccessDetails], - ); + ]; + + // Only a metastore admin may change another account's state, and without a + // SCIM id there is nothing to address. + if (!isMetastoreAdmin || !userId) { + return items; + } + + items.push({ type: 'divider' }); + + if (active) { + // Administrator status is only offered for an active user: the server + // will not grant it to a deactivated account, and withdrawing it from one + // is a distraction from reactivating or deleting them. + items.push({ + key: 'admin', + icon: , + // The bootstrap administrator keeps the privilege: the password sign-in + // would still work without it but administer nothing, and no restart + // puts it back. + disabled: isAdmin && isBootstrapAdmin, + label: ( + + {isAdmin ? 'Withdraw administrator' : 'Make administrator'} + + ), + onClick: () => + setAdmin.mutate( + { email: principal, admin: !isAdmin }, + { + onError: (error: Error) => + setNotification(error.message, 'error'), + onSuccess: () => + setNotification( + isAdmin + ? `${principal} is no longer an administrator` + : `${principal} is now an administrator`, + 'success', + ), + }, + ), + }); + items.push({ + key: 'deactivate', + icon: , + // Deactivating yourself locks you out of the UI, and only another + // administrator (or the password sign-in) can undo it. Deactivating the + // bootstrap administrator closes the password sign-in itself, which is + // the way back in when the identity provider is unavailable. + disabled: isSelf || isBootstrapAdmin, + label: ( + + Deactivate + + ), + onClick: () => + setActive.mutate( + { id: userId, active: false }, + { + onError: (error: Error) => + setNotification(error.message, 'error'), + onSuccess: () => + setNotification(`User ${principal} deactivated`, 'success'), + }, + ), + }); + } else { + items.push({ + key: 'reactivate', + icon: , + label: 'Reactivate', + onClick: () => + setActive.mutate( + { id: userId, active: true }, + { + onError: (error: Error) => + setNotification(error.message, 'error'), + onSuccess: () => + setNotification(`User ${principal} reactivated`, 'success'), + }, + ), + }); + items.push({ + key: 'delete', + icon: , + danger: true, + disabled: isSelf || isBootstrapAdmin, + label: ( + + Delete permanently + + ), + onClick: () => setDeleteOpen(true), + }); + } + + return items; + }, [ + onShowAccessDetails, + isMetastoreAdmin, + userId, + active, + isAdmin, + isSelf, + isBootstrapAdmin, + principal, + setActive, + setAdmin, + setNotification, + ]); return ( - <> + // The row opens the user drawer when clicked, and everything below is rendered from inside + // that row's cell. Ant Design puts menus and modals in portals, so they look detached from + // it, but React events travel the React tree rather than the DOM — on that tree all of this + // is still the cell's child. One stop here covers the lot: opening the menu, choosing an + // action, and clicking or typing inside a dialog the action opened. Without it the drawer + // kept appearing behind whatever had just been opened. + // eslint-disable-next-line jsx-a11y/no-static-element-interactions, jsx-a11y/click-events-have-key-events -- not a control: it only keeps clicks from escaping to the row + e.stopPropagation()}> e.stopPropagation()} icon={ setGrantOpen(false)} principal={principal} /> - + {userId && ( + setDeleteOpen(false)} + userId={userId} + principal={principal} + displayName={displayName} + /> + )} + ); } diff --git a/ui/src/hooks/users.test.tsx b/ui/src/hooks/users.test.tsx new file mode 100644 index 00000000..7b88d006 --- /dev/null +++ b/ui/src/hooks/users.test.tsx @@ -0,0 +1,109 @@ +import { useEffect } from 'react'; +import { screen } from '@testing-library/react'; +import { renderWithProviders } from '../test-utils/render'; +import { programClient, requestsTo } from '../test-utils/mockClient'; +import { useSetScimUserActive, usePurgeScimUser } from './users'; + +// Inlined rather than calling mockClientModule(): jest.mock is hoisted above the imports, so the +// factory cannot close over anything this file imports. +jest.mock('../context/client', () => ({ CLIENT: { request: jest.fn() } })); + +const USER_ID = 'u-1'; +const USERS_URL = `/scim2/Users/${USER_ID}`; + +/** + * Fires a mutation once on mount. These tests are about the request that leaves the browser, not + * about any UI, so the component exists only to run the hook. + */ +function renderMutation( + useHook: () => { mutate: (vars: never) => void }, + vars: unknown, +) { + function Probe() { + const { mutate } = useHook(); + useEffect(() => { + mutate(vars as never); + // Mount only: re-firing on every render would loop, since the mutation itself re-renders. + // eslint-disable-next-line react-hooks/exhaustive-deps + }, []); + return
fired
; + } + renderWithProviders(); +} + +describe('useSetScimUserActive request shape', () => { + beforeEach(() => { + programClient([{ method: 'patch', url: USERS_URL, response: {} }]); + }); + + /** + * The bug this pins down: the UI used to send `value: false`. RFC 7644 §3.5.2.1 says a `replace` + * without a `path` carries the object of attributes to replace, and the server's SCIM library + * enforces that while deserialising — a bare scalar is rejected as a 500 before any handler runs, + * so the user saw "failed to parse a JSON document" instead of a deactivated account. + */ + it('sends a pathless replace whose value is an object, not a scalar', async () => { + renderMutation(useSetScimUserActive, { id: USER_ID, active: false }); + await screen.findByText('fired'); + + const [request] = requestsTo('patch', USERS_URL); + expect(request).toBeDefined(); + + const operation = request.data.Operations[0]; + expect(operation.op).toBe('replace'); + expect(operation.value).toEqual({ active: false }); + // A scalar value is exactly what the server refuses. + expect(typeof operation.value).toBe('object'); + // A path would take the request down the branch the server answers with 501. + expect(operation.path).toBeUndefined(); + expect(request.data.schemas).toEqual([ + 'urn:ietf:params:scim:api:messages:2.0:PatchOp', + ]); + }); + + it('reactivating sends the same shape with active true', async () => { + renderMutation(useSetScimUserActive, { id: USER_ID, active: true }); + await screen.findByText('fired'); + + const [request] = requestsTo('patch', USERS_URL); + expect(request.data.Operations[0].value).toEqual({ active: true }); + }); +}); + +describe('usePurgeScimUser request shape', () => { + beforeEach(() => { + programClient([{ method: 'delete', url: USERS_URL, response: {} }]); + }); + + /** + * Purging is the irreversible one, and the server refuses it unless the principal is repeated + * back. If the UI ever stopped sending that, every purge would fail — or, worse, a future server + * that stopped checking would delete whichever row the caller happened to name. + */ + it('sends purge with the principal confirmed back', async () => { + renderMutation(usePurgeScimUser, { + id: USER_ID, + principal: 'someone@example.com', + }); + await screen.findByText('fired'); + + const [request] = requestsTo('delete', USERS_URL); + expect(request.params).toMatchObject({ + purge: true, + confirm_principal: 'someone@example.com', + }); + expect(request.params.reassign_to).toBeUndefined(); + }); + + it('passes reassign_to only when one was chosen', async () => { + renderMutation(usePurgeScimUser, { + id: USER_ID, + principal: 'someone@example.com', + reassignTo: 'heir@example.com', + }); + await screen.findByText('fired'); + + const [request] = requestsTo('delete', USERS_URL); + expect(request.params.reassign_to).toBe('heir@example.com'); + }); +}); diff --git a/ui/src/hooks/users.ts b/ui/src/hooks/users.ts index 772dcd3d..cd7beb96 100644 --- a/ui/src/hooks/users.ts +++ b/ui/src/hooks/users.ts @@ -99,6 +99,235 @@ export function useScimUserOptions() { }, [data]); } +export interface OwnedObjectInterface + extends Model {} + +/** + * Everything a user owns, for the delete confirmation. + * + * Disabled until asked for: it is cheap on the server (one indexed query per + * securable type, unlike the per-object walk in `useUserAccessScan`) but there + * is no reason to run it for every row of the Users table. + */ +export function useUserOwnedObjects(id: string | undefined, enabled: boolean) { + return useQuery< + SuccessResponseBody< + ControlApi, + '/scim2/Users/{id}/ownedObjects', + 'get', + 'application/json' + > + >({ + queryKey: ['userOwnedObjects', id], + enabled: enabled && !!id, + queryFn: async () => { + const response = await (route as Route)({ + client: CLIENT, + request: { + path: '/scim2/Users/{id}/ownedObjects', + method: 'get', + params: { paths: { id: id! } }, + }, + config: { baseURL: UC_AUTH_API_PREFIX }, + errorMessage: 'Failed to fetch the objects this user owns', + }).call(); + if (isError(response)) { + // NOTE: + // When an expected error occurs, as defined in the OpenAPI specification, the following line will + // be executed. This block serves as a placeholder for expected errors. + return assertNever(response.data.status); + } + return response.data; + }, + }); +} + +export interface SetScimUserActiveParams { + id: string; + active: boolean; +} + +/** + * Deactivates or reactivates a user. + * + * Deliberately a SCIM patch rather than a DELETE: both flip the same flag, but + * a patch only ever touches `active`, whereas a PUT would write back whatever + * else the request body happened to carry. Deactivation takes effect on the + * next request the user makes — the server re-reads their state every time, so + * tokens already issued stop working at once — and nothing else about them + * changes, which is what makes it reversible. + */ +export function useSetScimUserActive() { + const queryClient = useQueryClient(); + + return useMutation({ + mutationFn: async ({ id, active }: SetScimUserActiveParams) => { + const response = await (route as Route)({ + client: CLIENT, + request: { + path: '/scim2/Users/{id}', + method: 'patch', + params: { + paths: { id }, + body: { + schemas: ['urn:ietf:params:scim:api:messages:2.0:PatchOp'], + // A pathless `replace` is the only shape the server honours, and RFC 7644 §3.5.2.1 + // requires its value to be the object of attributes to replace — not a bare scalar. + // The SCIM library enforces that while deserialising, so `value: active` never + // reaches the service: it fails as a 500 before any handler runs. + Operations: [{ op: 'replace', value: { active } }], + }, + }, + }, + config: { baseURL: UC_AUTH_API_PREFIX }, + errorMessage: active + ? 'Failed to reactivate user' + : 'Failed to deactivate user', + }).call(); + if (isError(response)) { + // NOTE: + // When an expected error occurs, as defined in the OpenAPI specification, the following line will + // be executed. This block serves as a placeholder for expected errors. + return assertNever(response.data.status); + } + return response.data; + }, + onSuccess: () => { + queryClient.invalidateQueries({ queryKey: ['listScimUsers'] }); + }, + }); +} + +export interface PurgeScimUserParams { + id: string; + /** The user's principal, repeated back — the server rejects a mismatch. */ + principal: string; + /** Who takes over the user's objects; omitted when they own nothing. */ + reassignTo?: string; +} + +/** + * Permanently removes a user. + * + * This is what frees the email and externalId for reuse; it does not block + * access any harder than deactivation, which the server already enforces on + * every request. Every guard lives on the server (deactivated first, not + * yourself, not the built-in admin, an administrator left over, a valid + * reassignment target) — what the UI does here is keep the caller from + * reaching an obviously refused state, and surface the refusal when it comes. + */ +export function usePurgeScimUser() { + const queryClient = useQueryClient(); + + return useMutation({ + mutationFn: async ({ id, principal, reassignTo }: PurgeScimUserParams) => { + const response = await (route as Route)({ + client: CLIENT, + request: { + path: '/scim2/Users/{id}', + method: 'delete', + params: { + paths: { id }, + query: { + purge: true, + confirm_principal: principal, + ...(reassignTo ? { reassign_to: reassignTo } : {}), + }, + }, + }, + config: { baseURL: UC_AUTH_API_PREFIX }, + errorMessage: 'Failed to delete user', + }).call(); + if (isError(response)) { + // NOTE: + // When an expected error occurs, as defined in the OpenAPI specification, the following line will + // be executed. This block serves as a placeholder for expected errors. + return assertNever(response.data.status); + } + return response.data; + }, + onSuccess: () => { + queryClient.invalidateQueries({ queryKey: ['listScimUsers'] }); + }, + }); +} + +/** + * The metastore's administrators, as a set of principals. + * + * Nothing else exposes this: OWNER is filtered out of /permissions responses, + * so a user list cannot otherwise tell an administrator from anyone else. + * Readable only by an administrator, so a failure means "cannot tell" rather + * than "there are none" — callers render nothing rather than guessing. + */ +export function useMetastoreAdmins() { + return useQuery>({ + queryKey: ['metastoreAdmins'], + queryFn: async () => { + try { + const response = await (route as Route)({ + client: CLIENT, + request: { path: '/metastore/admins', method: 'get' }, + config: { baseURL: UC_AUTH_API_PREFIX }, + errorMessage: 'Failed to fetch administrators', + }).call(); + if (isError(response)) return new Set(); + return new Set(response.data.admins ?? []); + } catch { + return new Set(); + } + }, + }); +} + +export interface MetastoreAdminMutationParams { + /** The user's principal (email). */ + email: string; + /** True to make them an administrator, false to withdraw it. */ + admin: boolean; +} + +/** + * Makes a user an administrator, or withdraws it. + * + * Any administrator may do either — metastore OWNER is the top of the + * privilege lattice, so granting one escalates nobody past the grantor. The + * server keeps the invariant that matters: one enabled administrator always + * remains. + */ +export function useSetMetastoreAdmin() { + const queryClient = useQueryClient(); + + return useMutation({ + mutationFn: async ({ email, admin }: MetastoreAdminMutationParams) => { + const response = await (route as Route)({ + client: CLIENT, + request: { + path: '/metastore/admins/{email}', + method: admin ? 'put' : 'delete', + params: { paths: { email } }, + }, + config: { baseURL: UC_AUTH_API_PREFIX }, + errorMessage: admin + ? 'Failed to make user an administrator' + : 'Failed to withdraw the administrator privilege', + }).call(); + if (isError(response)) { + // NOTE: + // When an expected error occurs, as defined in the OpenAPI specification, the following line will + // be executed. This block serves as a placeholder for expected errors. + return assertNever(response.data.status); + } + return response.data; + }, + onSuccess: () => { + queryClient.invalidateQueries({ queryKey: ['metastoreAdmins'] }); + // The caller may have just changed their own standing. + queryClient.invalidateQueries({ queryKey: ['isMetastoreAdmin'] }); + }, + }); +} + export interface CreateScimUserMutationParams extends RequestBody {} diff --git a/ui/src/pages/UsersList.test.tsx b/ui/src/pages/UsersList.test.tsx new file mode 100644 index 00000000..48ea720b --- /dev/null +++ b/ui/src/pages/UsersList.test.tsx @@ -0,0 +1,107 @@ +import { screen, fireEvent, within } from '@testing-library/react'; +import { renderWithProviders } from '../test-utils/render'; +import UsersList from './UsersList'; +import { useListScimUsers, useMetastoreAdmins } from '../hooks/users'; +import { useAuthorized } from '../hooks/authz'; + +jest.mock('../context/client', () => ({ CLIENT: { request: jest.fn() } })); +jest.mock('../hooks/users'); +jest.mock('../hooks/authz'); +jest.mock('../components/users/UserActionsDropdown', () => () => null); +jest.mock('../components/modals/CreateUserModal', () => ({ + CreateUserModal: () => null, +})); + +const mockUseListScimUsers = useListScimUsers as jest.Mock; +const mockUseMetastoreAdmins = useMetastoreAdmins as jest.Mock; +const mockUseAuthorized = useAuthorized as jest.Mock; + +const ACTIVE_ONE = { + id: 'u1', + displayName: 'Ada Active', + active: true, + emails: [{ primary: true, value: 'ada@example.com' }], +}; +const ACTIVE_TWO = { + id: 'u2', + displayName: 'Ben Active', + active: true, + emails: [{ primary: true, value: 'ben@example.com' }], +}; +const INACTIVE_ONE = { + id: 'u3', + displayName: 'Cleo Inactive', + active: false, + emails: [{ primary: true, value: 'cleo@example.com' }], +}; + +function renderList(users: unknown[]) { + mockUseListScimUsers.mockReturnValue({ + data: { Resources: users }, + isLoading: false, + }); + mockUseMetastoreAdmins.mockReturnValue({ data: new Set() }); + mockUseAuthorized.mockReturnValue({ allowed: true, ready: true }); + renderWithProviders(); +} + +describe('UsersList status filter', () => { + beforeEach(() => jest.clearAllMocks()); + + /** + * Deactivated accounts keep their grants and their email, so they stay in the list forever. + * Showing them alongside everyone else would make the list grow without bound and bury the + * people who can actually sign in, which is why active is the default view rather than "all". + */ + it('shows only active users by default', () => { + renderList([ACTIVE_ONE, ACTIVE_TWO, INACTIVE_ONE]); + + expect(screen.getByText('Ada Active')).toBeInTheDocument(); + expect(screen.getByText('Ben Active')).toBeInTheDocument(); + expect(screen.queryByText('Cleo Inactive')).not.toBeInTheDocument(); + }); + + it('shows the deactivated ones when asked, and only those', () => { + renderList([ACTIVE_ONE, ACTIVE_TWO, INACTIVE_ONE]); + + fireEvent.click(screen.getByText(/^Inactive/)); + + expect(screen.getByText('Cleo Inactive')).toBeInTheDocument(); + expect(screen.queryByText('Ada Active')).not.toBeInTheDocument(); + expect(screen.queryByText('Ben Active')).not.toBeInTheDocument(); + }); + + /** + * The count sits on the control rather than in the table, because the whole point is to tell an + * administrator that deactivated accounts exist while they are looking at a list without them. + */ + it('counts the deactivated accounts on the control', () => { + renderList([ACTIVE_ONE, INACTIVE_ONE]); + + expect(screen.getByText('Inactive (1)')).toBeInTheDocument(); + }); + + it('says just Inactive when there are none', () => { + renderList([ACTIVE_ONE, ACTIVE_TWO]); + + expect(screen.getByText('Inactive')).toBeInTheDocument(); + expect(screen.queryByText(/Inactive \(/)).not.toBeInTheDocument(); + }); + + it('marks each row with its state', () => { + renderList([ACTIVE_ONE, INACTIVE_ONE]); + + // eslint-disable-next-line testing-library/no-node-access -- the state tag is a sibling cell, reachable only through the row + const activeRow = screen.getByText('Ada Active').closest('tr'); + expect( + within(activeRow as HTMLElement).getByText('Active'), + ).toBeInTheDocument(); + + fireEvent.click(screen.getByText(/^Inactive/)); + // eslint-disable-next-line testing-library/no-node-access -- same reason + const inactiveRow = screen.getByText('Cleo Inactive').closest('tr'); + expect( + within(inactiveRow as HTMLElement).getByText('Inactive'), + ).toBeInTheDocument(); + }); +}); diff --git a/ui/src/pages/UsersList.tsx b/ui/src/pages/UsersList.tsx index a7e5a654..6abec75a 100644 --- a/ui/src/pages/UsersList.tsx +++ b/ui/src/pages/UsersList.tsx @@ -6,13 +6,18 @@ import { Divider, Drawer, Flex, + Segmented, Tag, Tooltip, Typography, } from 'antd'; import { TeamOutlined, UserOutlined } from '@ant-design/icons'; import ListLayout from '../components/layouts/ListLayout'; -import { ScimUserInterface, useListScimUsers } from '../hooks/users'; +import { + ScimUserInterface, + useListScimUsers, + useMetastoreAdmins, +} from '../hooks/users'; import { CreateUserModal } from '../components/modals/CreateUserModal'; import UserPermissions from '../components/users/UserPermissions'; import UserAccessDetails from '../components/users/UserAccessDetails'; @@ -25,18 +30,30 @@ interface UserRow extends ScimUserInterface { name: string; } +type StatusFilter = 'active' | 'inactive'; + function primaryEmailOf(user: ScimUserInterface): string { const emails = user.emails ?? []; return (emails.find((email) => email.primary) ?? emails[0])?.value ?? ''; } +// A user the server has not told us about is shown in the default view rather +// than in neither: the two tabs must always add up to the whole directory. +function isInactive(user: ScimUserInterface): boolean { + return user.active === false; +} + export default function UsersList() { const { data, isLoading, error } = useListScimUsers(); + // Empty for a non-administrator, who is not allowed to read it — the column + // then simply shows nothing rather than claiming there are no administrators. + const { data: admins } = useMetastoreAdmins(); const [selectedUser, setSelectedUser] = useState(null); const [accessDetailsUser, setAccessDetailsUser] = useState( null, ); const [createOpen, setCreateOpen] = useState(false); + const [status, setStatus] = useState('active'); // Server rule: only the metastore OWNER may create users; gate on the // metastore owner-side signal (fail-open when identity is unknown). const canCreateUser = useAuthorized([ @@ -52,6 +69,16 @@ export default function UsersList() { [data], ); + const inactiveCount = useMemo(() => users.filter(isInactive).length, [users]); + + const visibleUsers = useMemo( + () => + users.filter((user) => + status === 'inactive' ? isInactive(user) : !isInactive(user), + ), + [users, status], + ); + return ( <> @@ -65,6 +92,22 @@ export default function UsersList() { Users + {/* The inactive count is on the control on purpose: deactivated + accounts are the queue that permanent deletion works through, + and nobody switches to a tab they cannot tell is empty. */} + + value={status} + onChange={setStatus} + options={[ + { label: 'Active', value: 'active' }, + { + label: inactiveCount + ? `Inactive (${inactiveCount})` + : 'Inactive', + value: 'inactive', + }, + ]} + /> } - data={users} + data={visibleUsers} onRowClick={(record) => setSelectedUser(record)} rowKey={(record) => `user-${record.id}`} columns={[ @@ -112,13 +155,21 @@ export default function UsersList() { title: 'Status', dataIndex: 'active', key: 'active', - width: '10%', - render: (value) => - value ? ( - Active - ) : ( - Inactive - ), + width: '16%', + render: (value, record) => ( + + {value ? ( + Active + ) : ( + Inactive + )} + {admins?.has(primaryEmailOf(record)) && ( + + Admin + + )} + + ), }, { title: 'Created', @@ -141,6 +192,10 @@ export default function UsersList() { return principal ? ( setAccessDetailsUser(record)} /> ) : null; diff --git a/ui/src/types/api/control.gen.ts b/ui/src/types/api/control.gen.ts index c6a8f48f..a18d0211 100644 --- a/ui/src/types/api/control.gen.ts +++ b/ui/src/types/api/control.gen.ts @@ -50,12 +50,66 @@ export interface paths { put: operations['updateUser']; post?: never; /** - * Delete a user - * @description Deletes the user that matches the supplied id. + * Deactivate or permanently delete a user + * @description Without `purge`, deactivates the user — the same state change as a SCIM patch setting + * `active` to false, and equally reversible. + * + * With `purge=true`, removes the user record permanently. This is the only way to free the + * user's email and externalId for reuse; it does not block access any harder than + * deactivation, which already refuses the user on every request. Because it cannot be undone, + * the server requires that: + * + * * the caller is a metastore administrator, and is not the user being deleted; + * * the user is already deactivated; + * * `confirm_principal` repeats the user's principal exactly; + * * another enabled metastore administrator remains afterwards; + * * the built-in `admin` account is never the target — it is the account behind the + * password sign-in, the way back in when the identity provider is unavailable; + * * `reassign_to` names an enabled user, if the target owns anything. + * + * Ownership transfers in both places it is recorded: the `owner` field of each object, and + * the OWNER privileges that actually authorize. `created_by` and `updated_by` are left alone — + * they record what happened, which stays true. */ delete: operations['deleteUser']; options?: never; head?: never; + /** + * Patch a user + * @description Applies a SCIM patch to the user that matches the supplied id. Only a `replace` operation + * with no path is supported, whose value sets the user's `active` flag — this is how a user + * is deactivated and reactivated. + * + * Deactivation takes effect on the next request: the server re-reads the user's state on + * every call, so access tokens already issued stop working immediately. Nothing else about + * the user changes — their grants, the objects they own and their email are left as they + * were, which is what makes it reversible. + */ + patch: operations['patchUser']; + trace?: never; + }; + '/scim2/Users/{id}/ownedObjects': { + parameters: { + query?: never; + header?: never; + path: { + /** @description The id of the user. */ + id: string; + }; + cookie?: never; + }; + /** + * List what a user owns + * @description Every securable whose owner is this user, so that a caller can see the consequences of + * deleting them before anything happens. One indexed query per securable type, unlike the + * per-object walk needed to enumerate a user's privileges. + */ + get: operations['getUserOwnedObjects']; + put?: never; + post?: never; + delete?: never; + options?: never; + head?: never; patch?: never; trace?: never; }; @@ -79,6 +133,67 @@ export interface paths { patch?: never; trace?: never; }; + '/metastore/admins': { + parameters: { + query?: never; + header?: never; + path?: never; + cookie?: never; + }; + /** + * List the metastore's administrators + * @description The users holding OWNER on the metastore, deactivated ones included. This is the only way + * to see that: OWNER is filtered out of `/permissions` responses, so no other endpoint + * reveals who holds it. + */ + get: operations['listMetastoreAdmins']; + put?: never; + post?: never; + delete?: never; + options?: never; + head?: never; + patch?: never; + trace?: never; + }; + '/metastore/admins/{email}': { + parameters: { + query?: never; + header?: never; + path: { + /** @description The user's principal. */ + email: string; + }; + cookie?: never; + }; + get?: never; + /** + * Make a user an administrator + * @description Grants the user OWNER on the metastore, which is what being an administrator means: it + * authorizes every securable, including ones they do not personally own. + * + * Any administrator may do this. Metastore OWNER is the top of the privilege lattice, so an + * administrator granting one has escalated nobody past themselves — whereas restricting it + * to the built-in `admin` account would make the break-glass password sign-in a participant + * in routine administration. + * + * The user must exist and be active; ownership of a deactivated account authorizes nothing + * and would only obscure who is really in charge. + */ + put: operations['grantMetastoreAdmin']; + post?: never; + /** + * Withdraw a user's administrator privilege + * @description Revoking your own is allowed — handing over is a real thing to want — but only while + * somebody else still holds it. At least one *enabled* administrator must remain: a + * deactivated one cannot sign in, and a metastore left without one has no supported way back, + * because the bootstrap re-grants OWNER only when the `admin` user does not exist at all. + */ + delete: operations['revokeMetastoreAdmin']; + options?: never; + head?: never; + patch?: never; + trace?: never; + }; '/auth/capabilities': { parameters: { query?: never; @@ -332,6 +447,69 @@ export interface components { */ metastore_admin?: boolean; }; + MetastoreAdmins: { + /** @description Principals holding OWNER on the metastore. */ + admins?: string[]; + }; + /** + * @description A SCIM patch. Only a single `replace` operation with no path is honoured, and its value + * sets the user's `active` flag. + */ + PatchRequest: { + /** + * @example [ + * "urn:ietf:params:scim:api:messages:2.0:PatchOp" + * ] + */ + schemas?: string[]; + Operations?: components['schemas']['PatchOperation'][]; + }; + PatchOperation: { + /** + * @description The patch operation; only `replace` is supported. + * @example replace + */ + op?: string; + /** + * @description The attribute to replace. Omit it to send the attributes as an object in `value` (the form Okta and this project's UI use); set it to `active` to send the bare value. `active` is the only attribute this endpoint changes; anything else is answered with 501. + * @example active + */ + path?: string; + /** + * @description With no `path`, the object of attributes to replace — RFC 7644 §3.5.2.1 requires an object here, not a bare value, and the server's SCIM library enforces that while parsing, so a scalar is refused before any handler runs. With `path` set, this is instead that attribute's new value (for `active`, a boolean). + * @example { + * "active": false + * } + */ + value?: { + /** @description The user's new `active` state. */ + active?: boolean; + }; + }; + OwnedObjectsResponse: { + /** @description The user whose objects these are. */ + principal?: string; + /** + * @description Every securable this user owns. Empty means the user can be deleted without naming + * anyone to take ownership. + */ + owned?: components['schemas']['OwnedObject'][]; + }; + OwnedObject: { + /** + * @description The kind of securable, named as in /permissions/{securable_type}/{full_name}. + * @example table + */ + securable_type?: string; + /** + * @description The securable's qualified name. Falls back to the bare name when an ancestor has + * already been deleted — a catalog's removal does not clear what was under it. + * @example sales.raw.orders + */ + full_name?: string; + /** @description The securable's id. */ + securable_id?: string; + }; /** @description SCIM provides a resource type for "User" resources. */ UserResource: { /** @description The id of the user. */ @@ -583,7 +761,23 @@ export interface operations { }; deleteUser: { parameters: { - query?: never; + query?: { + /** @description Permanently remove the user record instead of deactivating them. Cannot be undone. */ + purge?: boolean; + /** + * @description Principal (email) taking ownership of everything the user owns. Required with + * `purge=true` when the user owns anything; must name an enabled user other than the + * target. Prefer the administrator performing the deletion over the built-in `admin` + * account, which may itself be deactivated. + */ + reassign_to?: string; + /** + * @description The target user's principal, repeated back. Required with `purge=true`. The user list + * mixes people with Relyt instance principals, which look alike; this is what catches + * the wrong row being selected. + */ + confirm_principal?: string; + }; header?: never; path: { /** @description The id of the user. */ @@ -593,7 +787,52 @@ export interface operations { }; requestBody?: never; responses: { - /** @description The user was successfully deleted. */ + /** @description The user was successfully deactivated or deleted. */ + 200: { + headers: { + [name: string]: unknown; + }; + content: { + 'application/scim+json': unknown; + }; + }; + /** + * @description A guard refused the purge — missing or mismatched `confirm_principal`, the user is + * still active, the target is the caller or the built-in administrator, no other enabled + * administrator would remain, or `reassign_to` is unusable. + */ + 400: { + headers: { + [name: string]: unknown; + }; + content?: never; + }; + /** @description The user owns objects and no `reassign_to` was supplied. */ + 409: { + headers: { + [name: string]: unknown; + }; + content?: never; + }; + }; + }; + patchUser: { + parameters: { + query?: never; + header?: never; + path: { + /** @description The id of the user. */ + id: string; + }; + cookie?: never; + }; + requestBody?: { + content: { + 'application/json': components['schemas']['PatchRequest']; + }; + }; + responses: { + /** @description The user was successfully patched. */ 200: { headers: { [name: string]: unknown; @@ -602,6 +841,36 @@ export interface operations { 'application/scim+json': unknown; }; }; + /** @description The patch contained no supported operation. */ + 501: { + headers: { + [name: string]: unknown; + }; + content?: never; + }; + }; + }; + getUserOwnedObjects: { + parameters: { + query?: never; + header?: never; + path: { + /** @description The id of the user. */ + id: string; + }; + cookie?: never; + }; + requestBody?: never; + responses: { + /** @description The owned objects were successfully retrieved. */ + 200: { + headers: { + [name: string]: unknown; + }; + content: { + 'application/json': components['schemas']['OwnedObjectsResponse']; + }; + }; }; }; getSelf: { @@ -624,6 +893,96 @@ export interface operations { }; }; }; + listMetastoreAdmins: { + parameters: { + query?: never; + header?: never; + path?: never; + cookie?: never; + }; + requestBody?: never; + responses: { + /** @description The administrators were successfully retrieved. */ + 200: { + headers: { + [name: string]: unknown; + }; + content: { + 'application/json': components['schemas']['MetastoreAdmins']; + }; + }; + }; + }; + grantMetastoreAdmin: { + parameters: { + query?: never; + header?: never; + path: { + /** @description The user's principal. */ + email: string; + }; + cookie?: never; + }; + requestBody?: never; + responses: { + /** @description The user is now an administrator. */ + 200: { + headers: { + [name: string]: unknown; + }; + content?: never; + }; + /** @description The user is deactivated. */ + 400: { + headers: { + [name: string]: unknown; + }; + content?: never; + }; + /** @description No such user. */ + 404: { + headers: { + [name: string]: unknown; + }; + content?: never; + }; + }; + }; + revokeMetastoreAdmin: { + parameters: { + query?: never; + header?: never; + path: { + /** @description The user's principal. */ + email: string; + }; + cookie?: never; + }; + requestBody?: never; + responses: { + /** @description The user is no longer an administrator. */ + 200: { + headers: { + [name: string]: unknown; + }; + content?: never; + }; + /** @description This would leave the metastore without an enabled administrator. */ + 400: { + headers: { + [name: string]: unknown; + }; + content?: never; + }; + /** @description No such user. */ + 404: { + headers: { + [name: string]: unknown; + }; + content?: never; + }; + }; + }; getSelfCapabilities: { parameters: { query?: never; From c00f1c648708a2173d075a7035281d3e4f72b8e7 Mon Sep 17 00:00:00 2001 From: huyang Date: Thu, 24 Sep 2026 07:31:18 +0000 Subject: [PATCH 2/4] docs: record #19 in features.md Co-Authored-By: Claude Opus 5 (1M context) --- features.md | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/features.md b/features.md index a837e55f..2c0039ff 100644 --- a/features.md +++ b/features.md @@ -59,3 +59,10 @@ Feature history of this fork, by PR merge date. - features: UI sign-in, per user. Three entry points, each at an address of its own: `/login` with **Sign in with Microsoft** (Entra ID, through an OAuth flow the server hosts end to end, so the client secret never reaches the browser), `/login/admin` for the administrator password, and `/login/token` for an access token the server issued. The application is no longer an entry point and the UI server injects nothing; what a signed-in person sees is filtered by their grants. On the server: the static JWKS file and OIDC discovery now coexist (routed by whether the issuer is registered in the file), the caller resolves through an ordered claim chain (`email`, `preferred_username`, `upn`, `sub`), discovery is cached per issuer and bounded, and Entra's `{tenantid}` issuer template is matched segment-wise. `deploy-uc.sh` derives the audience and issuer from the Microsoft settings and starts the UI with the server. - bugfix: The session cookie is now one a browser keeps -- `Secure` only when the browser is on HTTPS, and `SameSite=Lax` rather than `Strict`, which was withheld on the cross-site navigation back from the identity provider. The URL transcoder passes a redirect back instead of following it (it relayed the provider's error page as a 400) and ends a bodyless response on its headers instead of leaving the caller waiting. A refused authorization code now names the provider's error code, which separates an expired client secret from a mismatched redirect URI. + +## 2026-09-24 + +#19 (https://github.com/relytcloud/unitycatalog/pull/19) + +- features: Deactivating and deleting a user are now separate operations. Deactivation is reversible and leaves grants, owned objects and the email untouched; purging (`DELETE ...?purge=true`) is the irreversible one and the only thing that frees an email for reuse, guarded by typing the principal back, requiring the account to be deactivated first, refusing your own and the bootstrap administrator, and requiring an heir when the user owns securables -- those objects change hands as part of it. `GET .../ownedObjects` lists what is at stake beforehand. Metastore administrators can be appointed and stood down from the Users page, under the invariant that at least one *enabled* administrator always remains. The Users list separates active from deactivated accounts and carries each one's state. +- bugfix: `PATCH /scim2/Users/{id}` carried no authorization annotation, and the access decorator lets a method without one through -- any signed-in account could deactivate any other, an administrator included; it now requires metastore OWNER. The administrator password sign-in never consulted the user table, so a deactivated `admin` still received a session whose every request was then refused; it now checks the account exists and is enabled. The bootstrap administrator could be deactivated by three separate routes and stripped of its administrator status, either of which closes the password sign-in that is the way back in when the identity provider is unavailable. The PatchOp schema typed `value` as a boolean, so the UI sent a scalar where RFC 7644 3.5.2.1 requires an object and the SCIM library rejected it while parsing; both forms the RFC allows are now accepted. In the UI, choosing any action from a user's menu -- or clicking inside a dialog it opened -- also opened the user drawer behind it. Test servers raced for their port, failing whole test classes with `Address already in use` on a loaded runner (this is what turned `test (Spark 4.1.0)` red on `main` after #17). From 2ae92c39d8a7cb984194f291c12c8f9d26643149 Mon Sep 17 00:00:00 2001 From: huyang Date: Thu, 24 Sep 2026 07:41:12 +0000 Subject: [PATCH 3/4] fix: update the CLI's deleteUser call to the four-argument signature The `purge` / `reassign_to` / `confirm_principal` parameters changed `UsersApi.deleteUser`, and the CLI still called the one-argument form, so the `cli` module did not compile. Passing nulls keeps what `uc user delete` has always done -- deactivate. It went unnoticed because the GitLab pipeline builds and tests `server` alone; GitHub builds every module, which is where it surfaced. Co-Authored-By: Claude Opus 5 (1M context) --- examples/cli/src/main/java/io/unitycatalog/cli/UserCli.java | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/examples/cli/src/main/java/io/unitycatalog/cli/UserCli.java b/examples/cli/src/main/java/io/unitycatalog/cli/UserCli.java index 40a11335..af4f1959 100644 --- a/examples/cli/src/main/java/io/unitycatalog/cli/UserCli.java +++ b/examples/cli/src/main/java/io/unitycatalog/cli/UserCli.java @@ -136,7 +136,10 @@ private static String getUser(UsersApi usersApi, JSONObject json) private static String deleteUser(UsersApi usersApi, JSONObject json) throws ApiException { String id = json.getString(CliParams.ID.getServerParam()); - usersApi.deleteUser(id); + // Deactivation, which is what this command has always done. The remaining arguments are + // purge / reassign_to / confirm_principal: purging is irreversible and needs the principal + // typed back, so it stays out of a positional CLI argument for now. + usersApi.deleteUser(id, null, null, null); return EMPTY; } From d3c4c83802b3c25e0954a0e834f204bae92dab72 Mon Sep 17 00:00:00 2001 From: huyang Date: Thu, 24 Sep 2026 08:00:46 +0000 Subject: [PATCH 4/4] fix: release a half-started test server before retrying its port The port retry added alongside this series builds a new server and binds it, but a start that failed partway was left as it was. Anything it had already bound would keep listening, and keep answering requests meant for the server the test actually ends up using. Stop it before retrying. Nothing in CI has taken this path yet -- the retry has not fired since it landed -- which is the point: it should not be the first thing to go wrong when it does. Co-Authored-By: Claude Opus 5 (1M context) --- .../java/io/unitycatalog/server/base/BaseServerTest.java | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/server/src/test/java/io/unitycatalog/server/base/BaseServerTest.java b/server/src/test/java/io/unitycatalog/server/base/BaseServerTest.java index a961cae4..82490c0a 100644 --- a/server/src/test/java/io/unitycatalog/server/base/BaseServerTest.java +++ b/server/src/test/java/io/unitycatalog/server/base/BaseServerTest.java @@ -118,6 +118,13 @@ private void startOnAFreePort(int firstAttemptPort, ServerProperties properties) serverConfig.setServerUrl("http://localhost:" + port); return; } catch (RuntimeException e) { + // Whatever did bind has to be released before trying again: a half-started server left + // listening would still answer requests aimed at the one this test ends up using. + try { + server.stop(); + } catch (RuntimeException ignored) { + // Nothing started, or it is already down; the original failure is the one that matters. + } if (attempt == START_ATTEMPTS || !isAddressInUse(e)) { throw e; }