Skip to content

permissions: skip /permissions/available fetch on unauthenticated pages#1808

Merged
gugu merged 2 commits into
mainfrom
frontend/gate-permissions-available-fetch
May 28, 2026
Merged

permissions: skip /permissions/available fetch on unauthenticated pages#1808
gugu merged 2 commits into
mainfrom
frontend/gate-permissions-available-fetch

Conversation

@gugu
Copy link
Copy Markdown
Contributor

@gugu gugu commented May 28, 2026

UsersService is provided in root and eagerly creates an httpResource for /permissions/available, which auto-fires on app bootstrap before any auth check, leaking a 401 request from /login and other unauthenticated pages. Gate the request function on a new isAuthenticated signal in AuthService — seeded from the existing localStorage 'token_expiration' and flipped via setAuthenticated() at the login, session-restoration, and logout transitions already managed by AppComponent. Mirrors the connection-id gating used by the other httpResource-backed services.

Summary by CodeRabbit

  • Bug Fixes

    • More reliable authentication state updates during login, session restore, and logout.
    • Improved session token validation to reduce unexpected sign-outs.
  • Behavior Change

    • Available permissions now load only when signed in, preventing unauthorized/empty permission views.
  • Tests

    • Unit tests updated to reflect the revised authentication behavior.

Review Change Stack

UsersService is provided in root and eagerly creates an httpResource for
/permissions/available, which auto-fires on app bootstrap before any
auth check, leaking a 401 request from /login and other unauthenticated
pages. Gate the request function on a new isAuthenticated signal in
AuthService — seeded from the existing localStorage 'token_expiration'
and flipped via setAuthenticated() at the login, session-restoration,
and logout transitions already managed by AppComponent. Mirrors the
connection-id gating used by the other httpResource-backed services.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@gugu gugu enabled auto-merge (squash) May 28, 2026 10:02
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 28, 2026

📝 Walkthrough

Walkthrough

Adds an Angular Signal-based auth state to AuthService, updates AppComponent to call setAuthenticated(...) during login/session restore and logout flows, and gates UsersService permission fetching on the auth signal. Tests updated to mock isAuthenticated as a signal.

Changes

Signal-based Reactive Authentication

Layer / File(s) Summary
AuthService signal foundation and token validation
frontend/src/app/services/auth.service.ts
Adds signal import; creates private _isAuthenticated initialized from _hasValidSessionToken(), exposes isAuthenticated as readonly, adds setAuthenticated() and the _hasValidSessionToken() helper that checks localStorage.token_expiration.
AppComponent authentication state synchronization
frontend/src/app/app.component.ts, frontend/src/app/app.component.spec.ts
AppComponent calls this._auth.setAuthenticated(true) on login init and session restore, and this._auth.setAuthenticated(false) after logoutAndRedirectToRegistration() and logOut(). Unit test updated to mock isAuthenticated using Angular signal.
UsersService conditional permission loading
frontend/src/app/services/users.service.ts
Injects AuthService as _auth and changes availablePermissions resource to fetch /permissions/available only when _auth.isAuthenticated() is true; otherwise sets the resource to undefined so computed permissions are empty.

Sequence Diagram

sequenceDiagram
  participant AppComponent
  participant AuthService
  participant UsersService
  participant PermissionsAPI
  AppComponent->>AuthService: setAuthenticated(true) on login/session restore
  AuthService->>UsersService: isAuthenticated signal update triggers
  UsersService->>PermissionsAPI: fetch /permissions/available
  PermissionsAPI-->>UsersService: permissions
  AppComponent->>AuthService: setAuthenticated(false) on logout
  UsersService->>UsersService: availablePermissions becomes undefined / cleared
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

🐰 I hop on signals, soft and bright,

True on login, false on night.
Permissions wake when auth is clear,
And sleep again when logout's near.

🚥 Pre-merge checks | ✅ 6
✅ Passed checks (6 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title directly and clearly reflects the main change: gating the /permissions/available fetch to skip it when unauthenticated, which is the core problem being solved.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Security Check ✅ Passed Gating /permissions/available on isAuthenticated signal. Server validates via httpOnly cookies. Client signal cannot bypass auth. Safe date parsing.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch frontend/gate-permissions-available-fetch

Warning

There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure.

🔧 ESLint

If the error stems from missing dependencies, add them to the package.json file. For unrecoverable errors (e.g., due to private dependencies), disable the tool in the CodeRabbit configuration.

ESLint skipped: no ESLint configuration detected in root package.json. To enable, add eslint to devDependencies.


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
frontend/src/app/app.component.ts (1)

353-361: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Clear local auth state even when /user/logout fails.

Both logout paths only remove token_expiration and flip isAuthenticated inside the success callback. Since AuthService.logOutUser() catches errors and returns EMPTY (frontend/src/app/services/auth.service.ts, Lines 301-310), any failed logout request skips that callback and leaves the app authenticated locally.

Move the local cleanup into a shared helper that runs before the request, or into a finalize() path that always executes.

Also applies to: 365-381

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@frontend/src/app/app.component.ts` around lines 353 - 361, The local auth
cleanup (clearing token_expiration, setting isDemo and isAuthenticated, clearing
currentUser) must run regardless of the HTTP logout result; modify
logoutAndRedirectToRegistration (and the similar method around lines 365-381) to
perform the local cleanup in a shared helper (e.g., applyLocalLogoutState()) or
invoke it in an RxJS finalize() on the AuthService.logOutUser() observable so
cleanup always executes even if logOutUser() returns EMPTY or errors; keep the
navigation (router.navigate(['/registration']) or the other method’s redirect)
in the success path if you want it conditional, but ensure the local state
changes and localStorage.removeItem('token_expiration') are applied
unconditionally via the helper/finalize.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Outside diff comments:
In `@frontend/src/app/app.component.ts`:
- Around line 353-361: The local auth cleanup (clearing token_expiration,
setting isDemo and isAuthenticated, clearing currentUser) must run regardless of
the HTTP logout result; modify logoutAndRedirectToRegistration (and the similar
method around lines 365-381) to perform the local cleanup in a shared helper
(e.g., applyLocalLogoutState()) or invoke it in an RxJS finalize() on the
AuthService.logOutUser() observable so cleanup always executes even if
logOutUser() returns EMPTY or errors; keep the navigation
(router.navigate(['/registration']) or the other method’s redirect) in the
success path if you want it conditional, but ensure the local state changes and
localStorage.removeItem('token_expiration') are applied unconditionally via the
helper/finalize.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: f6a2bf66-e4b3-4288-8c0d-6e4eaea5c192

📥 Commits

Reviewing files that changed from the base of the PR and between 6927ca0 and 99e4f14.

📒 Files selected for processing (3)
  • frontend/src/app/app.component.ts
  • frontend/src/app/services/auth.service.ts
  • frontend/src/app/services/users.service.ts

….spec

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@frontend/src/app/app.component.spec.ts`:
- Around line 47-53: The isAuthenticatedSignal is shared across the suite
causing state leakage; modify the tests to recreate/reset the signal for each
test (e.g., move creation of isAuthenticatedSignal and mockAuthService into a
beforeEach or call isAuthenticatedSignal.set(false) in an afterEach) so each
test gets a fresh signal; update references to isAuthenticatedSignal,
mockAuthService, and its setAuthenticated wrapper accordingly to ensure
isolation between tests.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 40631277-d2a7-43ab-9647-f9d4875e4258

📥 Commits

Reviewing files that changed from the base of the PR and between 99e4f14 and e4c8cc3.

📒 Files selected for processing (1)
  • frontend/src/app/app.component.spec.ts

Comment thread frontend/src/app/app.component.spec.ts
@gugu gugu merged commit 736cfea into main May 28, 2026
17 of 19 checks passed
@gugu gugu deleted the frontend/gate-permissions-available-fetch branch May 28, 2026 10:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant