Spike: Keycloak auth integration design #23

Merged

ldraney merged 2 commits from 16-keycloak-integration-spike into main 2026-06-08 03:16:32 +00:00

Conversation 4 Commits 2 Files changed 2 +396

ldraney commented 2026-06-07 16:33:05 +00:00

Owner

Copy link

Summary

• Spike deliverable for Keycloak OIDC integration into palinks, answering all five design questions from issue #16.
• Recommends omniauth-keycloak gem, designs four new tables (users, clicks, feature_flags, visibility column on links), documents Keycloak provisioning steps, and maps secrets wiring through pal-e-deployments.

Changes

• docs/auth.md: New consolidated auth design document covering OIDC gem evaluation, schema design with mermaid ER diagram, Keycloak realm/client provisioning (Terraform config), secrets wiring, controller integration sketches, and follow-up ticket breakdown.

Test Plan

• Review design doc for completeness against the five spike questions
• Verify Terraform config snippets match existing patterns in pal-e-services/terraform/k3s.tfvars
• Verify schema design aligns with existing architecture.md and visibility.md docs
• No code changes to verify — this is a docs-only spike

Review Checklist

• Passed automated review-fix loop
• No secrets committed
• No unnecessary file changes
• Commit messages are descriptive
• Feature flag needed? No — docs-only spike, no runtime changes

Related Notes

• ldraney/palinks #16 — Spike: Keycloak integration and role-based schema design
• sop-keycloak-client-creation — Platform SOP for Keycloak realm/client management

Closes #16

## Summary - Spike deliverable for Keycloak OIDC integration into palinks, answering all five design questions from issue #16. - Recommends `omniauth-keycloak` gem, designs four new tables (users, clicks, feature_flags, visibility column on links), documents Keycloak provisioning steps, and maps secrets wiring through pal-e-deployments. ## Changes - `docs/auth.md`: New consolidated auth design document covering OIDC gem evaluation, schema design with mermaid ER diagram, Keycloak realm/client provisioning (Terraform config), secrets wiring, controller integration sketches, and follow-up ticket breakdown. ## Test Plan - [ ] Review design doc for completeness against the five spike questions - [ ] Verify Terraform config snippets match existing patterns in `pal-e-services/terraform/k3s.tfvars` - [ ] Verify schema design aligns with existing `architecture.md` and `visibility.md` docs - [ ] No code changes to verify — this is a docs-only spike ## Review Checklist - [ ] Passed automated review-fix loop - [ ] No secrets committed - [ ] No unnecessary file changes - [ ] Commit messages are descriptive - [ ] Feature flag needed? No — docs-only spike, no runtime changes ## Related Notes - `ldraney/palinks #16` — Spike: Keycloak integration and role-based schema design - `sop-keycloak-client-creation` — Platform SOP for Keycloak realm/client management Closes #16

ldraney added 1 commit 2026-06-07 16:33:05 +00:00

Add Keycloak auth integration design doc ... d5490efcac

Spike output covering OIDC gem evaluation, schema design for users/clicks/feature_flags tables, Keycloak realm and client provisioning steps, secrets wiring through pal-e-deployments, and follow-up ticket breakdown.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

ldraney commented 2026-06-07 16:33:48 +00:00

Author

Owner

Copy link

Review

Scope: Docs-only spike. Single new file docs/auth.md (393 lines). No code changes, no runtime impact.

Findings

1. Minor -- feature_enabled? nil comparison (line 147)
The != nil check in user&.preferences&.dig("flags", flag_name) != nil will treat false as a valid override (correct), but the Ruby idiom !user.preferences.dig(...).nil? is more conventional. Not a bug since this is a design sketch, not shipped code.

2. Minor -- Logout does not revoke Keycloak session (line 357-359)
The SessionsController#destroy only calls reset_session, which clears the Rails session but leaves the Keycloak SSO session alive. A user who logs out and clicks "Log in" again will be silently re-authenticated without seeing the Keycloak login screen. The follow-up implementation ticket should note that RP-initiated logout (redirect to Keycloak's end_session_endpoint with post_logout_redirect_uri) is needed. The post_logout_redirect_uris are already configured in the client spec, so this is a matter of wiring the redirect in the controller.

3. Observation -- Consistency with existing docs
The schema in docs/auth.md aligns with the ER diagram in docs/architecture.md. The visibility model matches docs/visibility.md. The Keycloak provisioning config follows the patterns documented in sop-keycloak-client-creation (realm per project, confidential client, PKCE S256, direct access grants disabled). No contradictions found.

4. Observation -- All five spike questions answered

• OIDC gem: Recommended with tradeoff table.
• Schema: Four tables designed with mermaid ER diagram.
• Feature flags: Separate table with resolution logic.
• Keycloak provisioning: Realm, client, network policy, user creation steps.
• Secrets: Four env vars mapped with deployment patch YAML.

VERDICT: APPROVE

Clean spike doc. The two minor findings are design-sketch-level observations that should be addressed in the implementation tickets, not in this doc. No blockers.

## Review **Scope:** Docs-only spike. Single new file `docs/auth.md` (393 lines). No code changes, no runtime impact. ### Findings **1. Minor -- `feature_enabled?` nil comparison** (line 147) The `!= nil` check in `user&.preferences&.dig("flags", flag_name) != nil` will treat `false` as a valid override (correct), but the Ruby idiom `!user.preferences.dig(...).nil?` is more conventional. Not a bug since this is a design sketch, not shipped code. **2. Minor -- Logout does not revoke Keycloak session** (line 357-359) The `SessionsController#destroy` only calls `reset_session`, which clears the Rails session but leaves the Keycloak SSO session alive. A user who logs out and clicks "Log in" again will be silently re-authenticated without seeing the Keycloak login screen. The follow-up implementation ticket should note that RP-initiated logout (redirect to Keycloak's `end_session_endpoint` with `post_logout_redirect_uri`) is needed. The `post_logout_redirect_uris` are already configured in the client spec, so this is a matter of wiring the redirect in the controller. **3. Observation -- Consistency with existing docs** The schema in `docs/auth.md` aligns with the ER diagram in `docs/architecture.md`. The visibility model matches `docs/visibility.md`. The Keycloak provisioning config follows the patterns documented in `sop-keycloak-client-creation` (realm per project, confidential client, PKCE S256, direct access grants disabled). No contradictions found. **4. Observation -- All five spike questions answered** - OIDC gem: Recommended with tradeoff table. - Schema: Four tables designed with mermaid ER diagram. - Feature flags: Separate table with resolution logic. - Keycloak provisioning: Realm, client, network policy, user creation steps. - Secrets: Four env vars mapped with deployment patch YAML. ### VERDICT: APPROVE Clean spike doc. The two minor findings are design-sketch-level observations that should be addressed in the implementation tickets, not in this doc. No blockers.

ldraney commented 2026-06-08 01:40:56 +00:00

Author

Owner

Copy link

PR #23 Review

DOMAIN REVIEW

Tech stack: Ruby on Rails 8.1, PostgreSQL, Keycloak OIDC, Terraform/k8s (design doc references). This is a docs-only spike -- no runtime code changes. Review focuses on design document quality, consistency with existing docs, and correctness of proposed patterns.

Gem recommendation (omniauth-keycloak): Sound. The rationale for choosing it over Devise and bare openid_connect is well-articulated. The fallback path to omniauth_openid_connect if the gem is abandoned is a practical escape hatch. The inclusion of omniauth-rails_csrf_protection is correct -- OmniAuth 2.x mandates POST-only auth initiation.

Schema design: The four-table design (users, clicks, feature_flags, visibility column on links) is coherent. Key design decisions are well-reasoned:

• keycloak_sub as stable identifier over email -- correct
• String column over Postgres enum for visibility -- pragmatic for zero-downtime deploys
• Separate feature_flags table vs JSONB on users -- appropriate separation of concerns
• Safe default of superadmin visibility for new links -- correct security posture

Controller sketches: The SessionsController#create and ApplicationController helper methods are clean. The role extraction from realm_access.roles is correct for Keycloak's token structure.

Terraform config: PKCE S256, confidential client, disabled direct access grants -- all align with platform SOP patterns. Registration disabled is appropriate for a manually-managed user base.

Secrets wiring: Uses secretKeyRef references to a SOPS-encrypted secret -- no plaintext secrets. The provisioning order (realm -> client -> network policy -> user -> secrets) is logically sequenced.

Follow-up tickets: Seven concrete tickets with clear scope and dependency ordering. Good breakdown.

BLOCKERS

None. This is a docs-only spike with no runtime code changes. No secrets, credentials, or executable code is being committed.

NITS

1. Schema drift between docs: The feature_flags table in docs/auth.md omits the description column that exists in docs/architecture.md (line 118 of architecture.md: string description "human-readable explanation"). These two docs should stay in sync. The auth.md version has name, enabled_globally, role_overrides, created_at, updated_at -- architecture.md adds description. Since architecture.md already exists as the canonical schema reference, auth.md should include description to avoid confusion during implementation.

2. Flag resolution nil check: In the feature_enabled? method, the per-user override check uses != nil:

   if user&.preferences&.dig("flags", flag_name) != nil
   

   This is intentional (distinguishing false from absent), but a comment explaining the != nil pattern would help future readers understand it is not a mistake. Minor readability nit.

3. Clicks table constraint: The doc states "at least one of user_id or session_id should be present (enforced at the model level, not the DB)." Consider adding a brief note about why not at the DB level -- the reasoning (keeping migrations simple) is stated but a sentence about the tradeoff (model-only validation can be bypassed by raw SQL) would strengthen the design rationale.

4. Local dev auth bypass: Follow-up ticket 7 mentions checking Rails.env.development? for a mock superadmin session. The "Secrets > Local Development" section also references this pattern. Consider consolidating these into a single reference to avoid divergence when the implementation ticket is written.

5. Missing FEATURE_FLAGS relationship line: The mermaid ER diagram in auth.md shows USERS ||--o{ CLICKS and LINKS ||--o{ CLICKS but omits the FEATURE_FLAGS }o--o{ USERS relationship that exists in docs/architecture.md (line 126). The relationship exists conceptually (via users.preferences JSONB referencing flag names) and should be diagrammed for completeness.

SOP COMPLIANCE

• Branch named after issue: 16-keycloak-integration-spike follows {issue-number}-{kebab-case-purpose}
• PR body follows template: Summary, Changes, Test Plan, Review Checklist, Related Notes all present
• Related Notes references parent issue: ldraney/palinks #16
• Closes #16 present in PR body
• No secrets committed: env vars use ENV.fetch(), secrets reference SOPS paths
• No unnecessary file changes: single file (docs/auth.md), appropriate for spike scope
• Commit messages: not visible in diff but PR title is descriptive
• Scope is contained: docs-only spike, no runtime code changes

PROCESS OBSERVATIONS

• This spike delivers a thorough design document that addresses gem selection, schema design, Keycloak provisioning, secrets wiring, and follow-up ticket breakdown. The scope is well-contained for a spike deliverable.
• The seven follow-up tickets are well-sequenced with clear dependency chains. This positions the implementation phase for parallel work where possible (e.g., visibility column migration can ship independently of auth wiring).
• The doc is consistent with existing docs/architecture.md and docs/visibility.md with minor schema drift (nits above). Resolving the drift before merge prevents confusion during implementation.
• Change failure risk is low -- docs-only change with no deployment impact.

VERDICT: APPROVED

## PR #23 Review ### DOMAIN REVIEW **Tech stack**: Ruby on Rails 8.1, PostgreSQL, Keycloak OIDC, Terraform/k8s (design doc references). This is a docs-only spike -- no runtime code changes. Review focuses on design document quality, consistency with existing docs, and correctness of proposed patterns. **Gem recommendation (omniauth-keycloak)**: Sound. The rationale for choosing it over Devise and bare `openid_connect` is well-articulated. The fallback path to `omniauth_openid_connect` if the gem is abandoned is a practical escape hatch. The inclusion of `omniauth-rails_csrf_protection` is correct -- OmniAuth 2.x mandates POST-only auth initiation. **Schema design**: The four-table design (users, clicks, feature_flags, visibility column on links) is coherent. Key design decisions are well-reasoned: - `keycloak_sub` as stable identifier over email -- correct - String column over Postgres enum for visibility -- pragmatic for zero-downtime deploys - Separate `feature_flags` table vs JSONB on users -- appropriate separation of concerns - Safe default of `superadmin` visibility for new links -- correct security posture **Controller sketches**: The `SessionsController#create` and `ApplicationController` helper methods are clean. The role extraction from `realm_access.roles` is correct for Keycloak's token structure. **Terraform config**: PKCE S256, confidential client, disabled direct access grants -- all align with platform SOP patterns. Registration disabled is appropriate for a manually-managed user base. **Secrets wiring**: Uses `secretKeyRef` references to a SOPS-encrypted secret -- no plaintext secrets. The provisioning order (realm -> client -> network policy -> user -> secrets) is logically sequenced. **Follow-up tickets**: Seven concrete tickets with clear scope and dependency ordering. Good breakdown. ### BLOCKERS None. This is a docs-only spike with no runtime code changes. No secrets, credentials, or executable code is being committed. ### NITS 1. **Schema drift between docs**: The `feature_flags` table in `docs/auth.md` omits the `description` column that exists in `docs/architecture.md` (line 118 of architecture.md: `string description "human-readable explanation"`). These two docs should stay in sync. The auth.md version has `name`, `enabled_globally`, `role_overrides`, `created_at`, `updated_at` -- architecture.md adds `description`. Since architecture.md already exists as the canonical schema reference, auth.md should include `description` to avoid confusion during implementation. 2. **Flag resolution nil check**: In the `feature_enabled?` method, the per-user override check uses `!= nil`: ```ruby if user&.preferences&.dig("flags", flag_name) != nil ``` This is intentional (distinguishing `false` from absent), but a comment explaining the `!= nil` pattern would help future readers understand it is not a mistake. Minor readability nit. 3. **Clicks table constraint**: The doc states "at least one of user_id or session_id should be present (enforced at the model level, not the DB)." Consider adding a brief note about *why* not at the DB level -- the reasoning (keeping migrations simple) is stated but a sentence about the tradeoff (model-only validation can be bypassed by raw SQL) would strengthen the design rationale. 4. **Local dev auth bypass**: Follow-up ticket 7 mentions checking `Rails.env.development?` for a mock superadmin session. The "Secrets > Local Development" section also references this pattern. Consider consolidating these into a single reference to avoid divergence when the implementation ticket is written. 5. **Missing `FEATURE_FLAGS` relationship line**: The mermaid ER diagram in auth.md shows `USERS ||--o{ CLICKS` and `LINKS ||--o{ CLICKS` but omits the `FEATURE_FLAGS }o--o{ USERS` relationship that exists in `docs/architecture.md` (line 126). The relationship exists conceptually (via `users.preferences` JSONB referencing flag names) and should be diagrammed for completeness. ### SOP COMPLIANCE - [x] Branch named after issue: `16-keycloak-integration-spike` follows `{issue-number}-{kebab-case-purpose}` - [x] PR body follows template: Summary, Changes, Test Plan, Review Checklist, Related Notes all present - [x] Related Notes references parent issue: `ldraney/palinks #16` - [x] `Closes #16` present in PR body - [x] No secrets committed: env vars use `ENV.fetch()`, secrets reference SOPS paths - [x] No unnecessary file changes: single file (`docs/auth.md`), appropriate for spike scope - [x] Commit messages: not visible in diff but PR title is descriptive - [x] Scope is contained: docs-only spike, no runtime code changes ### PROCESS OBSERVATIONS - This spike delivers a thorough design document that addresses gem selection, schema design, Keycloak provisioning, secrets wiring, and follow-up ticket breakdown. The scope is well-contained for a spike deliverable. - The seven follow-up tickets are well-sequenced with clear dependency chains. This positions the implementation phase for parallel work where possible (e.g., visibility column migration can ship independently of auth wiring). - The doc is consistent with existing `docs/architecture.md` and `docs/visibility.md` with minor schema drift (nits above). Resolving the drift before merge prevents confusion during implementation. - Change failure risk is low -- docs-only change with no deployment impact. ### VERDICT: APPROVED

ldraney added 1 commit 2026-06-08 02:12:01 +00:00

Fix QA review nits on auth.md spike doc ... 0d5625bc1a

- Add missing description column to FEATURE_FLAGS ER diagram
- Add FEATURE_FLAGS-USERS relationship to match architecture.md
- Add auth.md to README docs section
- Clarify != nil pattern with inline comment on intent
- Note that raw SQL bypasses clicks table model validation

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

ldraney commented 2026-06-08 02:15:00 +00:00

Author

Owner

Copy link

PR #23 Review

DOMAIN REVIEW

Tech stack: This is a docs-only spike PR (Ruby on Rails context). The deliverable is docs/auth.md -- a design document for Keycloak OIDC integration. No runtime code changes. Review focuses on design quality, consistency with existing docs, and spike completeness.

Spike coverage: The five implicit design questions from issue #16 are addressed:

1. OIDC gem selection -- evaluated three options, recommended omniauth-keycloak with clear rationale
2. Schema design -- four new tables with ER diagram and migration plan
3. Keycloak provisioning -- realm, client, user creation, network policy
4. Secrets wiring -- env vars, SOPS encryption, k8s secretKeyRef, local dev bypass
5. Follow-up ticket breakdown -- seven concrete implementation tickets with dependency ordering

Design quality: The document is thorough. The gem comparison table is well-structured. The schema ER diagram, flag resolution logic, controller sketches, and provisioning order are all clear and actionable. The Terraform config snippets match the patterns described in docs/infrastructure.md.

Consistency with existing docs: The schema defined in docs/auth.md is consistent with the schema already documented in docs/architecture.md (lines 80-127). The visibility model matches docs/visibility.md. The feature flag resolution order matches docs/feature-flags.md. No contradictions found.

Design observation (non-blocking): The extract_role fallback in the SessionsController sketch defaults any authenticated user without an explicit Keycloak role to "member". This is a permissive default -- if someone gets a Keycloak account but is not assigned a role, they silently get member-level access. The implementation ticket should explicitly decide whether to fail closed (reject users with no role) or fail open (default to member). For a spike doc this is fine to leave as a design note.

Overlap note: docs/auth.md necessarily restates some schema and visibility content that exists in docs/architecture.md and docs/visibility.md. This is acceptable for a spike deliverable -- the auth doc is meant to be a self-contained design record. The implementation tickets should reference auth.md as the source of truth and avoid further duplication.

BLOCKERS

1. Merge conflict -- PR reports mergeable: false. The README.md edit (adding the Auth doc link after the Visibility line) conflicts with the Feature Flags doc link that was merged to main via PR #27. The branch must be rebased onto current main to resolve this conflict before merge. The fix is trivial (the Auth link line just needs to be placed after the Feature Flags line), but the PR cannot merge in its current state.

NITS

1. README link ordering -- After rebase, the Auth doc link should be placed after Feature Flags to maintain the logical grouping: Architecture > Infrastructure > Pipeline > Visibility > Feature Flags > Auth > User Stories > Rails Notes. Auth depends on concepts from both Visibility and Feature Flags, so it reads better after them.

2. != nil pattern in flag resolution -- The code sketch uses user.preferences.dig("flags", flag_name) != nil with a comment explaining the explicit nil check. This is correct Ruby but idiomatic Rails would use .key? or .has_key? to distinguish "key absent" from "key present with nil value". Since this is a sketch and not shipped code, this is purely a nit for the implementation ticket to consider.

3. Clicks table constraint -- The doc notes that "at least one of user_id or session_id should be present" but is enforced at the model level, not DB. The stated rationale (keep migration simple) is reasonable, but the implementation ticket should consider a DB-level check constraint (CHECK (user_id IS NOT NULL OR session_id IS NOT NULL)) since it is a one-line addition to the migration and prevents data integrity issues from raw SQL or console operations.

4. PKCE for confidential client -- The Terraform config mandates pkce_code_challenge_method = "S256" for a confidential client. While PKCE for confidential clients is technically unnecessary (the client secret already prevents code interception), the doc correctly notes this is per platform SOP. No action needed, just noting the design rationale is sound.

5. Redirect URI -- The valid_redirect_uris only lists the production Tailscale URL. The implementation ticket should consider whether a dev redirect URI (e.g., http://localhost:9999/auth/keycloak/callback) should be added, or whether the local dev auth bypass (ticket #7 in the follow-ups) makes this unnecessary.

SOP COMPLIANCE

• Branch named after issue (16-keycloak-integration-spike)
• PR body follows template (Summary, Changes, Test Plan, Review Checklist, Related Notes)
• Related Notes references parent issue (ldraney/palinks #16)
• Closes #16 present in PR body
• No secrets committed -- env var references use ENV.fetch(), no actual values
• No unnecessary file changes -- only docs/auth.md (new) and README.md (doc index update)
• Commit messages -- single doc-add commit, descriptive enough for a spike
• Scope appropriate -- docs-only spike, no runtime code changes

PROCESS OBSERVATIONS

• Deployment frequency: No deployment impact -- docs-only change. Once the merge conflict is resolved, this can merge cleanly.
• Change failure risk: Zero -- no runtime changes. The design doc is a reference artifact.
• Documentation gap: The follow-up ticket list (7 tickets) is well-structured with clear dependency ordering. The implementation phase will benefit from the explicit provisioning order (Keycloak realm first, then secrets, then Rails integration).
• Merge conflict cause: The branch was created before PR #27 (Feature Flags guide) merged. This is a normal sequencing issue with parallel spike work. Rebase will resolve it.

VERDICT: NOT APPROVED

Reason: Single blocker -- merge conflict (mergeable: false). The README.md change conflicts with the Feature Flags doc link added by PR #27. Rebase onto main, resolve the one-line conflict in README.md, and this is ready to merge. The design content itself is thorough and well-aligned with existing docs.

## PR #23 Review ### DOMAIN REVIEW **Tech stack**: This is a docs-only spike PR (Ruby on Rails context). The deliverable is `docs/auth.md` -- a design document for Keycloak OIDC integration. No runtime code changes. Review focuses on design quality, consistency with existing docs, and spike completeness. **Spike coverage**: The five implicit design questions from issue #16 are addressed: 1. OIDC gem selection -- evaluated three options, recommended `omniauth-keycloak` with clear rationale 2. Schema design -- four new tables with ER diagram and migration plan 3. Keycloak provisioning -- realm, client, user creation, network policy 4. Secrets wiring -- env vars, SOPS encryption, k8s secretKeyRef, local dev bypass 5. Follow-up ticket breakdown -- seven concrete implementation tickets with dependency ordering **Design quality**: The document is thorough. The gem comparison table is well-structured. The schema ER diagram, flag resolution logic, controller sketches, and provisioning order are all clear and actionable. The Terraform config snippets match the patterns described in `docs/infrastructure.md`. **Consistency with existing docs**: The schema defined in `docs/auth.md` is consistent with the schema already documented in `docs/architecture.md` (lines 80-127). The visibility model matches `docs/visibility.md`. The feature flag resolution order matches `docs/feature-flags.md`. No contradictions found. **Design observation (non-blocking)**: The `extract_role` fallback in the `SessionsController` sketch defaults any authenticated user without an explicit Keycloak role to `"member"`. This is a permissive default -- if someone gets a Keycloak account but is not assigned a role, they silently get member-level access. The implementation ticket should explicitly decide whether to fail closed (reject users with no role) or fail open (default to member). For a spike doc this is fine to leave as a design note. **Overlap note**: `docs/auth.md` necessarily restates some schema and visibility content that exists in `docs/architecture.md` and `docs/visibility.md`. This is acceptable for a spike deliverable -- the auth doc is meant to be a self-contained design record. The implementation tickets should reference auth.md as the source of truth and avoid further duplication. ### BLOCKERS 1. **Merge conflict** -- PR reports `mergeable: false`. The README.md edit (adding the Auth doc link after the Visibility line) conflicts with the Feature Flags doc link that was merged to main via PR #27. The branch must be rebased onto current main to resolve this conflict before merge. The fix is trivial (the Auth link line just needs to be placed after the Feature Flags line), but the PR cannot merge in its current state. ### NITS 1. **README link ordering** -- After rebase, the Auth doc link should be placed after Feature Flags to maintain the logical grouping: Architecture > Infrastructure > Pipeline > Visibility > Feature Flags > Auth > User Stories > Rails Notes. Auth depends on concepts from both Visibility and Feature Flags, so it reads better after them. 2. **`!= nil` pattern in flag resolution** -- The code sketch uses `user.preferences.dig("flags", flag_name) != nil` with a comment explaining the explicit nil check. This is correct Ruby but idiomatic Rails would use `.key?` or `.has_key?` to distinguish "key absent" from "key present with nil value". Since this is a sketch and not shipped code, this is purely a nit for the implementation ticket to consider. 3. **Clicks table constraint** -- The doc notes that "at least one of user_id or session_id should be present" but is enforced at the model level, not DB. The stated rationale (keep migration simple) is reasonable, but the implementation ticket should consider a DB-level check constraint (`CHECK (user_id IS NOT NULL OR session_id IS NOT NULL)`) since it is a one-line addition to the migration and prevents data integrity issues from raw SQL or console operations. 4. **PKCE for confidential client** -- The Terraform config mandates `pkce_code_challenge_method = "S256"` for a confidential client. While PKCE for confidential clients is technically unnecessary (the client secret already prevents code interception), the doc correctly notes this is per platform SOP. No action needed, just noting the design rationale is sound. 5. **Redirect URI** -- The `valid_redirect_uris` only lists the production Tailscale URL. The implementation ticket should consider whether a dev redirect URI (e.g., `http://localhost:9999/auth/keycloak/callback`) should be added, or whether the local dev auth bypass (ticket #7 in the follow-ups) makes this unnecessary. ### SOP COMPLIANCE - [x] Branch named after issue (`16-keycloak-integration-spike`) - [x] PR body follows template (Summary, Changes, Test Plan, Review Checklist, Related Notes) - [x] Related Notes references parent issue (`ldraney/palinks #16`) - [x] `Closes #16` present in PR body - [x] No secrets committed -- env var references use `ENV.fetch()`, no actual values - [x] No unnecessary file changes -- only `docs/auth.md` (new) and `README.md` (doc index update) - [x] Commit messages -- single doc-add commit, descriptive enough for a spike - [x] Scope appropriate -- docs-only spike, no runtime code changes ### PROCESS OBSERVATIONS - **Deployment frequency**: No deployment impact -- docs-only change. Once the merge conflict is resolved, this can merge cleanly. - **Change failure risk**: Zero -- no runtime changes. The design doc is a reference artifact. - **Documentation gap**: The follow-up ticket list (7 tickets) is well-structured with clear dependency ordering. The implementation phase will benefit from the explicit provisioning order (Keycloak realm first, then secrets, then Rails integration). - **Merge conflict cause**: The branch was created before PR #27 (Feature Flags guide) merged. This is a normal sequencing issue with parallel spike work. Rebase will resolve it. ### VERDICT: NOT APPROVED **Reason**: Single blocker -- merge conflict (`mergeable: false`). The README.md change conflicts with the Feature Flags doc link added by PR #27. Rebase onto main, resolve the one-line conflict in README.md, and this is ready to merge. The design content itself is thorough and well-aligned with existing docs.

ldraney force-pushed 16-keycloak-integration-spike from 0d5625bc1a to 0b75419538 2026-06-08 02:16:18 +00:00 Compare

ldraney commented 2026-06-08 02:18:17 +00:00

Author

Owner

Copy link

PR #23 Review

Re-review after rebase to resolve merge conflicts.

DOMAIN REVIEW

Tech stack: Ruby on Rails (docs-only spike -- no runtime code changes). The PR adds docs/auth.md (395 lines) and a one-line README.md update linking to it.

Document quality -- auth.md:

The design doc is thorough and well-structured. It covers five design areas with concrete, actionable recommendations:

1. OIDC Gem Evaluation -- Three options compared in a table with pros/cons. omniauth-keycloak recommendation is well-reasoned: narrow auth requirement, drop-in Rails convention, and a clear escape hatch if the gem is abandoned. The CSRF protection gem (omniauth-rails_csrf_protection) is correctly called out as mandatory for OmniAuth 2.x.

2. Schema Design -- The ER diagram is consistent with the existing data model in docs/architecture.md (lines 80-127). The visibility column design (string, not Postgres enum) has a clear rationale. The visible_to scope matches the implementation sketch in docs/visibility.md (lines 95-101) exactly. Feature flag resolution logic (per-user > role > global) is consistent with docs/feature-flags.md (lines 40-46).

3. Keycloak Provisioning -- Terraform config snippets follow the existing k3s.tfvars pattern. Key security decisions are correct: confidential client (not public), PKCE S256 mandatory, direct access grants disabled, registration disabled. The provisioning order (8 steps) is complete and correctly sequences the cross-repo dependencies.

4. Secrets Wiring -- All four env vars are documented. The secretKeyRef YAML matches existing patterns in pal-e-deployments. No actual secrets are committed -- only variable names and structure.

5. Follow-Up Tickets -- Seven concrete implementation tickets are listed with dependency ordering and specific file targets. Each ticket's scope is narrow enough to implement independently.

Cross-reference accuracy:

• The schema in auth.md matches docs/architecture.md exactly (same table structures, same column names, same relationships).
• The visible_to scope matches docs/visibility.md verbatim.
• The feature flag resolution order matches docs/feature-flags.md.
• The secrets table aligns with docs/infrastructure.md (lines 156-165), which already lists KEYCLOAK_CLIENT_ID and KEYCLOAK_CLIENT_SECRET as planned.
• The README update inserts the auth doc link in alphabetical order within the existing docs list.

Minor design observations (non-blocking):

• The extract_role method in the SessionsController sketch defaults unknown authenticated users to "member". This is a reasonable safe default for this app's two-role model, but the design doc should note this is a deliberate choice (it is implied but not explicitly called out as a design decision).
• The != nil check on line user.preferences.dig("flags", flag_name) != nil in the flag resolution logic is correct but unconventional Ruby -- !user.preferences.dig("flags", flag_name).nil? is more idiomatic. This is a design sketch, not shipped code, so it is a minor observation.
• The clicks table allows both user_id and session_id to be null at the DB level, with model-level validation as a trade-off. The doc acknowledges this explicitly and explains why. Acceptable for a spike.

BLOCKERS

None. This is a docs-only spike with no runtime code changes. No secrets committed, no unvalidated input, no test coverage gap (nothing to test).

NITS

1. The README diff inserts the [Auth] link between [Feature Flags] and [User Stories]. This breaks the existing ordering pattern -- the docs list is currently ordered by topic flow (Architecture > Infrastructure > Pipeline > Visibility > Feature Flags > User Stories > Rails Notes). Auth logically belongs after Visibility and before or alongside Feature Flags. The current placement is acceptable but could be reordered to Architecture > Infrastructure > Pipeline > Visibility > Auth > Feature Flags > ... to follow the logical flow from access tiers to auth to feature gating. Non-blocking.

2. The initializer sketch references OmniAuth::Strategies::KeycloakOpenId with an explicit strategy_class parameter. This may not be needed if the :keycloak_openid symbol resolves correctly. Worth verifying during implementation. Non-blocking since this is a design sketch.

SOP COMPLIANCE

• Branch named after issue: 16-keycloak-integration-spike follows {issue-number}-{kebab-case-purpose} convention
• PR body follows template: Summary, Changes, Test Plan, Review Checklist, Related sections all present
• Related section references parent issue: ldraney/palinks #16 and sop-keycloak-client-creation
• No secrets committed: only env var names and structure documented, no actual values
• No unnecessary file changes: 2 files changed (auth.md + README link), both directly relevant
• Commit messages: not visible in diff but PR title is descriptive
• Closes #16 present in PR body

PROCESS OBSERVATIONS

• This is a well-executed spike deliverable. The design doc is actionable -- each section has enough detail for a developer to implement without ambiguity.
• The seven follow-up tickets are properly sequenced with dependency notes, which will support clean parallel work.
• The doc correctly identifies that Migration 1 (add visibility column) can ship independently before auth is wired, reducing change failure risk.
• Cross-repo consistency is strong: the schema, visibility scope, and feature flag resolution all match existing docs exactly, indicating the rebase resolved conflicts cleanly.

VERDICT: APPROVED

## PR #23 Review Re-review after rebase to resolve merge conflicts. ### DOMAIN REVIEW **Tech stack**: Ruby on Rails (docs-only spike -- no runtime code changes). The PR adds `docs/auth.md` (395 lines) and a one-line `README.md` update linking to it. **Document quality -- auth.md**: The design doc is thorough and well-structured. It covers five design areas with concrete, actionable recommendations: 1. **OIDC Gem Evaluation** -- Three options compared in a table with pros/cons. `omniauth-keycloak` recommendation is well-reasoned: narrow auth requirement, drop-in Rails convention, and a clear escape hatch if the gem is abandoned. The CSRF protection gem (`omniauth-rails_csrf_protection`) is correctly called out as mandatory for OmniAuth 2.x. 2. **Schema Design** -- The ER diagram is consistent with the existing data model in `docs/architecture.md` (lines 80-127). The `visibility` column design (string, not Postgres enum) has a clear rationale. The `visible_to` scope matches the implementation sketch in `docs/visibility.md` (lines 95-101) exactly. Feature flag resolution logic (per-user > role > global) is consistent with `docs/feature-flags.md` (lines 40-46). 3. **Keycloak Provisioning** -- Terraform config snippets follow the existing `k3s.tfvars` pattern. Key security decisions are correct: confidential client (not public), PKCE S256 mandatory, direct access grants disabled, registration disabled. The provisioning order (8 steps) is complete and correctly sequences the cross-repo dependencies. 4. **Secrets Wiring** -- All four env vars are documented. The `secretKeyRef` YAML matches existing patterns in `pal-e-deployments`. No actual secrets are committed -- only variable names and structure. 5. **Follow-Up Tickets** -- Seven concrete implementation tickets are listed with dependency ordering and specific file targets. Each ticket's scope is narrow enough to implement independently. **Cross-reference accuracy**: - The schema in auth.md matches `docs/architecture.md` exactly (same table structures, same column names, same relationships). - The `visible_to` scope matches `docs/visibility.md` verbatim. - The feature flag resolution order matches `docs/feature-flags.md`. - The secrets table aligns with `docs/infrastructure.md` (lines 156-165), which already lists `KEYCLOAK_CLIENT_ID` and `KEYCLOAK_CLIENT_SECRET` as planned. - The README update inserts the auth doc link in alphabetical order within the existing docs list. **Minor design observations (non-blocking)**: - The `extract_role` method in the `SessionsController` sketch defaults unknown authenticated users to `"member"`. This is a reasonable safe default for this app's two-role model, but the design doc should note this is a deliberate choice (it is implied but not explicitly called out as a design decision). - The `!= nil` check on line `user.preferences.dig("flags", flag_name) != nil` in the flag resolution logic is correct but unconventional Ruby -- `!user.preferences.dig("flags", flag_name).nil?` is more idiomatic. This is a design sketch, not shipped code, so it is a minor observation. - The clicks table allows both `user_id` and `session_id` to be null at the DB level, with model-level validation as a trade-off. The doc acknowledges this explicitly and explains why. Acceptable for a spike. ### BLOCKERS None. This is a docs-only spike with no runtime code changes. No secrets committed, no unvalidated input, no test coverage gap (nothing to test). ### NITS 1. The README diff inserts the `[Auth]` link between `[Feature Flags]` and `[User Stories]`. This breaks the existing ordering pattern -- the docs list is currently ordered by topic flow (Architecture > Infrastructure > Pipeline > Visibility > Feature Flags > User Stories > Rails Notes). Auth logically belongs after Visibility and before or alongside Feature Flags. The current placement is acceptable but could be reordered to `Architecture > Infrastructure > Pipeline > Visibility > Auth > Feature Flags > ...` to follow the logical flow from access tiers to auth to feature gating. Non-blocking. 2. The initializer sketch references `OmniAuth::Strategies::KeycloakOpenId` with an explicit `strategy_class` parameter. This may not be needed if the `:keycloak_openid` symbol resolves correctly. Worth verifying during implementation. Non-blocking since this is a design sketch. ### SOP COMPLIANCE - [x] Branch named after issue: `16-keycloak-integration-spike` follows `{issue-number}-{kebab-case-purpose}` convention - [x] PR body follows template: Summary, Changes, Test Plan, Review Checklist, Related sections all present - [x] Related section references parent issue: `ldraney/palinks #16` and `sop-keycloak-client-creation` - [x] No secrets committed: only env var names and structure documented, no actual values - [x] No unnecessary file changes: 2 files changed (auth.md + README link), both directly relevant - [x] Commit messages: not visible in diff but PR title is descriptive - [x] `Closes #16` present in PR body ### PROCESS OBSERVATIONS - This is a well-executed spike deliverable. The design doc is actionable -- each section has enough detail for a developer to implement without ambiguity. - The seven follow-up tickets are properly sequenced with dependency notes, which will support clean parallel work. - The doc correctly identifies that Migration 1 (add visibility column) can ship independently before auth is wired, reducing change failure risk. - Cross-repo consistency is strong: the schema, visibility scope, and feature flag resolution all match existing docs exactly, indicating the rebase resolved conflicts cleanly. ### VERDICT: APPROVED

ldraney merged commit 3be5f73b42 into main 2026-06-08 03:16:32 +00:00

ldraney deleted branch 16-keycloak-integration-spike 2026-06-08 03:16:32 +00:00

ldraney referenced this pull request from a commit 2026-06-08 03:16:33 +00:00

Spike: Keycloak auth integration design (#23)

Sign in to join this conversation.

Reviewers

No reviewers

Labels

Clear labels

No items

No labels

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

1 participant

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

ldraney/palinks!23

No description provided.