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

Add persistent session cookie (30-day expiry) #170

Merged
ldraney merged 2 commits from ui-observations into main 2026-06-08 00:31:45 +00:00
Conversation 1 Commits 2 Files changed 2 +2
ldraney commented 2026-06-08 00:20:00 +00:00
Owner
Copy link

Summary

Fix users getting signed out on browser/tab close by adding a 30-day persistent session cookie. Rails default is ephemeral browser-session cookies — no expire_after means the cookie dies when the browser process exits.

Changes

  • Add config/initializers/session_store.rb with expire_after: 30.days

Test Plan

  • Log in, close browser completely, reopen — session persists
  • Cookie inspector shows _landscaping_assistant_session with 30-day max-age
  • Logout still works (clears session)

Review Checklist

  • Single-line change, no risk of breaking existing behavior
  • Cookie is still httponly and secure in production (Rails defaults)

Related Notes

Closes #168

Partial fix for persistent auth. The Keycloak SSO idle timeout (30 min default) still needs a pal-e-services Terraform change — that remains on the spike backlog.

## Summary Fix users getting signed out on browser/tab close by adding a 30-day persistent session cookie. Rails default is ephemeral browser-session cookies — no `expire_after` means the cookie dies when the browser process exits. ## Changes - Add `config/initializers/session_store.rb` with `expire_after: 30.days` ## Test Plan - [ ] Log in, close browser completely, reopen — session persists - [ ] Cookie inspector shows `_landscaping_assistant_session` with 30-day max-age - [ ] Logout still works (clears session) ## Review Checklist - [x] Single-line change, no risk of breaking existing behavior - [x] Cookie is still httponly and secure in production (Rails defaults) ## Related Notes Closes #168 Partial fix for persistent auth. The Keycloak SSO idle timeout (30 min default) still needs a pal-e-services Terraform change — that remains on the spike backlog.
Add persistent session cookie (30-day expiry)
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 failed
Details
ci/woodpecker/pr/woodpecker Pipeline failed
Details
2608c41ea6 
Rails default is browser-session cookies that die on tab close.
This was causing users to get signed out constantly.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
ldraney commented 2026-06-08 00:21:28 +00:00
Author
Owner
Copy link

PR #170 Review

DOMAIN REVIEW

Tech stack: Ruby on Rails 8.1, cookie-based session store, OmniAuth/Keycloak SSO.

Change: Adds config/initializers/session_store.rb with a single line setting expire_after: 30.days on the cookie store.

Correctness:

  • Rails default is :cookie_store with no expire_after, meaning ephemeral browser-session cookies. This change correctly overrides that to persist the session across browser restarts.
  • The expire_after option sets the Max-Age attribute on the cookie, which is the standard mechanism for persistent cookies. This is the correct Rails API.
  • 30 days is a reasonable duration for a crew-facing app where users should not need to re-authenticate frequently.

Security analysis:

  • httponly: Rails sets this by default on session cookies via ActionDispatch -- no risk of JS access.
  • secure flag: Production has config.assume_ssl = true (line 29 of production.rb). In Rails 8.x, this causes ActionDispatch to mark cookies as secure. The cookie will not be sent over plain HTTP in production. Confirmed safe.
  • SameSite: Rails 8.x defaults to SameSite=Lax, which provides CSRF protection for the cookie. No concern.
  • Session content: The session stores {username, email, roles} (per sessions_controller.rb:17-21). Extending cookie lifetime to 30 days means this data persists longer. Since Rails encrypts/signs the cookie by default, the data is not exposed client-side. Acceptable.
  • Logout still works: session.delete(:user) in SessionsController#destroy clears the server-side session data, and Rails will issue a new (empty) cookie. The 30-day expiry does not interfere with explicit logout.

One consideration (not a blocker): The Keycloak SSO session has a separate idle timeout (30 min by default, per the PR body). After the Keycloak session expires, the Rails cookie will still keep the user "logged in" to the Rails app until either: (a) 30 days pass, or (b) the user explicitly logs out. This is acknowledged in the PR body as a known gap requiring a separate Terraform change. For this app's threat model (internal crew tool on Tailscale), this is acceptable.

BLOCKERS

None. This is a one-line declarative configuration change with no logic to test, no user input, no secrets, and correct security posture.

On test coverage: Normally new functionality without tests is a blocker. However, this is a Rails initializer setting a framework-provided option. The behavior (cookie Max-Age header) is tested by Rails itself. An integration test proving "session persists after browser close" would require browser lifecycle management beyond what RSpec/Capybara provides in-process. No test gap here.

NITS

  1. The file has no trailing newline after the single line. Most editors and linters prefer files end with \n. Minor -- git diff shows \ No newline at end of file is not present, so this is fine.

  2. Consider adding a brief comment explaining the "why" for future developers:

# Persist sessions across browser restarts (default is ephemeral).
# Users were getting signed out on every browser/tab close.
Rails.application.config.session_store :cookie_store, expire_after: 30.days

This is purely optional for a one-line file.

SOP COMPLIANCE

  • Branch named after issue -- branch is ui-observations, should be 168-persistent-session-cookie per convention
  • PR body follows template (Summary, Changes, Test Plan, Related)
  • Related references plan slug -- no plan slug (acknowledged by caller)
  • No secrets committed
  • No unnecessary file changes (single file, single line)
  • Closes #168 referenced

PROCESS OBSERVATIONS

  • Branch naming convention not followed. This appears to be a quick fix bundled under a general "ui-observations" branch rather than a dedicated issue branch. Low risk for a one-line change, but worth noting for process hygiene.
  • The PR correctly identifies that this is only a partial fix -- the Keycloak SSO idle timeout is a separate concern in pal-e-services infrastructure. Good scope discipline.
  • Change failure risk: very low. This is additive configuration with no behavior change for existing sessions until they renew.

VERDICT: APPROVED

## PR #170 Review ### DOMAIN REVIEW **Tech stack:** Ruby on Rails 8.1, cookie-based session store, OmniAuth/Keycloak SSO. **Change:** Adds `config/initializers/session_store.rb` with a single line setting `expire_after: 30.days` on the cookie store. **Correctness:** - Rails default is `:cookie_store` with no `expire_after`, meaning ephemeral browser-session cookies. This change correctly overrides that to persist the session across browser restarts. - The `expire_after` option sets the `Max-Age` attribute on the cookie, which is the standard mechanism for persistent cookies. This is the correct Rails API. - 30 days is a reasonable duration for a crew-facing app where users should not need to re-authenticate frequently. **Security analysis:** - `httponly`: Rails sets this by default on session cookies via ActionDispatch -- no risk of JS access. - `secure` flag: Production has `config.assume_ssl = true` (line 29 of `production.rb`). In Rails 8.x, this causes ActionDispatch to mark cookies as `secure`. The cookie will not be sent over plain HTTP in production. Confirmed safe. - `SameSite`: Rails 8.x defaults to `SameSite=Lax`, which provides CSRF protection for the cookie. No concern. - Session content: The session stores `{username, email, roles}` (per `sessions_controller.rb:17-21`). Extending cookie lifetime to 30 days means this data persists longer. Since Rails encrypts/signs the cookie by default, the data is not exposed client-side. Acceptable. - Logout still works: `session.delete(:user)` in `SessionsController#destroy` clears the server-side session data, and Rails will issue a new (empty) cookie. The 30-day expiry does not interfere with explicit logout. **One consideration (not a blocker):** The Keycloak SSO session has a separate idle timeout (30 min by default, per the PR body). After the Keycloak session expires, the Rails cookie will still keep the user "logged in" to the Rails app until either: (a) 30 days pass, or (b) the user explicitly logs out. This is acknowledged in the PR body as a known gap requiring a separate Terraform change. For this app's threat model (internal crew tool on Tailscale), this is acceptable. ### BLOCKERS None. This is a one-line declarative configuration change with no logic to test, no user input, no secrets, and correct security posture. **On test coverage:** Normally new functionality without tests is a blocker. However, this is a Rails initializer setting a framework-provided option. The behavior (cookie `Max-Age` header) is tested by Rails itself. An integration test proving "session persists after browser close" would require browser lifecycle management beyond what RSpec/Capybara provides in-process. No test gap here. ### NITS 1. The file has no trailing newline after the single line. Most editors and linters prefer files end with `\n`. Minor -- `git diff` shows `\ No newline at end of file` is not present, so this is fine. 2. Consider adding a brief comment explaining the "why" for future developers: ```ruby # Persist sessions across browser restarts (default is ephemeral). # Users were getting signed out on every browser/tab close. Rails.application.config.session_store :cookie_store, expire_after: 30.days ``` This is purely optional for a one-line file. ### SOP COMPLIANCE - [ ] Branch named after issue -- branch is `ui-observations`, should be `168-persistent-session-cookie` per convention - [x] PR body follows template (Summary, Changes, Test Plan, Related) - [ ] Related references plan slug -- no plan slug (acknowledged by caller) - [x] No secrets committed - [x] No unnecessary file changes (single file, single line) - [x] Closes #168 referenced ### PROCESS OBSERVATIONS - Branch naming convention not followed. This appears to be a quick fix bundled under a general "ui-observations" branch rather than a dedicated issue branch. Low risk for a one-line change, but worth noting for process hygiene. - The PR correctly identifies that this is only a partial fix -- the Keycloak SSO idle timeout is a separate concern in pal-e-services infrastructure. Good scope discipline. - Change failure risk: very low. This is additive configuration with no behavior change for existing sessions until they renew. ### VERDICT: APPROVED
Fix authorize_property_access nil user in dev mode
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
841fbd8728 
Guard with keycloak_configured? && logged_in? before accessing
current_user — matches the pattern used everywhere else in the app.
Fixes 15 spec failures from PR #169.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
ldraney deleted branch ui-observations 2026-06-08 00:31:45 +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!170
No description provided.