Skip to content

Commit 609bcdd

Browse files
hendrikebbershendrikebbers
authored
Merge pull request #4 from OpenElementsLabs/fix/001-canonical-role-constants
fix(roles): align role constants to canonical OIDC names
2 parents 173373b + 860ccbf commit 609bcdd

10 files changed

Lines changed: 378 additions & 13 deletions

File tree

‎README.md‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,7 @@ pnpm add @open-elements/ui next next-auth react react-dom lucide-react
2525

2626
### `@open-elements/nextjs-app-layer` (client-safe)
2727

28-
- `ROLE_ADMIN`, `ROLE_IT_ADMIN`, `hasRole(session, role)`
28+
- `ROLE_APP_ADMIN`, `ROLE_APP_USER`, `ROLE_IT_ADMIN`, `hasRole(session, role)`
2929
- `ForbiddenError`
3030
- DTO types: `Page<T>`, `UserDto`, `AuditAction`, `AuditLogDto`,
3131
`ApiKeyDto`, `ApiKeyCreateDto`, `ApiKeyCreatedDto`, `WebhookDto`,
@@ -153,7 +153,7 @@ The same 2-line shape applies to every other admin page.
153153

154154
## OE conventions this lib assumes
155155

156-
- OIDC role names: `IT-ADMIN` and `ADMIN` (hardcoded).
156+
- OIDC role names: `APP-ADMIN`, `APP-USER`, and `IT-ADMIN` (hardcoded).
157157
- Proxy pattern: every backend call goes through `/api/...` in the same
158158
origin as the Next.js app.
159159
- Fonts: Montserrat (heading), Lato (body).

‎docs/TODO.md‎

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,15 @@
1+
# TODO
2+
3+
## Test the OIDC session claim mapping in `auth.ts`
4+
5+
Add tests for the NextAuth `session` callback in `src/server/auth.ts`: the
6+
`roles` claim should be carried through to `session.roles` 1:1, and a missing
7+
claim should yield `[]` (not `undefined`). No auth test file exists today, so
8+
these invariants are unverified.
9+
10+
**Context:** Surfaced during `/spec-review` of spec `001-canonical-role-constants`.
11+
The two "Session claim mapping" behavior scenarios are unchanged by that fix and
12+
were left untested (out of scope for the bug fix). Testing requires extracting
13+
the callback or mocking NextAuth, which is disproportionate for that spec.
14+
15+
**Prerequisite:** None.

‎docs/specs/001-canonical-role-constants/behaviors.md‎

Lines changed: 108 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,108 @@
1+
# Behaviors: Canonical role constants
2+
3+
## Role constant values
4+
5+
### Admin role constant holds the canonical value
6+
7+
- **Given** the `@open-elements/nextjs-app-layer` public API
8+
- **When** `ROLE_APP_ADMIN` is read
9+
- **Then** it equals the string `"APP-ADMIN"`
10+
11+
### App-user role constant is exported
12+
13+
- **Given** the public API
14+
- **When** `ROLE_APP_USER` is read
15+
- **Then** it equals the string `"APP-USER"`
16+
17+
### IT-admin role constant is unchanged
18+
19+
- **Given** the public API
20+
- **When** `ROLE_IT_ADMIN` is read
21+
- **Then** it equals the string `"IT-ADMIN"`
22+
23+
### Old admin constant name is no longer exported
24+
25+
- **Given** the public API after this change
26+
- **When** a consumer imports `ROLE_ADMIN`
27+
- **Then** the symbol does not exist (compile-time error / `undefined` at
28+
runtime), forcing migration to `ROLE_APP_ADMIN`
29+
30+
## hasRole — happy paths
31+
32+
### Real admin passes the admin check
33+
34+
- **Given** a session with `roles = ["APP-ADMIN", "APP-USER", "IT-ADMIN"]`
35+
- **When** `hasRole(session, ROLE_APP_ADMIN)` is called
36+
- **Then** it returns `true`
37+
38+
### App user passes the app-user check
39+
40+
- **Given** a session with `roles = ["APP-USER"]`
41+
- **When** `hasRole(session, ROLE_APP_USER)` is called
42+
- **Then** it returns `true`
43+
44+
### IT admin passes the IT-admin check
45+
46+
- **Given** a session with `roles = ["IT-ADMIN"]`
47+
- **When** `hasRole(session, ROLE_IT_ADMIN)` is called
48+
- **Then** it returns `true`
49+
50+
## hasRole — negative and edge cases
51+
52+
### User without the role fails the check
53+
54+
- **Given** a session with `roles = ["APP-USER"]`
55+
- **When** `hasRole(session, ROLE_APP_ADMIN)` is called
56+
- **Then** it returns `false`
57+
58+
### Legacy "ADMIN" claim no longer grants admin (intentional breakage)
59+
60+
- **Given** a session with `roles = ["ADMIN"]` (the old, pre-fix claim value)
61+
- **When** `hasRole(session, ROLE_APP_ADMIN)` is called
62+
- **Then** it returns `false`, because the match is exact and `"ADMIN"` is not
63+
the canonical `"APP-ADMIN"`
64+
65+
### Matching is case-sensitive
66+
67+
- **Given** a session with `roles = ["app-admin"]`
68+
- **When** `hasRole(session, ROLE_APP_ADMIN)` is called
69+
- **Then** it returns `false` (exact, case-sensitive match is preserved)
70+
71+
### Empty roles array
72+
73+
- **Given** a session with `roles = []`
74+
- **When** `hasRole(session, ROLE_APP_ADMIN)` is called
75+
- **Then** it returns `false`
76+
77+
### Null session
78+
79+
- **Given** a `null` session
80+
- **When** `hasRole(null, ROLE_APP_ADMIN)` is called
81+
- **Then** it returns `false`
82+
83+
### Undefined session
84+
85+
- **Given** an `undefined` session
86+
- **When** `hasRole(undefined, ROLE_APP_ADMIN)` is called
87+
- **Then** it returns `false`
88+
89+
### Session with no roles property
90+
91+
- **Given** a session object where `roles` is `undefined`
92+
- **When** `hasRole(session, ROLE_APP_ADMIN)` is called
93+
- **Then** it returns `false` (no throw)
94+
95+
## Session claim mapping (unchanged, regression guard)
96+
97+
### OIDC roles claim is carried through 1:1
98+
99+
- **Given** an OIDC `roles` claim of `["APP-ADMIN", "APP-USER", "IT-ADMIN"]`
100+
- **When** the session is built by `createAppLayerAuth`
101+
- **Then** `session.roles` equals `["APP-ADMIN", "APP-USER", "IT-ADMIN"]`
102+
(values passed through without transformation)
103+
104+
### Missing roles claim yields an empty array
105+
106+
- **Given** an OIDC profile with no `roles` claim
107+
- **When** the session is built
108+
- **Then** `session.roles` equals `[]` (not `undefined`)

‎docs/specs/001-canonical-role-constants/design.md‎

Lines changed: 148 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,148 @@
1+
# Design: Canonical role constants
2+
3+
## GitHub Issue
4+
5+
[#3 — ROLE_ADMIN constant is "ADMIN" but the canonical app-admin role is "APP-ADMIN" (frontend/backend mismatch)](https://github.com/OpenElementsLabs/nextjs-app-layer/issues/3)
6+
7+
## Summary
8+
9+
The role constants exported from `src/lib/roles.ts` do not match the canonical
10+
role names used across the Open Elements ecosystem. `ROLE_ADMIN` resolves to
11+
the string `"ADMIN"`, but the admin role issued by the OIDC provider and
12+
enforced by the backend is `"APP-ADMIN"` (backend authority `ROLE_APP-ADMIN`
13+
via the shared `spring-services` `@RequiresAppAdmin`).
14+
15+
Because `hasRole()` performs an exact, case-sensitive match, any downstream app
16+
calling `hasRole(session, ROLE_ADMIN)` to gate admin UI gets `false` even for
17+
real admins — admin-only controls stay hidden. This fixes the constant values,
18+
aligns the constant naming to a consistent `APP_`/`IT_` prefix scheme, and adds
19+
the missing `APP-USER` role constant. It is a **breaking change** to the public
20+
API and warrants a version bump.
21+
22+
## Reproduction
23+
24+
- **Given** an OIDC provider that issues the roles claim `["APP-ADMIN", "APP-USER", "IT-ADMIN"]`
25+
- **And** a downstream app that gates admin UI with `hasRole(session, ROLE_ADMIN)`
26+
- **When** a real admin (holding `APP-ADMIN`) signs in
27+
- **Then** `session.roles` contains `"APP-ADMIN"` (copied 1:1 from the JWT), but
28+
`ROLE_ADMIN === "ADMIN"`, so `session.roles.includes("ADMIN")` is `false`
29+
- **Result:** admin-only UI (e.g. delete buttons) stays disabled/hidden even
30+
though the backend correctly authorizes the same user.
31+
32+
## Root cause analysis
33+
34+
The bug is a **wrong constant value**, not a logic error.
35+
36+
- `src/lib/roles.ts` declares `export const ROLE_ADMIN = "ADMIN";` — the string
37+
literal `"ADMIN"` never matches the canonical `"APP-ADMIN"` role name.
38+
- `hasRole()` is correct: `session?.roles?.includes(role)` is an exact match,
39+
and roles line up exactly once the constant holds the right value. No
40+
normalization is needed.
41+
- `src/server/auth.ts` copies the OIDC `roles` claim into `session.roles`
42+
unchanged — also correct; the session carries the true role strings.
43+
44+
The frontend was simply the outlier: the IdP, `session.roles`, and the backend
45+
all agree on `"APP-ADMIN"`; only this library's constant disagreed.
46+
47+
Note: `ROLE_ADMIN` is **not used internally** anywhere in this library — every
48+
admin page (`users`, `audit-logs`, `status`, `token`, `api-keys`, `webhooks`)
49+
gates on `ROLE_IT_ADMIN`. `ROLE_ADMIN` is only re-exported from `src/index.ts`
50+
for downstream consumers, so the practical breakage is entirely downstream.
51+
52+
## Fix approach
53+
54+
Correct the canonical role strings and align the constant names to a consistent
55+
prefix scheme. `ROLE_IT_ADMIN` already held the correct value `"IT-ADMIN"` and
56+
keeps it.
57+
58+
`src/lib/roles.ts`:
59+
60+
```ts
61+
export const ROLE_APP_ADMIN = "APP-ADMIN";
62+
export const ROLE_APP_USER = "APP-USER";
63+
export const ROLE_IT_ADMIN = "IT-ADMIN";
64+
65+
export function hasRole(session: Session | null | undefined, role: string): boolean {
66+
return !!session?.roles?.includes(role);
67+
}
68+
```
69+
70+
Changes:
71+
72+
1. **Rename** `ROLE_ADMIN` → `ROLE_APP_ADMIN` and **fix its value** from
73+
`"ADMIN"` to `"APP-ADMIN"`.
74+
2. **Add** `ROLE_APP_USER = "APP-USER"` (the third supported role, previously
75+
missing).
76+
3. **Keep** `ROLE_IT_ADMIN = "IT-ADMIN"` unchanged.
77+
4. **Keep** `hasRole()` as an exact, case-sensitive `includes()` match. Roles
78+
now match the claim exactly, so no normalization is introduced (it would
79+
only mask future mismatches).
80+
81+
Update the public export barrel `src/index.ts`:
82+
83+
```ts
84+
export { ROLE_APP_ADMIN, ROLE_APP_USER, ROLE_IT_ADMIN, hasRole } from "./lib/roles";
85+
```
86+
87+
Update the test `src/lib/__tests__/roles.test.ts` to assert the new names and
88+
values, and add coverage for `ROLE_APP_USER`.
89+
90+
Update documentation in `README.md`:
91+
92+
- The exported-symbols list (currently `ROLE_ADMIN, ROLE_IT_ADMIN, hasRole`).
93+
- The "OE conventions this lib assumes" line (currently
94+
`OIDC role names: IT-ADMIN and ADMIN (hardcoded)`) to reflect
95+
`APP-ADMIN`, `APP-USER`, and `IT-ADMIN`.
96+
97+
**Rationale for the naming decision:** since the fix is already a breaking
98+
change (the `ROLE_ADMIN` value changes), it is the right moment to also rename
99+
the constant so all three exports follow the same `<AREA>_<ROLE>` convention
100+
(`ROLE_APP_ADMIN`, `ROLE_APP_USER`, `ROLE_IT_ADMIN`). This mirrors the actual
101+
role strings (`APP-`/`IT-` prefixes) and removes the mismatch between a bare
102+
`ROLE_ADMIN` name and an `"APP-ADMIN"` value.
103+
104+
**Rationale for keeping `hasRole()` exact:** the OIDC IdP, `session.roles`, and
105+
the backend all use the exact canonical strings. An exact match keeps the
106+
contract simple and surfaces any real future mismatch loudly instead of
107+
silently coercing it.
108+
109+
## Versioning
110+
111+
This is a **breaking change** to the public API:
112+
113+
- `ROLE_ADMIN` is removed (renamed to `ROLE_APP_ADMIN`), so any consumer
114+
importing `ROLE_ADMIN` will fail to compile.
115+
- The value of the admin role constant changes from `"ADMIN"` to `"APP-ADMIN"`,
116+
breaking anyone who still issues an `"ADMIN"` claim to match the old value.
117+
118+
Recommendation: bump the package from `0.5.0` to **`0.6.0`** (minor bump on a
119+
pre-1.0 line signals a breaking change per this project's convention) and add a
120+
clear changelog / release note describing the rename and the new value, with a
121+
migration hint (`ROLE_ADMIN` → `ROLE_APP_ADMIN`, ensure the IdP issues
122+
`APP-ADMIN`). A release/upgrade note can be produced with `/release-doc`.
123+
124+
## Non-goals
125+
126+
- **Configurable / per-app role mapping** — remains deferred follow-up work (see
127+
README "Deferred follow-up work"). This spec keeps role names hardcoded.
128+
- **Changing `hasRole()` semantics** (case-insensitivity, prefix stripping,
129+
`ROLE_`-authority handling) — explicitly out of scope.
130+
- **Transforming the roles claim in `src/server/auth.ts`** — the claim is
131+
already carried through correctly.
132+
133+
## Regression risk
134+
135+
- **Internal:** low. `ROLE_ADMIN` is unused internally; admin pages use
136+
`ROLE_IT_ADMIN`, which is unchanged. No internal call sites depend on the old
137+
admin value.
138+
- **Downstream:** intentional breakage. Consumers importing `ROLE_ADMIN` must
139+
rename to `ROLE_APP_ADMIN`; consumers relying on the `"ADMIN"` string must
140+
update their IdP role claim to `"APP-ADMIN"`. This is the point of the fix and
141+
is communicated via the version bump + changelog.
142+
- **Tests:** `roles.test.ts` hard-codes the old name/value and must be updated
143+
in the same change, or the build fails.
144+
145+
## Open questions
146+
147+
None. Supported roles confirmed via issue #3 comment: `APP-ADMIN`, `IT-ADMIN`,
148+
`APP-USER`. `IT-ADMIN` intentionally has no `APP-` prefix.

‎docs/specs/001-canonical-role-constants/steps.md‎

Lines changed: 57 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,57 @@
1+
# Implementation Steps: Canonical role constants
2+
3+
## Step 1: Fix role constants in `src/lib/roles.ts`
4+
5+
- [ ] Rename `ROLE_ADMIN` → `ROLE_APP_ADMIN` and set value to `"APP-ADMIN"`
6+
- [ ] Add `ROLE_APP_USER = "APP-USER"`
7+
- [ ] Keep `ROLE_IT_ADMIN = "IT-ADMIN"` and `hasRole()` exact-match unchanged
8+
9+
**Acceptance criteria:**
10+
- [ ] Constants hold canonical values
11+
- [ ] Project type-checks
12+
13+
**Related behaviors:** Role constant values
14+
15+
---
16+
17+
## Step 2: Update public export barrel `src/index.ts`
18+
19+
- [ ] Export `ROLE_APP_ADMIN, ROLE_APP_USER, ROLE_IT_ADMIN, hasRole`
20+
21+
**Acceptance criteria:**
22+
- [ ] No dangling `ROLE_ADMIN` export
23+
24+
**Related behaviors:** Old admin constant name is no longer exported
25+
26+
---
27+
28+
## Step 3: Update tests `src/lib/__tests__/roles.test.ts`
29+
30+
- [ ] Assert new names/values, add `ROLE_APP_USER` coverage
31+
- [ ] Add negative cases (legacy `"ADMIN"` claim, case-sensitivity)
32+
33+
**Acceptance criteria:**
34+
- [ ] `pnpm test` passes
35+
36+
**Related behaviors:** hasRole happy paths, negatives and edge cases
37+
38+
---
39+
40+
## Step 4: Update `README.md`
41+
42+
- [ ] Update exported-symbols list
43+
- [ ] Update "OE conventions" role-names line to `APP-ADMIN`, `APP-USER`, `IT-ADMIN`
44+
45+
**Acceptance criteria:**
46+
- [ ] Docs match code
47+
48+
**Related behaviors:** —
49+
50+
---
51+
52+
## Step 5: Version bump
53+
54+
- [ ] Bump `package.json` version `0.5.0` → `0.6.0` (breaking change)
55+
56+
**Acceptance criteria:**
57+
- [ ] Version reflects breaking change

‎docs/specs/INDEX.md‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
# Spec Index
2+
3+
| ID | Spec-Folder | Name | Areas | Description | GitHub Issue | Status |
4+
|-----|-------------|------|-------|-------------|--------------|--------|
5+
| 001 | 001-canonical-role-constants | Canonical role constants | frontend, authentication, api | Fix `ROLE_ADMIN` value to `APP-ADMIN`, rename to `ROLE_APP_ADMIN`, add `ROLE_APP_USER`; align role constants to canonical OIDC names (breaking) | #3 | done |

‎package.json‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
{
22
"name": "@open-elements/nextjs-app-layer",
3-
"version": "0.5.0",
3+
"version": "0.6.0",
44
"description": "Next.js foundation (auth, proxy, admin pages, login/forbidden, layout) for Open Elements applications",
55
"packageManager": "pnpm@11.3.0",
66
"engines": {

‎src/index.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@
22
// Server-only factories live in ./server.ts and are imported via
33
// `@open-elements/nextjs-app-layer/server`.
44

5-
export { ROLE_ADMIN, ROLE_IT_ADMIN, hasRole } from "./lib/roles";
5+
export { ROLE_APP_ADMIN, ROLE_APP_USER, ROLE_IT_ADMIN, hasRole } from "./lib/roles";
66
export { ForbiddenError } from "./lib/forbidden-error";
77

88
export {

0 commit comments

Comments
 (0)