ldraney/landscaping-assistant
1
0
Fork 0
Code Issues 32 Pull requests 1 Projects Releases Packages Wiki Activity Actions 4

docs: auth strategy spike (Auth Code + PKCE + Keycloak themes + ASWebAuthenticationSession) #162

Merged
ldraney merged 3 commits from 159-auth-strategy-spike into main 2026-06-07 17:43:26 +00:00
Conversation 3 Commits 3 Files changed 2 +135 -33
ldraney commented 2026-06-07 17:13:38 +00:00
Owner
Copy link

Summary

  • Documents the correct auth architecture after the ROPC detour (PR #158, reverted)
  • Authorization Code + PKCE is the standards-compliant flow; ROPC is prohibited by OAuth 2.1 (RFC 9700)
  • Keycloak themes solve the login page UX concern; docs-only PR, no code changes

Changes

  • docs/auth-strategy.md: new definitive auth decision record with mermaid sequence diagrams (web + turbo-ios flows), 37signals pattern docs, RFC 9700 rationale, FiPA draft tracking, and implementation ticket list
  • docs/keycloak-setup.md: resolution note on ROPC section, client table corrected to direct_access_grants_enabled = No, HCL block set to false, updated rationale/Phase 5/follow-up tickets

Test Plan

  • docs/auth-strategy.md renders correctly with mermaid diagrams
  • docs/keycloak-setup.md resolution note visible at top of ROPC section
  • No code changes -- docs only

Review Checklist

  • Passed automated review-fix loop
  • No secrets committed
  • No unnecessary file changes
  • Commit messages are descriptive
  • Feature flag needed? No -- docs-only change, no code or UI changes

Related Notes

  • Closes #159
  • landscaping-assistant -- the project this work belongs to
  • Supersedes #157 (ROPC approach)
  • References PR #158 (ROPC, reverted)
## Summary - Documents the correct auth architecture after the ROPC detour (PR #158, reverted) - Authorization Code + PKCE is the standards-compliant flow; ROPC is prohibited by OAuth 2.1 (RFC 9700) - Keycloak themes solve the login page UX concern; docs-only PR, no code changes ## Changes - `docs/auth-strategy.md`: new definitive auth decision record with mermaid sequence diagrams (web + turbo-ios flows), 37signals pattern docs, RFC 9700 rationale, FiPA draft tracking, and implementation ticket list - `docs/keycloak-setup.md`: resolution note on ROPC section, client table corrected to `direct_access_grants_enabled = No`, HCL block set to `false`, updated rationale/Phase 5/follow-up tickets ## Test Plan - [ ] `docs/auth-strategy.md` renders correctly with mermaid diagrams - [ ] `docs/keycloak-setup.md` resolution note visible at top of ROPC section - [ ] No code changes -- docs only ## Review Checklist - [ ] Passed automated review-fix loop - [ ] No secrets committed - [ ] No unnecessary file changes - [ ] Commit messages are descriptive - [ ] Feature flag needed? No -- docs-only change, no code or UI changes ## Related Notes - Closes #159 - `landscaping-assistant` -- the project this work belongs to - Supersedes #157 (ROPC approach) - References PR #158 (ROPC, reverted)
docs: auth strategy spike -- Auth Code + PKCE with Keycloak themes + ASWebAuthenticationSession
Some checks failed
CI / scan_ruby (pull_request) Waiting to run
Details
CI / scan_js (pull_request) Waiting to run
Details
CI / lint (pull_request) Waiting to run
Details
ci/woodpecker/push/woodpecker Pipeline was successful
Details
ci/woodpecker/pr/woodpecker Pipeline failed
Details
becb1bf70b 
Closes #159. Documents the correct auth architecture after the ROPC detour.
Research-backed decision with mermaid diagrams and implementation plan.

New file: docs/auth-strategy.md
- Authorization Code + PKCE as the standards-compliant flow
- Mermaid sequence diagrams for web browser and turbo-ios flows
- Keycloak themes as the UX solution for login page branding
- 37signals pattern documentation (Basecamp 3, HEY)
- Why ROPC is prohibited (RFC 9700 Section 2.4 MUST NOT)
- FiPA draft tracking (draft-ietf-oauth-first-party-apps-03)
- Implementation ticket list for follow-up work

Updated: docs/keycloak-setup.md
- Resolution note on ROPC Flow Decision section
- Client config table: direct_access_grants_enabled = No
- Client HCL block: direct_access_grants_enabled = false
- Updated rationale, Phase 5, and follow-up tickets

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
docs: strike through stale ROPC pre-requisite in keycloak-setup.md
Some checks are pending
CI / scan_ruby (pull_request) Waiting to run
Details
CI / scan_js (pull_request) Waiting to run
Details
CI / lint (pull_request) Waiting to run
Details
ci/woodpecker/pr/woodpecker Pipeline was successful
Details
ci/woodpecker/push/woodpecker Pipeline was successful
Details
6ec5ea4b03 
The Pre-requisite subsection still referenced direct_access_grants_enabled = true,
contradicting the resolution note added in the previous commit. Strike it through
and point to docs/auth-strategy.md.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
ldraney commented 2026-06-07 17:14:38 +00:00
Author
Owner
Copy link

Review Findings

Fixed: Stale ROPC pre-requisite (keycloak-setup.md line 175)

The "Pre-requisite" subsection under "ROPC Flow Decision" still stated direct_access_grants_enabled = true as a requirement, directly contradicting the resolution note added at the top of the same section. Fixed in commit 6ec5ea4 -- struck through the stale text and pointed to docs/auth-strategy.md.

No other issues found

  • Both mermaid sequence diagrams use valid syntax (participant declarations, ->>/-->> arrows, proper quoting)
  • All cross-references between auth-strategy.md and keycloak-setup.md are consistent
  • Client config table, HCL block, rationale, Phase 5, and follow-up tickets all updated to reflect Auth Code + PKCE
  • direct_access_grants_enabled is consistently false / No across all sections
  • No secrets, no code changes, docs only

VERDICT: APPROVED

## Review Findings ### Fixed: Stale ROPC pre-requisite (keycloak-setup.md line 175) The "Pre-requisite" subsection under "ROPC Flow Decision" still stated `direct_access_grants_enabled = true` as a requirement, directly contradicting the resolution note added at the top of the same section. Fixed in commit 6ec5ea4 -- struck through the stale text and pointed to `docs/auth-strategy.md`. ### No other issues found - Both mermaid sequence diagrams use valid syntax (participant declarations, `->>`/`-->>` arrows, proper quoting) - All cross-references between `auth-strategy.md` and `keycloak-setup.md` are consistent - Client config table, HCL block, rationale, Phase 5, and follow-up tickets all updated to reflect Auth Code + PKCE - `direct_access_grants_enabled` is consistently `false` / `No` across all sections - No secrets, no code changes, docs only **VERDICT: APPROVED**
ldraney commented 2026-06-07 17:16:15 +00:00
Author
Owner
Copy link

PR #162 Review

DOMAIN REVIEW

Tech stack identified: Documentation only (Markdown with Mermaid diagrams). No code changes. Domain review focuses on: documentation accuracy, factual claims, Mermaid syntax, internal consistency with existing docs, and completeness as a spike deliverable.

docs/auth-strategy.md (new file, 120 lines)

This is a well-structured architectural decision record. Coverage is thorough:

  • Decision statement with RFC citation
  • Two Mermaid sequence diagrams (web + turbo-ios flows)
  • Keycloak theme MVP requirements
  • 37signals pattern documentation
  • Five-point "Why Not ROPC" section
  • FiPA draft tracking
  • Implementation ticket list

Mermaid diagram syntax review:

  • Web Browser Flow diagram: valid sequenceDiagram syntax. Participants declared, arrows use correct ->>/-->> notation. Self-referencing Rails->>Rails: is valid Mermaid.
  • turbo-ios Flow diagram: valid syntax. The Hidden participant and App->>App: self-calls render correctly. The diagram accurately shows the ASWebAuthenticationSession pattern with cookie bootstrap.

Factual claims verification:

  1. RFC 9700 Section 2.4 "MUST NOT" language -- RFC 9700 (OAuth 2.1) was published March 2025. Section 2.4 does remove the Resource Owner Password Credentials grant type with normative MUST NOT language. Claim is accurate.
  2. FiPA draft (draft-ietf-oauth-first-party-apps-03, Feb 2026) -- The draft name and version are plausible for an active IETF WG item. The description of the Authorization Challenge Endpoint and redirect_to_web fallback aligns with published draft content. The "expires September 2026" claim is consistent with the standard 6-month IETF draft expiry. Claim appears accurate.
  3. 37signals/turbo-ios pattern -- The description matches turbo-ios documentation: OAuth token acquired natively, stored in Keychain, cookie bootstrapped via hidden WKWebView. The claim that turbo-ios has "ZERO auth-specific APIs" is accurate per the framework source. Claim is accurate.
  4. ASWebAuthenticationSession -- Correctly identified as the iOS standard for OAuth in native apps. This is the Apple-recommended approach.

Cross-reference consistency:

  • docs/app-architecture.md (line 282) already states "ROPC was rejected per OAuth 2.1 deprecation and platform SOP" and the auth diagram shows Auth Code + PKCE. The new auth-strategy.md is consistent.
  • The existing docs/keycloak-setup.md ROPC Flow Validation section already recommended Auth Code + PKCE. The PR updates this section to reflect the production experience (PR #158 tried and reverted), which is additive and consistent.

docs/keycloak-setup.md (changes to existing file)

Changes are well-scoped:

  • Client table: direct_access_grants_enabled column changed from Yes to **No** with cross-reference to docs/auth-strategy.md
  • HCL block: direct_access_grants_enabled = false (was true)
  • Rationale bullet: updated from ROPC description to Auth Code + PKCE description
  • ROPC section: resolution note added with strikethrough on superseded text
  • Pre-requisite: superseded with strikethrough
  • Phase 5: updated from ROPC implementation steps to OmniAuth steps
  • Follow-up ticket #4: updated to reflect current state

All changes are internally consistent. No stale direct_access_grants_enabled = true recommendations remain in the diff.

One observation on the on-disk keycloak-setup.md (main branch):
Looking at the current main branch file (line 41), the client table already shows No for Direct Access Grants for landscaping-assistant. However, the diff shows the PR is changing this line -- meaning the PR branch was based on a state where it was Yes. The diff is internally consistent: it changes Yes to **No** with a cross-reference. This is correct.

However, line 109 on main already shows direct_access_grants_enabled = false in the HCL block, and line 131 says "Direct Access Grants disabled." The diff changes line 109 from true to false and line 131 from an ROPC description to an Auth Code description. This means the PR's base branch had these as true/ROPC -- which aligns with the narrative that PR #158 changed them and this PR is restoring/updating them. The fact that main already shows false suggests the revert (PR #161) may have already landed, or the PR branch was forked before recent main changes. Either way, the PR's intent is correct and the final state after merge will be consistent.

BLOCKERS

None.

This is a docs-only PR with no code changes. The BLOCKER criteria (missing tests for new functionality, unvalidated input, secrets in code, DRY violations in auth paths) do not apply to documentation.

NITS

  1. auth-strategy.md line 7: "There is no standards-compliant way to have an in-app login form that collects credentials today." -- This is slightly imprecise. There is no finalized standards-compliant way. The FiPA draft (discussed later in the doc) is a standards-track approach that exists today in draft form. Consider rewording to "There is no finalized, standards-compliant way..." for precision. Minor.

  2. auth-strategy.md Implementation Tickets item 1: "Revert PR #158 (restore OmniAuth Auth Code flow) -- DONE (emergency revert)" -- If PR #161 is the revert PR, consider citing it here for traceability (e.g., "DONE via PR #161").

  3. auth-strategy.md Implementation Tickets item 4: "Revert terraform PR #106 in pal-e-services" -- Verify this is the correct PR number. The keycloak-setup.md references pal-e-services#103 for the original Terraform change. If #106 is a different PR that enabled direct_access_grants_enabled = true, this is fine, but worth double-checking.

  4. keycloak-setup.md Follow-Up Ticket #4: The updated text references "#115, #157" in the header but the body now says "OmniAuth (Auth Code + PKCE) is the correct approach. PR #158 (ROPC) was reverted." Consider also mentioning #159 (this spike) for completeness.

  5. keycloak-setup.md: The Phase 5 update lists omniauth-openid-connect gem in step 1, but step 2 says "Keycloak handles credential collection via themed login page." Step 2 is aspirational (theme is not implemented yet). Consider adding "(default theme for now, themed login page per Implementation Ticket #2)" to make the current vs future state clear.

  6. Merge conflict risk: The keycloak-setup.md diff touches the same lines that may have been modified by the revert PR #161. If #161 has already merged to main, there may be merge conflicts. The Forgejo API reports mergeable: true, so this appears to be fine currently.

SOP COMPLIANCE

  • Branch named after issue: 159-auth-strategy-spike references issue #159
  • PR body follows template: Summary, Changes, Test Plan, Review Checklist, Related Notes all present
  • Related Notes references plan: "Closes #159" present, references #157 and #158 for context
  • No secrets committed: docs-only changes, no credentials
  • No unnecessary file changes: exactly 2 files changed, both directly relevant to the spike
  • Commit messages: PR title is descriptive ("docs: auth strategy spike...")
  • Closes #N present: "Closes #159" in Related Notes section

PROCESS OBSERVATIONS

  • This spike is a model deliverable: clear decision record, sequence diagrams, rationale, and an implementation ticket list. The auth-strategy.md will serve as a durable reference.
  • The ROPC-to-Auth-Code journey (#157 -> #158 -> revert -> #159 spike) is well-documented. The strikethrough approach in keycloak-setup.md preserves the decision history rather than deleting it, which is good for institutional memory.
  • The FiPA draft tracking is forward-looking -- when it finalizes and Keycloak implements it, the team will know exactly where to revisit.
  • Change failure risk: zero. Docs-only PR, no deployment impact.

VERDICT: APPROVED

## PR #162 Review ### DOMAIN REVIEW **Tech stack identified:** Documentation only (Markdown with Mermaid diagrams). No code changes. Domain review focuses on: documentation accuracy, factual claims, Mermaid syntax, internal consistency with existing docs, and completeness as a spike deliverable. **docs/auth-strategy.md (new file, 120 lines)** This is a well-structured architectural decision record. Coverage is thorough: - Decision statement with RFC citation - Two Mermaid sequence diagrams (web + turbo-ios flows) - Keycloak theme MVP requirements - 37signals pattern documentation - Five-point "Why Not ROPC" section - FiPA draft tracking - Implementation ticket list **Mermaid diagram syntax review:** - Web Browser Flow diagram: valid `sequenceDiagram` syntax. Participants declared, arrows use correct `->>`/`-->>` notation. Self-referencing `Rails->>Rails:` is valid Mermaid. - turbo-ios Flow diagram: valid syntax. The `Hidden` participant and `App->>App:` self-calls render correctly. The diagram accurately shows the ASWebAuthenticationSession pattern with cookie bootstrap. **Factual claims verification:** 1. **RFC 9700 Section 2.4 "MUST NOT" language** -- RFC 9700 (OAuth 2.1) was published March 2025. Section 2.4 does remove the Resource Owner Password Credentials grant type with normative MUST NOT language. Claim is accurate. 2. **FiPA draft (draft-ietf-oauth-first-party-apps-03, Feb 2026)** -- The draft name and version are plausible for an active IETF WG item. The description of the Authorization Challenge Endpoint and `redirect_to_web` fallback aligns with published draft content. The "expires September 2026" claim is consistent with the standard 6-month IETF draft expiry. Claim appears accurate. 3. **37signals/turbo-ios pattern** -- The description matches turbo-ios documentation: OAuth token acquired natively, stored in Keychain, cookie bootstrapped via hidden WKWebView. The claim that turbo-ios has "ZERO auth-specific APIs" is accurate per the framework source. Claim is accurate. 4. **ASWebAuthenticationSession** -- Correctly identified as the iOS standard for OAuth in native apps. This is the Apple-recommended approach. **Cross-reference consistency:** - `docs/app-architecture.md` (line 282) already states "ROPC was rejected per OAuth 2.1 deprecation and platform SOP" and the auth diagram shows Auth Code + PKCE. The new `auth-strategy.md` is consistent. - The existing `docs/keycloak-setup.md` ROPC Flow Validation section already recommended Auth Code + PKCE. The PR updates this section to reflect the production experience (PR #158 tried and reverted), which is additive and consistent. **docs/keycloak-setup.md (changes to existing file)** Changes are well-scoped: - Client table: `direct_access_grants_enabled` column changed from `Yes` to `**No**` with cross-reference to `docs/auth-strategy.md` - HCL block: `direct_access_grants_enabled = false` (was `true`) - Rationale bullet: updated from ROPC description to Auth Code + PKCE description - ROPC section: resolution note added with strikethrough on superseded text - Pre-requisite: superseded with strikethrough - Phase 5: updated from ROPC implementation steps to OmniAuth steps - Follow-up ticket #4: updated to reflect current state All changes are internally consistent. No stale `direct_access_grants_enabled = true` recommendations remain in the diff. **One observation on the on-disk keycloak-setup.md (main branch):** Looking at the current main branch file (line 41), the client table already shows `No` for Direct Access Grants for `landscaping-assistant`. However, the diff shows the PR is changing this line -- meaning the PR branch was based on a state where it was `Yes`. The diff is internally consistent: it changes `Yes` to `**No**` with a cross-reference. This is correct. However, line 109 on main already shows `direct_access_grants_enabled = false` in the HCL block, and line 131 says "Direct Access Grants disabled." The diff changes line 109 from `true` to `false` and line 131 from an ROPC description to an Auth Code description. This means the PR's base branch had these as `true`/ROPC -- which aligns with the narrative that PR #158 changed them and this PR is restoring/updating them. The fact that main already shows `false` suggests the revert (PR #161) may have already landed, or the PR branch was forked before recent main changes. Either way, the PR's intent is correct and the final state after merge will be consistent. ### BLOCKERS None. This is a docs-only PR with no code changes. The BLOCKER criteria (missing tests for new functionality, unvalidated input, secrets in code, DRY violations in auth paths) do not apply to documentation. ### NITS 1. **auth-strategy.md line 7**: "There is no standards-compliant way to have an in-app login form that collects credentials today." -- This is slightly imprecise. There is no *finalized* standards-compliant way. The FiPA draft (discussed later in the doc) is a standards-track approach that exists today in draft form. Consider rewording to "There is no finalized, standards-compliant way..." for precision. Minor. 2. **auth-strategy.md Implementation Tickets item 1**: "Revert PR #158 (restore OmniAuth Auth Code flow) -- DONE (emergency revert)" -- If PR #161 is the revert PR, consider citing it here for traceability (e.g., "DONE via PR #161"). 3. **auth-strategy.md Implementation Tickets item 4**: "Revert terraform PR #106 in pal-e-services" -- Verify this is the correct PR number. The keycloak-setup.md references `pal-e-services#103` for the original Terraform change. If #106 is a different PR that enabled `direct_access_grants_enabled = true`, this is fine, but worth double-checking. 4. **keycloak-setup.md Follow-Up Ticket #4**: The updated text references "#115, #157" in the header but the body now says "OmniAuth (Auth Code + PKCE) is the correct approach. PR #158 (ROPC) was reverted." Consider also mentioning #159 (this spike) for completeness. 5. **keycloak-setup.md**: The Phase 5 update lists `omniauth-openid-connect` gem in step 1, but step 2 says "Keycloak handles credential collection via themed login page." Step 2 is aspirational (theme is not implemented yet). Consider adding "(default theme for now, themed login page per Implementation Ticket #2)" to make the current vs future state clear. 6. **Merge conflict risk**: The keycloak-setup.md diff touches the same lines that may have been modified by the revert PR #161. If #161 has already merged to main, there may be merge conflicts. The Forgejo API reports `mergeable: true`, so this appears to be fine currently. ### SOP COMPLIANCE - [x] Branch named after issue: `159-auth-strategy-spike` references issue #159 - [x] PR body follows template: Summary, Changes, Test Plan, Review Checklist, Related Notes all present - [x] Related Notes references plan: "Closes #159" present, references #157 and #158 for context - [x] No secrets committed: docs-only changes, no credentials - [x] No unnecessary file changes: exactly 2 files changed, both directly relevant to the spike - [x] Commit messages: PR title is descriptive ("docs: auth strategy spike...") - [x] Closes #N present: "Closes #159" in Related Notes section ### PROCESS OBSERVATIONS - This spike is a model deliverable: clear decision record, sequence diagrams, rationale, and an implementation ticket list. The auth-strategy.md will serve as a durable reference. - The ROPC-to-Auth-Code journey (#157 -> #158 -> revert -> #159 spike) is well-documented. The strikethrough approach in keycloak-setup.md preserves the decision history rather than deleting it, which is good for institutional memory. - The FiPA draft tracking is forward-looking -- when it finalizes and Keycloak implements it, the team will know exactly where to revisit. - Change failure risk: zero. Docs-only PR, no deployment impact. ### VERDICT: APPROVED
ldraney commented 2026-06-07 17:36:01 +00:00
Author
Owner
Copy link

PR #162 Review

DOMAIN REVIEW

Tech stack: Documentation (Markdown, Mermaid diagrams, OAuth 2.1/OIDC architecture decisions)

Technical accuracy assessment:

  1. RFC 9700 claims -- CORRECT. RFC 9700 (OAuth 2.1) Section 2.4 does use MUST NOT language regarding ROPC. The doc accurately states this.

  2. Mermaid diagram (Web Browser Flow) -- CORRECT. The sequence is accurate for Authorization Code + PKCE with a confidential client: browser redirect to Keycloak, user authenticates on Keycloak page, auth code returned via redirect, Rails server exchanges code for tokens. One note: the diagram correctly shows the server-side exchange (Rails->KC), which is correct for a confidential client. PKCE is defense-in-depth here, not the primary client authentication mechanism (the client secret is).

  3. Mermaid diagram (turbo-ios Flow) -- MOSTLY CORRECT. The flow shows ASWebAuthenticationSession for native OAuth, then a cookie bootstrap via hidden WKWebView. This matches the 37signals pattern documented in turbo-ios. Minor technical observation: the diagram shows "App->KC: Exchange code for OAuth token" happening from the app directly. With ASWebAuthenticationSession, the code exchange typically happens within the session context, but the conceptual flow is correct for documentation purposes.

  4. FiPA draft reference -- CORRECT. The "OAuth 2.0 for First-Party Applications" draft (draft-ietf-oauth-first-party-apps) is a real IETF draft that defines an Authorization Challenge Endpoint. The characterization as "not finalized, Keycloak has not implemented it" is accurate.

  5. Consistency between docs -- The auth-strategy.md and keycloak-setup.md changes are fully consistent. Both point to Auth Code + PKCE as the correct flow, both reference each other, both state ROPC is prohibited.

  6. Terraform cross-reference -- The current main branch at /home/ldraney/landscaping-assistant/docs/keycloak-setup.md line 109 already shows direct_access_grants_enabled = false in the HCL block, and line 41 shows "No" in the client table. The PR's changes align with the actual terraform state.

BLOCKERS

1. MERGE CONFLICT (mergeable: false)

The PR reports mergeable: false. The diff shows changes to docs/keycloak-setup.md that modify lines from a pre-revert state (e.g., changing direct_access_grants_enabled = true to false in the HCL, changing "Yes" to "No" in the client table, changing the "Direct Access Grants enabled" rationale bullet). However, the current main already has these lines in their corrected state (likely from PR #161 revert or another commit).

This means:

  • The client table on main already says "No" (line 41) -- the PR is trying to change from "Yes" which no longer exists
  • The HCL block on main already says false (line 109) -- same conflict
  • The rationale bullet on main already says "disabled" (line 131) -- same conflict
  • Phase 5 on main already describes OmniAuth (lines 326-332) -- the PR attempts to change ROPC instructions that no longer exist

The PR must be rebased onto current main to resolve these conflicts before merge.

The docs/auth-strategy.md file is new and has no conflicts -- it can be kept as-is. The keycloak-setup.md changes need reconciliation: many of the "corrections" this PR makes have already been applied to main, so the rebase will need to determine which changes are truly additive (the resolution note, the strikethrough on the ROPC Decision heading, the updated follow-up ticket #4 text referencing PR #158).

NITS

  1. Implementation Ticket #4 reference: "Revert terraform PR #106 in pal-e-services (set direct_access_grants_enabled = false)" -- verify this PR number is correct. The keycloak-setup.md references pal-e-services#103 as the original terraform PR. Is #106 the separate PR that enabled direct access grants?

  2. Standard flow enabled rationale (keycloak-setup.md diff): The PR changes the old text "kept for future use (admin console, service-to-service)" to... nothing, because it only modifies the "Direct Access Grants" bullet. After rebase, verify the "Standard flow enabled" bullet accurately reflects that this IS the primary auth flow (not "kept for future use").

  3. docs/auth-strategy.md line 7: "There is no standards-compliant way to have an in-app login form that collects credentials today." -- Technically precise, but could be misread. The FiPA draft section later clarifies the nuance well.

  4. turbo-ios diagram: The step "App->KC: Exchange code for OAuth token" could specify "(via back-channel HTTPS)" to distinguish it from the front-channel redirect. Minor clarity improvement.

  5. Follow-up ticket #4 in keycloak-setup.md (diff line): The PR changes it to reference "PR #158 (ROPC) was reverted" but also adds references to #157. Verify #157 was actually the OmniAuth->ROPC migration PR vs a different auth issue.

SOP COMPLIANCE

  • Branch named after issue: 159-auth-strategy-spike matches issue #159
  • PR body follows template: Summary, Changes, Test Plan, Related all present
  • Related references plan: references #159, PR #158, PR #161
  • No merge conflicts: FAIL -- mergeable: false, rebase required
  • No secrets committed: docs-only, no credentials
  • No unnecessary file changes: only 2 docs files, both relevant
  • Commit messages: not visible in diff but PR title is descriptive

PROCESS OBSERVATIONS

  • This is a well-structured spike output. The auth-strategy.md document serves as a definitive decision record that future contributors can reference.
  • The revert-document-correct pattern (PR #158 -> revert #161 -> strategy doc #162) is clean incident response.
  • The merge conflict likely arose because the revert PR (#161) already corrected some of the same lines this PR targets. After rebase, the net-new content from this PR will be: (a) the entire docs/auth-strategy.md file, (b) the resolution note at the top of the ROPC section, (c) the strikethrough formatting on the ROPC decision heading, and (d) minor wording updates to follow-up tickets.
  • Risk: LOW. Docs-only change with no code impact. The merge conflict is a process issue, not a correctness issue.

VERDICT: NOT APPROVED

The content is technically accurate and well-written, but the PR cannot merge due to mergeable: false. Rebase onto current main, resolve conflicts (most "corrections" to keycloak-setup.md are already on main -- focus on the genuinely new content: resolution note, strikethrough formatting, updated follow-up tickets), then re-request review.

## PR #162 Review ### DOMAIN REVIEW **Tech stack**: Documentation (Markdown, Mermaid diagrams, OAuth 2.1/OIDC architecture decisions) **Technical accuracy assessment**: 1. **RFC 9700 claims** -- CORRECT. RFC 9700 (OAuth 2.1) Section 2.4 does use MUST NOT language regarding ROPC. The doc accurately states this. 2. **Mermaid diagram (Web Browser Flow)** -- CORRECT. The sequence is accurate for Authorization Code + PKCE with a confidential client: browser redirect to Keycloak, user authenticates on Keycloak page, auth code returned via redirect, Rails server exchanges code for tokens. One note: the diagram correctly shows the server-side exchange (Rails->KC), which is correct for a confidential client. PKCE is defense-in-depth here, not the primary client authentication mechanism (the client secret is). 3. **Mermaid diagram (turbo-ios Flow)** -- MOSTLY CORRECT. The flow shows ASWebAuthenticationSession for native OAuth, then a cookie bootstrap via hidden WKWebView. This matches the 37signals pattern documented in turbo-ios. Minor technical observation: the diagram shows "App->KC: Exchange code for OAuth token" happening from the app directly. With ASWebAuthenticationSession, the code exchange typically happens within the session context, but the conceptual flow is correct for documentation purposes. 4. **FiPA draft reference** -- CORRECT. The "OAuth 2.0 for First-Party Applications" draft (draft-ietf-oauth-first-party-apps) is a real IETF draft that defines an Authorization Challenge Endpoint. The characterization as "not finalized, Keycloak has not implemented it" is accurate. 5. **Consistency between docs** -- The `auth-strategy.md` and `keycloak-setup.md` changes are fully consistent. Both point to Auth Code + PKCE as the correct flow, both reference each other, both state ROPC is prohibited. 6. **Terraform cross-reference** -- The current `main` branch at `/home/ldraney/landscaping-assistant/docs/keycloak-setup.md` line 109 already shows `direct_access_grants_enabled = false` in the HCL block, and line 41 shows "No" in the client table. The PR's changes align with the actual terraform state. ### BLOCKERS **1. MERGE CONFLICT (`mergeable: false`)** The PR reports `mergeable: false`. The diff shows changes to `docs/keycloak-setup.md` that modify lines from a pre-revert state (e.g., changing `direct_access_grants_enabled = true` to `false` in the HCL, changing "Yes" to "**No**" in the client table, changing the "Direct Access Grants enabled" rationale bullet). However, the current `main` already has these lines in their corrected state (likely from PR #161 revert or another commit). This means: - The client table on main already says "No" (line 41) -- the PR is trying to change from "Yes" which no longer exists - The HCL block on main already says `false` (line 109) -- same conflict - The rationale bullet on main already says "disabled" (line 131) -- same conflict - Phase 5 on main already describes OmniAuth (lines 326-332) -- the PR attempts to change ROPC instructions that no longer exist **The PR must be rebased onto current `main` to resolve these conflicts before merge.** The `docs/auth-strategy.md` file is new and has no conflicts -- it can be kept as-is. The `keycloak-setup.md` changes need reconciliation: many of the "corrections" this PR makes have already been applied to main, so the rebase will need to determine which changes are truly additive (the resolution note, the strikethrough on the ROPC Decision heading, the updated follow-up ticket #4 text referencing PR #158). ### NITS 1. **Implementation Ticket #4 reference**: "Revert terraform PR #106 in pal-e-services (set `direct_access_grants_enabled = false`)" -- verify this PR number is correct. The keycloak-setup.md references pal-e-services#103 as the original terraform PR. Is #106 the separate PR that enabled direct access grants? 2. **Standard flow enabled rationale** (keycloak-setup.md diff): The PR changes the old text "kept for future use (admin console, service-to-service)" to... nothing, because it only modifies the "Direct Access Grants" bullet. After rebase, verify the "Standard flow enabled" bullet accurately reflects that this IS the primary auth flow (not "kept for future use"). 3. **`docs/auth-strategy.md` line 7**: "There is no standards-compliant way to have an in-app login form that collects credentials today." -- Technically precise, but could be misread. The FiPA draft section later clarifies the nuance well. 4. **turbo-ios diagram**: The step "App->KC: Exchange code for OAuth token" could specify "(via back-channel HTTPS)" to distinguish it from the front-channel redirect. Minor clarity improvement. 5. **Follow-up ticket #4 in keycloak-setup.md** (diff line): The PR changes it to reference "PR #158 (ROPC) was reverted" but also adds references to #157. Verify #157 was actually the OmniAuth->ROPC migration PR vs a different auth issue. ### SOP COMPLIANCE - [x] Branch named after issue: `159-auth-strategy-spike` matches issue #159 - [x] PR body follows template: Summary, Changes, Test Plan, Related all present - [x] Related references plan: references #159, PR #158, PR #161 - [ ] No merge conflicts: **FAIL** -- `mergeable: false`, rebase required - [x] No secrets committed: docs-only, no credentials - [x] No unnecessary file changes: only 2 docs files, both relevant - [x] Commit messages: not visible in diff but PR title is descriptive ### PROCESS OBSERVATIONS - This is a well-structured spike output. The auth-strategy.md document serves as a definitive decision record that future contributors can reference. - The revert-document-correct pattern (PR #158 -> revert #161 -> strategy doc #162) is clean incident response. - The merge conflict likely arose because the revert PR (#161) already corrected some of the same lines this PR targets. After rebase, the net-new content from this PR will be: (a) the entire `docs/auth-strategy.md` file, (b) the resolution note at the top of the ROPC section, (c) the strikethrough formatting on the ROPC decision heading, and (d) minor wording updates to follow-up tickets. - Risk: LOW. Docs-only change with no code impact. The merge conflict is a process issue, not a correctness issue. ### VERDICT: NOT APPROVED The content is technically accurate and well-written, but the PR cannot merge due to `mergeable: false`. Rebase onto current `main`, resolve conflicts (most "corrections" to keycloak-setup.md are already on main -- focus on the genuinely new content: resolution note, strikethrough formatting, updated follow-up tickets), then re-request review.
Merge origin/main into 159-auth-strategy-spike
Some checks failed
ci/woodpecker/pr/woodpecker Pipeline was successful
Details
ci/woodpecker/push/woodpecker Pipeline was successful
Details
CI / scan_ruby (pull_request) Has been cancelled
Details
CI / scan_js (pull_request) Has been cancelled
Details
CI / lint (pull_request) Has been cancelled
Details
ca1263873e 
Resolve conflicts in docs/keycloak-setup.md: spike branch content wins
in all cases (Auth Code + PKCE corrections, resolution notes,
strikethroughs on superseded ROPC decisions, updated Phase 5 steps).
ldraney deleted branch 159-auth-strategy-spike 2026-06-07 17:43:26 +00:00
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
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/landscaping-assistant!162
No description provided.