fix(WEB-1077): redirect to originally requested URL after login - #3772
fix(WEB-1077): redirect to originally requested URL after login#3772Ruba-Tawk-FOO wants to merge 1 commit into
Conversation
|
👋 Hi @Ruba-Tawk-FOO — thank you for your pull request. This PR is currently blocked because we do not have a Contributor License Agreement (CLA) on file for your GitHub account. To get unblocked:
|
mifos-cla-check
Bot
added
the
cla-required
|
Note
|
| Layer / File(s) | Summary |
|---|---|
Guard redirect preservation src/app/core/authentication/authentication.guard.ts |
AuthenticationGuard receives router state and redirects unauthenticated requests to /login with an optional returnUrl while retaining replaceUrl. |
Login return navigation src/app/login/login.component.ts |
LoginComponent reads and validates returnUrl, then navigates to it after successful authentication, falling back to /. |
Estimated code review effort: 3 (Moderate) | ~20 minutes
Suggested reviewers: alberto-art3ch
Sequence Diagram(s)
sequenceDiagram
participant Router
participant AuthenticationGuard
participant LoginComponent
Router->>AuthenticationGuard: Request protected URL
AuthenticationGuard->>Router: Redirect to /login with returnUrl
Router->>LoginComponent: Load login page
LoginComponent->>Router: Navigate to validated returnUrl after authentication
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
| Check name | Status | Explanation |
|---|---|---|
| Description Check | ✅ Passed | Check skipped - CodeRabbit’s high-level summary is enabled. |
| Title check | ✅ Passed | The title accurately summarizes the main change: preserving and restoring the originally requested URL after login. |
| 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. |
✨ Finishing Touches
🧪 Generate unit tests (beta)
- Create PR with unit tests
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.
Comment @coderabbitai help to get the list of available commands.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/app/login/login.component.ts (1)
237-243: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd focused tests for the return-URL contract.
Cover guard preservation and login validation for
/,/login, valid paths with query/fragment data, external URLs, non-string query values, protocol-relative paths, and failed destination navigation.🤖 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 `@src/app/login/login.component.ts` around lines 237 - 243, Add focused tests around LoginComponent.getReturnUrl and the login navigation flow. Verify guard preservation and validation for "/", "/login", valid paths with query strings or fragments, external and protocol-relative URLs, non-string returnUrl values, and failed destination navigation, preserving the expected safe fallback and error behavior.
🤖 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 `@src/app/login/login.component.ts`:
- Line 140: Update the post-login navigation around router.navigateByUrl to
handle both a false result and rejected navigation by falling back to the
dashboard route. Preserve replaceUrl behavior and use the existing dashboard
navigation mechanism.
---
Nitpick comments:
In `@src/app/login/login.component.ts`:
- Around line 237-243: Add focused tests around LoginComponent.getReturnUrl and
the login navigation flow. Verify guard preservation and validation for "/",
"/login", valid paths with query strings or fragments, external and
protocol-relative URLs, non-string returnUrl values, and failed destination
navigation, preserving the expected safe fallback and error behavior.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 154d12d0-780f-4e2f-b839-1d287526bcca
📒 Files selected for processing (2)
src/app/core/authentication/authentication.guard.tssrc/app/login/login.component.ts
|
@Ruba-Tawk-FOO could you please sign the CLA and also resolve the comment opened by CodeRabbit |
aad6c4e to
f8c28ec
Compare
mifos-cla-check
Bot
removed
the
cla-required
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/app/login/login.component.ts (2)
241-252: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd regression coverage for the return URL contract.
Cover nested paths/query parameters/fragments, root and login fallbacks, duplicate values, unsafe schemes, and both unsuccessful and rejected post-login navigation.
🤖 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 `@src/app/login/login.component.ts` around lines 241 - 252, Add regression tests around LoginComponent.getReturnUrl and the post-login navigation flow covering nested paths with query parameters or fragments, root and login fallbacks, duplicate returnUrl values, unsafe schemes, and navigation that is both unsuccessful and rejected. Verify each case resolves to the expected safe fallback or navigation outcome.
247-248: 📐 Maintainability & Code Quality | 🔵 TrivialUse
queryParamMapto keepreturnUrlstrictly typed.
this.route.snapshot.queryParamsis typed as a genericParamsmap (string => any), soreturnUrlloses strict string typing. Usethis.route.snapshot.queryParamMap.getAll('returnUrl')and keep the existing runtime validation/array-handling before selecting the single value.[loweffort_and_high_reward]
🤖 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 `@src/app/login/login.component.ts` around lines 247 - 248, Update the returnUrl retrieval in the login component to use this.route.snapshot.queryParamMap.getAll('returnUrl') instead of the generic queryParams map. Preserve the existing runtime validation and array-handling, then select the single validated value before applying the current URL checks.Source: Coding guidelines
🤖 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.
Nitpick comments:
In `@src/app/login/login.component.ts`:
- Around line 241-252: Add regression tests around LoginComponent.getReturnUrl
and the post-login navigation flow covering nested paths with query parameters
or fragments, root and login fallbacks, duplicate returnUrl values, unsafe
schemes, and navigation that is both unsuccessful and rejected. Verify each case
resolves to the expected safe fallback or navigation outcome.
- Around line 247-248: Update the returnUrl retrieval in the login component to
use this.route.snapshot.queryParamMap.getAll('returnUrl') instead of the generic
queryParams map. Preserve the existing runtime validation and array-handling,
then select the single validated value before applying the current URL checks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 896bb3ed-066d-4693-b5c0-14b2f2404012
📒 Files selected for processing (2)
src/app/core/authentication/authentication.guard.tssrc/app/login/login.component.ts
|
Needs to be updated to match base branch too |
f8c28ec to
cab784d
Compare
There was a problem hiding this comment.
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 `@src/app/login/login.component.ts`:
- Around line 246-251: Update getReturnUrl to reject query parameters beginning
with “//” in addition to the existing validation, before returning the supplied
URL. Preserve the current “/login” rejection and fallback to “/” for all invalid
return URLs.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: cd5ba884-3730-499d-8537-a94a92369f69
📒 Files selected for processing (2)
src/app/core/authentication/authentication.guard.tssrc/app/login/login.component.ts
cab784d to
97a83fa
Compare
Preserve the attempted URL as a `returnUrl` query param when the authentication guard redirects an unauthenticated user to the login page, and navigate back to it after a successful login (falling back to the dashboard for missing/unsafe values). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
97a83fa to
9cd08a7
Compare
3 participants
Preserve the attempted URL as a
returnUrlquery param when the authentication guard redirects an unauthenticated user to the login page, and navigate back to it after a successful login (falling back to the dashboard for missing/unsafe values).Description
Describe the changes made and why they were made instead of how they were made. List any dependencies that are required for this change.
Related issues and discussion
#{Issue Number}
Screenshots, if any
Checklist
Please make sure these boxes are checked before submitting your pull request - thanks!
If you have multiple commits please combine them into one commit by squashing them.
Read and understood the contribution guidelines at
web-app/.github/CONTRIBUTING.md.Summary by CodeRabbit