Do not redirect to the login page on a 403 Forbidden error.

[minwoox]

Motivation:
A 403 Forbidden error indicates that the user is authenticated but lacks the necessary permissions to access a specific resource. Redirecting an already logged-in user back to the login page in this scenario is incorrect behavior.

Modification:

• Do not redirect to the login page on a 403 Forbidden error.

Result:

• You are no longer forcibly redirected to the login page when a 403 Forbidden response is received from an API request.

Summary by CodeRabbit

• Bug Fixes

  • Refined authentication error response handling to improve application stability and consistency

• Tests

  • Updated test server infrastructure configuration and authentication credential validation scenarios for enhanced test coverage

[coderabbitai]

Walkthrough

The changes disable encryption at rest in the SAML test server, expand credential validation to accept multiple login pairs in the IdP, and modify the web app API to handle only HTTP 401 errors for reauthentication instead of both 401 and 403 responses.

Changes

Cohort / File(s)	Summary
SAML Test Infrastructure webapp/javaTest/java/com/linecorp/centraldogma/webapp/SamlCentralDogmaTestServer.java, webapp/javaTest/java/com/linecorp/centraldogma/webapp/SamlIdpServer.java	Disabled encryption at rest in test server config; broadened login credential validation to accept two credential pairs instead of one, affecting authentication control flow.
Web App API Authentication webapp/src/dogma/features/api/apiSlice.ts	Narrowed reauthentication retry logic to trigger only on HTTP 401 errors; removed 403 error handling for auth/redirect.

Sequence Diagram

sequenceDiagram
    participant User
    participant IdP as SAML IdP Server
    participant LoginService

    User->>IdP: Submit login with credentials
    IdP->>LoginService: Validate credentials
    
    alt Credential pair ("foo", "bar") or ("foo2", "bar2")
        LoginService->>IdP: Valid credentials
        IdP->>IdP: Parse SAMLRequest
        IdP->>IdP: Generate SAMLResponse
        IdP-->>User: Login successful
    else Invalid credential pair
        LoginService->>IdP: Invalid credentials
        IdP-->>User: Login failed
    end

Loading

sequenceDiagram
    participant Client
    participant API as API Slice
    participant Server

    Client->>API: Make request
    API->>Server: Send request
    
    alt HTTP 401 Response
        Server-->>API: 401 Unauthorized
        API->>API: Clear auth state
        API->>Client: Redirect to login
    else HTTP 403 Response
        Server-->>API: 403 Forbidden
        API-->>Client: Return error (no auth clearing)
    else Other Response
        Server-->>API: Response
        API-->>Client: Return response
    end

Loading

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

• Config-only change in test server (trivial)
• Login credential validation logic is straightforward and localized
• API retry logic change is a single conditional branch modification
• All changes are concentrated in three files with clear, isolated intent

Suggested reviewers

• trustin
• jrhee17

Poem

🐰 With credentials now doubled and errors refined,
SAML flows smoothly, encryption unlined,
401 alone rings the re-auth bell,
While 403 stays silent—oh, what a spell!
Test servers dance free, no secrets to keep. 🎭

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%.	You can run @coderabbitai generate docstrings to improve docstring coverage.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The PR title directly and clearly describes the main change in apiSlice.ts where 403 Forbidden error handling was modified to prevent login page redirects.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment

---

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 2092bbd and 37ed79b.

📒 Files selected for processing (3)

• webapp/javaTest/java/com/linecorp/centraldogma/webapp/SamlCentralDogmaTestServer.java (1 hunks)
• webapp/javaTest/java/com/linecorp/centraldogma/webapp/SamlIdpServer.java (1 hunks)
• webapp/src/dogma/features/api/apiSlice.ts (1 hunks)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)

• GitHub Check: lint
• GitHub Check: docker-build
• GitHub Check: build-ubuntu-latest-jdk-17-min-java-11
• GitHub Check: build-ubuntu-latest-jdk-17-coverage
• GitHub Check: build-ubuntu-latest-jdk-11
• GitHub Check: build-macos-latest-jdk-21
• GitHub Check: build-ubuntu-latest-jdk-21-snapshot
• GitHub Check: build-windows-latest-jdk-21
• GitHub Check: flaky-tests

🔇 Additional comments (3)

webapp/javaTest/java/com/linecorp/centraldogma/webapp/SamlCentralDogmaTestServer.java (1)

55-55: Clarify why encryption at rest was disabled in the test server.

This change disables encryption at rest in the SAML test server but isn't explained in the PR description. The PR's stated objective is to fix 403 Forbidden error handling, which doesn't appear related to encryption configuration.

Is this change intentional and related to the PR objective, or should it be reverted? If intentional, please clarify the motivation in the PR description.

webapp/javaTest/java/com/linecorp/centraldogma/webapp/SamlIdpServer.java (1)

117-118: Clarify the purpose of adding a second test credential pair.

This change adds support for a second credential pair ("foo2"/"bar2") in the test IDP server, but the connection to the PR's stated objective (fixing 403 redirect behavior) is unclear.

Is this credential addition:

• Required for new test cases related to the 403 handling fix?
• An unrelated enhancement that happened to be included in this PR?
• Fixing a pre-existing test issue?

If unrelated to the PR objective, consider moving it to a separate PR for clearer change tracking.

webapp/src/dogma/features/api/apiSlice.ts (1)

129-132: LGTM! 401/403 distinction is correct and 403 error handling is verified in the codebase.

The code change correctly restricts reauthentication to 401 errors only. Verification confirms 403 Forbidden errors are handled appropriately: specific views (RepositorySettingsView, ProjectSettingsView) treat 403 as GUEST access and clear the error flag, while a generic Error component (Deferred.tsx) displays errors with status codes when needed. No issues found.

---

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

• X
• Mastodon
• Reddit
• LinkedIn

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