feat: extract Keycloak roles + role-based post-login routing #105
No reviewers
Labels
No labels
domain:backend
domain:devops
domain:frontend
No milestone
No project
No assignees
1 participant
Due date
No due date set.
Dependencies
No dependencies set.
Reference
ldraney/pal-e-app!105
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "102-role-extraction-routing"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Summary
Extract Keycloak token claims (roles, email, sub) into reusable helpers and a reactive user identity store. Add post-login redirect routing: admin users go to /dashboard, regular authenticated users go to /notes. Landing page stays public with no forced login.
Changes
src/lib/keycloak.ts-- Added getUserRoles(), hasRole(), getUserEmail(), getUserSub(), getPrimaryRole(), getRoleRedirectPath(), getTokenParsed(). Updated module docstring for pal-e-app naming.src/lib/stores/user.svelte.ts-- New reactive Svelte 5 store with syncFromKeycloak(), getIdentity(), hasRole(), hasAnyRole(). Typed UserIdentity interface.src/routes/+layout.svelte-- Import new keycloak helpers + user store. Sync store on init. Post-login redirect from "/" based on role (admin -> /dashboard, user -> /notes). Uses replaceState to avoid back-button loops.Test Plan
npm run buildpasses clean (verified)Review Checklist
npm run build)Related Notes
None.
Related
Closes #102
PR #105 Review
DOMAIN REVIEW
Stack: SvelteKit / TypeScript / Keycloak OIDC (keycloak-js)
Keycloak token handling: The role extraction from
realm_access.rolesis correct. Null checks are thorough --tokenParsedguard, optional chaining onrealm_access, fallback to empty array. Theascasts are the standard pattern for keycloak-js sinceKeycloakTokenParseddoes not strongly type custom claims.Svelte 5 store pattern:
user.svelte.tsuses$stateat module scope correctly for Svelte 5 runes. ThesyncFromKeycloak()write-gate pattern is clean. Interface is well-typed.Redirect logic: Only fires on
pathname === '/', usesreplaceState: trueto avoid back-button loops. Does not intercept deep links. Correct approach.Route targets verified: Both
/dashboard/+page.svelteand/notes/+page.svelteexist in the repo. Redirects will not 404.No forced login regression: The
initKeycloak()call is unchanged -- noonLoadparameter added, so the landing page remains public. Correct.BLOCKERS
1. No test coverage for new functionality.
This PR adds 7 new exported functions in
keycloak.ts, an entire new store module (user.svelte.ts), and redirect logic in the layout. There are zero unit tests for any of it. The repo has E2E tests (e2e/auth.spec.ts) but no unit tests for keycloak helpers or the user store. The test plan lists only manual verification steps andnpm run build.At minimum, the pure functions (
getUserRoles,hasRole,getPrimaryRole,getRoleRedirectPath,syncFromKeycloak,getIdentity,hasRole,hasAnyRole) are trivially unit-testable with a mockedkeycloak.tokenParsed. This is a BLOCKER per the review criteria: "New functionality with zero test coverage."NITS
1. DRY: duplicated role extraction logic.
Role parsing from
realm_accessis implemented twice with identical logic:keycloak.ts:getUserRoles()lines 107-112user.svelte.ts:syncFromKeycloak()line 56:(tokenParsed.realm_access as { roles?: string[] })?.roles ?? []The store should call
getUserRoles()(or better, the store should be the single owner of parsed identity, andkeycloak.tsshould not duplicate it). Not blocking since this is not an auth/security path divergence risk (both read the same token), but it will drift if one is updated without the other.2. Store not synced on token refresh.
syncFromKeycloak()is called once on mount. The existingonTokenExpiredhandler inkeycloak.ts(line 35-39) refreshes the token but does not re-sync the store. If a user's roles change in Keycloak mid-session, the store will hold stale data until page reload. Consider hookingsyncFromKeycloakinto the token refresh callback.3.
clientIddefault still sayspal-e-docs-app.The PR updates the docstring comment from
pal-e-docs-apptopal-e-app, butkeycloak.tsline 13 still hasclientId: ... || 'pal-e-docs-app'as the fallback. This is out of scope for this PR (it is a pre-existing issue), but worth noting since the docstring update creates a contradiction. Consider a follow-up ticket.4.
getIdentity()returns the mutable reactive object.getIdentity()returns theidentityreference directly. In Svelte 5, this is fine for reactivity (consumers reading properties will track), but the docstring says "Read-only access" while nothing preventsgetIdentity().roles.push('admin'). A shallow spread orObject.freezewould enforce the contract. Minor.5.
hasRolename collision.Both
keycloak.tsanduser.svelte.tsexport a function namedhasRole. Consumers must be careful about which import they use. The keycloak version reads the token directly; the store version reads cached state. Consider naming onekeycloakHasRoleor removing the duplication.SOP COMPLIANCE
102-role-extraction-routingmatches issue #102PROCESS OBSERVATIONS
src/lib/modules. Adding vitest (or similar) for pure function testing would significantly reduce change failure rate for auth-adjacent code. The keycloak helpers and user store are ideal first candidates.VERDICT: NOT APPROVED
One blocker: zero test coverage for new functionality. The 7 new keycloak helpers and the entire user store module are pure functions that are trivially unit-testable. Add tests covering at minimum:
getUserRoleswith/withoutrealm_access,getPrimaryRolefor admin/user/unauthenticated,getRoleRedirectPathmapping, andsyncFromKeycloakwith valid/invalid/missing token data.Addressed all QA findings in commit
d7a8f39:Blocker fixed:
src/lib/keycloak.test.tswith 26 unit tests covering all 7 new exported functions plusisAuthenticatedandgetUserName.Nits fixed:
getUserRoles()from keycloak.ts instead of duplicatingrealm_accessparsing.onTokenRefresh()callback in keycloak.ts, wired in +layout.svelte sosyncFromKeycloak()re-runs on every token refresh.pal-e-docs-apptopal-e-appto match the docstring and repo name.hasRoletouserHasRole(andhasAnyRoletouserHasAnyRole) to disambiguate from keycloak.tshasRole.Build passes. 26/26 tests pass.
PR #105 Review
DOMAIN REVIEW
Tech stack: SvelteKit 5 / TypeScript / Keycloak OIDC / Vitest
keycloak.ts -- Clean additions. Seven new exported functions for role extraction, routing, and token access. The role hierarchy (
admin > user) is simple and correct.getUserRoles()safely handles missingrealm_accesswith multiple fallback paths.onTokenRefreshcallback pattern is well-designed for store sync. TheclientIddefault changed frompal-e-docs-apptopal-e-app-- consistent with the repo rename (issue #98).user.svelte.ts -- Good use of Svelte 5
$statefor reactive identity. Delegates role extraction tokeycloak.ts(no DRY violation).syncFromKeycloak()properly resets to defaults when unauthenticated. TheuserHasRole/userHasAnyRolenaming avoids collision withkeycloak.ts hasRole-- documented in comments.+layout.svelte -- Post-login redirect is guarded correctly: only fires when
authenticated && page.url.pathname === '/'. UsesreplaceState: trueto prevent back-button loops. Deep-link navigation is not intercepted. Token refresh callback registered to keep store in sync.keycloak.test.ts -- 26 tests covering all new helpers. Mock strategy is sound: mocks
keycloak-jsconstructor, controlstokenParsedandauthenticatedper test. Good edge case coverage: no token, empty realm_access, missing roles key.vite.config.ts -- Imports from
vitest/configinstead ofvite, correct for vitest integration.package.json --
testscript now runstest:unit(vitest).vitest@^4.1.4andjsdom@^29.0.2added as devDependencies. No production dependency changes beyond what was already there.BLOCKERS
None.
The new functionality (role extraction helpers, user store, post-login redirect) has 26 unit tests covering happy path, edge cases, and error handling. No unvalidated user input -- all data comes from Keycloak token claims (server-signed JWTs). No secrets or credentials in code. No duplicated auth logic.
NITS
FAB button uses Tailwind classes (
fixed bottom-6 right-6 z-40 flex h-14 w-14 ...) -- this predates this PR (exists on main), but perfeedback_no_tailwind.mdconvention, these should eventually be replaced with pure CSS vars. Not introduced by this PR, so not blocking.Unit tests not wired in CI -- The
.woodpecker.yamlteststep runsnpx playwright test(E2E), notnpm run test:unit. The 26 new vitest tests will only run locally, not in the pipeline. This is a pre-existing CI gap (not introduced by this PR) but worth tracking as a follow-up issue so the tests don't silently rot.Module docstring update -- The keycloak.ts header comment updated
pal-e-docs-apptopal-e-appin the description and redirect URI. Good. Minor: the redirect URI comment sayspal-e-app.tail5b443a.ts.net/*but the actual Keycloak client may still be registered aspal-e-docs-appif the rename hasn't propagated to Keycloak config. Worth verifying during validation.getTokenParsed()return type -- ReturnsRecord<string, unknown> | undefined. This is fine for the store sync use case, but a typedKeycloakTokenParsedinterface would improve safety if more consumers appear later. Low priority.No test for
user.svelte.ts-- The store module is tested indirectly throughkeycloak.test.ts(which covers the functions the store delegates to), butsyncFromKeycloak(),getIdentity(),userHasRole(), anduserHasAnyRole()have no direct unit tests. The Svelte 5$statereactive primitive requires Svelte compilation in the test environment, which may have been the barrier. Not a blocker given the store is thin delegation, but worth noting.SOP COMPLIANCE
102-role-extraction-routingfollows{issue-number}-{kebab-case-purpose}PROCESS OBSERVATIONS
/for authenticated users. No existing behavior is broken for unauthenticated users or deep-link navigation.test:unitstep to.woodpecker.yamlbefore the test suite grows further. Tests that only run locally tend to diverge from reality./dashboard, (c) unauthenticated landing page still loads without login prompt.VERDICT: APPROVED