Changed concept User to Admin#2
Conversation
There was a problem hiding this comment.
Pull request overview
This PR renames the administrative “User” concept to “Admin” across the auth server, updating API routes, middleware, database schema, tests, and documentation to reflect the new terminology.
Changes:
- Replace
/users/*CRUD + realm-membership endpoints with/admins/*equivalents and introduceadmin_endpoints.rs. - Rename auth middleware/extension extraction from
UserAuth+user_from_requesttoAdminAuth+admin_from_request. - Rename persisted DB structures from
user/user_realmstoadmin/admin_realmsacross SQLite/Postgres/MySQL implementations and update client/test/docs accordingly.
Reviewed changes
Copilot reviewed 29 out of 29 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| server/tests.md | Updates test plan section headings/endpoint paths for /admins (still contains outdated test names). |
| server/src/tests/super_admin_api.rs | Switches tests to Admin model and *admin* client calls (helper naming still says test_user). |
| server/src/tests/mod.rs | Renames test module from user_api to admin_api. |
| server/src/tests/admin_api.rs | Renames integration tests and client calls to /admins (some comments/messages still reference User/UserAuth). |
| server/src/server/endpoints/user_endpoints.rs | Removes legacy /users endpoint implementation. |
| server/src/server/endpoints/totp_endpoints.rs | Uses admin_from_request for realm-admin authorization checks. |
| server/src/server/endpoints/super_admins_endpoints.rs | Uses admin_from_request and updates log/error wording to admin terminology. |
| server/src/server/endpoints/realms_endpoints.rs | Uses admin_from_request for credential-management authorization. |
| server/src/server/endpoints/mod.rs | Exposes admin_endpoints and adds admin_from_request helper returning Admin. |
| server/src/server/endpoints/admin_endpoints.rs | New /admins CRUD + realm membership endpoints with exclusive-ownership checks. |
| server/src/server/auth_server.rs | Rewires scopes from /users to /admins and middleware from UserAuth to AdminAuth. |
| server/src/middleware/mod.rs | Renames exported middleware module to admin_auth. |
| server/src/middleware/admin_auth.rs | Renames middleware to load/inject Admin via find_admins_by_auth_scheme. |
| server/src/lib.rs | Re-exports Admin from models and removes User export. |
| server/src/database/trait.rs | Renames trait CRUD methods and lookup to Admin equivalents; bootstrap creates seeded admin via create_admin. |
| server/src/database/impls/sqlite.rs | Renames tables/joins/queries to admin + admin_realms (no migration path included). |
| server/src/database/impls/postgres.rs | Same as sqlite for Postgres schema and queries. |
| server/src/database/impls/mysql.rs | Same as sqlite for MySQL schema and queries. |
| server/documentation/two_factor_authentication.md | Updates terminology and code snippets from User to Admin. |
| server/documentation/index.md | Updates glossary/terminology to Admin (minor grammar issue remains). |
| server/documentation/client_library.md | Updates client guide to Admin APIs (TOC link + sample code issue remains). |
| server/documentation/authorization_and_administration.md | Updates authorization model docs to Admin (diagram still references UserAuth/User.realms). |
| server/documentation/authentication_flows.md | Updates terminology to Admin (diagrams still reference UserAuth/find_users_by_auth_scheme). |
| server/documentation/api_reference.md | Renames “User Administration” to “Admin Administration” and updates paths (role/status-code details still inconsistent with implementation). |
| server/authorization.md | Updates terminology to Admin (still references find_users_by_auth_scheme/user_from_request). |
| client/src/models/mod.rs | Exports Admin instead of User. |
| client/src/models/base.rs | Renames User struct to Admin and updates helper methods. |
| client/src/lib.rs | Re-exports Admin in public API. |
| client/src/client/auth_client.rs | Renames user-management methods/endpoints to admin equivalents (/admins/*). |
Comments suppressed due to low confidence (4)
server/documentation/authorization_and_administration.md:91
- The authorization decision-flow diagram still references
UserAuth middlewareandUser.realms, but the implementation has been renamed toAdminAuthandAdmin.realms. Please update the mermaid diagram text so it matches the current types and middleware.
```mermaid
flowchart TD
A[Incoming request] --> B{Authenticated?\nUserAuth middleware}
B -- No --> Z[HTTP 401 Unauthorized]
B -- Yes --> C{Endpoint category}
C -- "Super-admin-only\n/admin/realm POST|PUT|DELETE\n/admins GET\n/admin/userpass GET" --> D{is_super_admin?}
D -- No --> E[HTTP 403 Forbidden]
D -- Yes --> F[Proceed]
C -- "Realm-scoped\n/realms/{realm}/…\n/admin/realm/{id} GET\n/sessions/…" --> G{"can_administer_realm\n(realm)?"}
G -- No --> E
G -- Yes --> F
C -- "Admin CRUD\n/admins/admin POST|GET|PUT|DELETE" --> SA{is_super_admin?}
SA -- Yes --> F
SA -- No --> OWN{"target.realms non-empty\nAND all realms in target\nadministered by requester?"}
OWN -- No --> E
OWN -- Yes --> PUTCHECK{"PUT only:\nbody.realms non-empty\nAND all new realms\nadministered by requester?"}
PUTCHECK -- No --> E
PUTCHECK -- Yes --> F
C -- "GET /admin/realms" --> H{is_super_admin?}
H -- Yes --> I[Return all realms]
H -- No --> J["Return only realms in\nUser.realms list"]
**server/src/tests/admin_api.rs:296**
* This section still refers to `/users/*` endpoints and an unauthenticated `list_users` call, but the test now exercises `/admins` via `list_admins_as_super_admin()`. Please update the docstring and assertion message to match the renamed endpoints/methods, otherwise failures will be confusing to interpret.
**server/src/tests/admin_api.rs:1147**
* This test’s comments still describe `UserAuth`/`find_users_by_auth_scheme` and `/users` endpoints, but the server now uses `AdminAuth`/`find_admins_by_auth_scheme` and `/admins`. Please update these comments to reflect the renamed middleware and routes so the security-property being asserted remains clear.
**server/src/tests/super_admin_api.rs:40**
* The helper is still named `test_user` but now constructs an `Admin`. Renaming it to `test_admin` (and updating nearby comments that say “User record”) would reduce confusion, especially since there is also a separate `admin_api` test module that already uses `test_admin`.
fn test_user(id: &str) -> Admin {
Admin {
id: id.to_string(),
realms: vec![],
userpass: None,
jwt: None,
fido2: None,
digital_credentials: None,
client_certificate: None,
totp_enabled: None,
totp_secret: None,
totp_auth_url: None,
}
</details>
---
💡 <a href="/Cosmian/authentication/new/main?filename=.github/instructions/*.instructions.md" class="Link--inTextBlock" target="_blank" rel="noopener noreferrer">Add Copilot custom instructions</a> for smarter, more guided reviews. <a href="https://docs.github.com/en/copilot/customizing-copilot/adding-repository-custom-instructions-for-github-copilot" class="Link--inTextBlock" target="_blank" rel="noopener noreferrer">Learn how to get started</a>.
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
Agent-Logs-Url: https://github.com/Cosmian/authentication/sessions/5bb7aaa9-c013-48cf-b449-f89707cdb61f Co-authored-by: charming-wicket-5502 <[email protected]>
| description = "API Server" | ||
|
|
||
| [features] | ||
| default = ["openssl", "database"] |
There was a problem hiding this comment.
Build the admin-ui by default.
default = ["openssl", "database", "admin-ui"]
| @@ -1,2 +1,2 @@ | |||
| [workspace.package] | |||
| version = "1.0.0" | |||
There was a problem hiding this comment.
I would rather keep it in 0.1.0 the time we are doing tests on it. It must have other 1.0.0 versions to be changed too.
| version = "0.1.0" |
Also, IMHO the UI package.json should be align with the workspace Cargo.toml version
| } | ||
| onSuccess(); | ||
| } catch { | ||
| // validation errors shown inline |
There was a problem hiding this comment.
Api errors are not handled on the form.
Also I don't see the messages (success) when testing in the UI
fmontaigne
left a comment
There was a problem hiding this comment.
Suggestions
- Use tanstack query or similar to isolate API interaction logic and handle loading and error states
- Be careful when using the "&&"" shortcut for conditional rendering as it can lead to strange renders in bad cases. Easy workaround it to cast the condition to a boolean with !! for example
- Current tests are broken on my local installation at least (18 failed)
- Some MD files probably needs to be removed or cleaned
| <Space direction="vertical" className="w-full mb-4"> | ||
| <div className="p-4 bg-white rounded text-center"> | ||
| <img | ||
| src={`https://api.qrserver.com/v1/create-qr-code/?size=200x200&data=${encodeURIComponent(totpData.otpauth_url)}`} |
There was a problem hiding this comment.
Call to an external server to display the QR code of the TOPT secret. I guess it can cause security issues / unavailability issues. We should use a generator lib
| await onSubmit(passwordBytes); | ||
| form.resetFields(); | ||
| } catch { | ||
| // validation errors are shown inline |
| zIndex: 900, | ||
| backgroundColor: isDarkMode ? "#3b0a0a" : "#fff1f0", | ||
| borderColor: isDarkMode ? "#ff4d4f" : "#ff675f", | ||
| color: isDarkMode ? "#ffb3b0" : "#a8071a", |
| api.list(realmId) | ||
| .then(setCredentials) | ||
| .catch(() => { | ||
| /* shown inline below */ |
| const handleResetPassword = async (password: number[]) => { | ||
| if (!resetTarget) return; | ||
| const api = createCredentialsApi(serverUrl); | ||
| await api.update(realmId, resetTarget.username, { ...resetTarget, password }); |
| itemName={realmLabel(selectedRealm)} | ||
| onConfirm={handleRevokeAll} | ||
| onCancel={() => setRevokeModalOpen(false)} | ||
| loading={false} |
There was a problem hiding this comment.
Should be removed or plugged in to the loading state
| return handleResponse<T>(response); | ||
| } | ||
|
|
||
| export async function apiDelete(baseUrl: string, path: string): Promise<void> { |
| export const realmTotpVerifyPath = (realmId: string): string => `/realms/${encodeURIComponent(realmId)}/totp/verify`; | ||
| export const realmTotpUserPath = (realmId: string, username: string): string => | ||
| `/realms/${encodeURIComponent(realmId)}/totp/${encodeURIComponent(username)}`; | ||
|
|
There was a problem hiding this comment.
Several exported paths are not used
| } | ||
|
|
||
| /** Session data */ | ||
| export interface SessionData { |
|
|
||
| tokens: { | ||
| light: { | ||
| colorPrimary: "#e34319", |
There was a problem hiding this comment.
Default colors must be in one place only, some are also in the Theme. This should be merged and exported
No description provided.