feat: session expired interceptor with login redirection#2379
Open
abdellah257 wants to merge 1 commit into
Open
feat: session expired interceptor with login redirection#2379abdellah257 wants to merge 1 commit into
abdellah257 wants to merge 1 commit into
Conversation
Contributor
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- In the
sessionTimeout$effect, the variousclear*StateAction()calls are just being invoked and their return values ignored; instead, return or dispatch these actions from the effect so the relevant slices actually get cleared (e.g., by returning aconcat/mergeof multiple actions or usingmapto an array). - The
AuthInterceptorcurrently importsSubscription,selectProfile, and injectsNgZonewithout using them; removing these unused imports/dependencies will keep the interceptor simpler and avoid confusion. - There are several
console.logstatements in the login component andAuthInterceptor; consider removing them or switching to the existing logging mechanism to avoid noisy console output in production.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In the `sessionTimeout$` effect, the various `clear*StateAction()` calls are just being invoked and their return values ignored; instead, return or dispatch these actions from the effect so the relevant slices actually get cleared (e.g., by returning a `concat`/`merge` of multiple actions or using `map` to an array).
- The `AuthInterceptor` currently imports `Subscription`, `selectProfile`, and injects `NgZone` without using them; removing these unused imports/dependencies will keep the interceptor simpler and avoid confusion.
- There are several `console.log` statements in the login component and `AuthInterceptor`; consider removing them or switching to the existing logging mechanism to avoid noisy console output in production.
## Individual Comments
### Comment 1
<location path="src/app/state-management/effects/user.effects.ts" line_range="253-262" />
<code_context>
+ return this.actions$.pipe(
+ ofType(fromActions.sessionTimeoutAction),
+ filter(() => this.authService.isAuthenticated()),
+ switchMap(() => {
+ this.authService.clear();
+ clearDatasetsStateAction();
+ clearInstrumentsStateAction();
+ clearJobsStateAction();
+ clearLogbooksStateAction();
+ clearPoliciesStateAction();
+ clearProposalsStateAction();
+ clearPublishedDataStateAction();
+ clearSamplesStateAction();
+ return of(fromActions.logoutCompleteAction({ logoutURL: "/login" }));
+ }),
+ );
</code_context>
<issue_to_address>
**issue (bug_risk):** The clear*StateAction calls are never dispatched, so the feature states will not actually be cleared on session timeout.
Within an effect, simply calling these action creators has no effect—they must be returned from the stream or dispatched. Currently only `logoutCompleteAction` is emitted, so the other slices are never reset on timeout. Please update the effect to emit all the clear*StateAction actions (e.g. `concatMap`/`mergeMap` to `of(clearDatasetsStateAction(), ...)`) or dispatch them via the store so the state is actually cleared.
</issue_to_address>
### Comment 2
<location path="src/app/users/login/login.component.ts" line_range="134-135" />
<code_context>
+ sessionStorage.removeItem("postLoginRedirect");
+ }
+
+ console.log("[LOGIN] return url: ", this.returnUrl);
+
this.proceedSubscription = this.vm$
</code_context>
<issue_to_address>
**suggestion:** Consider removing or guarding this console.log to avoid noisy production logs.
Since this component runs on every login, logging here will generate a lot of noise in production. If you still need this during development, consider removing it before release or wrapping it in an environment check or configurable logger.
```suggestion
```
</issue_to_address>
### Comment 3
<location path="src/app/shared/interceptors/auth.interceptor.ts" line_range="31-44" />
<code_context>
+ const isSessionExpired =
+ error.status === 401 && error.error?.message === "SESSION_EXPIRED";
+
+ console.log("isSessionExpired", isSessionExpired);
+
+ if (isSessionExpired) {
+ const currentUrl = this.router.url;
+ sessionStorage.setItem("postLoginRedirect", currentUrl);
+
+ this.store.dispatch(sessionTimeoutAction());
+
+ console.log("currentUrl", currentUrl);
+ }
+
</code_context>
<issue_to_address>
**🚨 suggestion (security):** Debugging console logs in the interceptor may be too noisy and potentially leak sensitive routing information.
Since this interceptor runs on every failed HTTP request, these `console.log` statements can both flood the console and expose internal URLs. Please remove them or replace them with structured logging that’s appropriately filtered/disabled in production.
```suggestion
catchError((error: HttpErrorResponse) => {
const isSessionExpired =
error.status === 401 && error.error?.message === "SESSION_EXPIRED";
if (isSessionExpired) {
const currentUrl = this.router.url;
sessionStorage.setItem("postLoginRedirect", currentUrl);
this.store.dispatch(sessionTimeoutAction());
}
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
8356562 to
c9ff0a3
Compare
c9ff0a3 to
aecf848
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Redirect to login page at session expiration if an authentication with an expired token is attempted. then redirect back in case of successful login.
Motivation
403failures returning empty results (e.i: emptydatafiles, no datasets at filtering, ...)Changes:
auth.interceptor.tssessionTimeoutuser actionsession timeoutBackend changes:
401HTTP status errors withSESSION_EXPIREDmessage to be thrown from the backend yo perform the redirection.Tests included
Documentation
official documentation info
If you have updated the official documentation, please provide PR # and URL of the pages where the updates are included
Summary by Sourcery
Handle expired authentication sessions by redirecting users to the login page and back to their original route after successful re-authentication.
New Features:
Enhancements: