Conversation
Ayurshi-Singh
force-pushed
the
feature/Implement-logout
branch
from
dfd0f7d to
17d8924
Compare
WalkthroughThis change adds a tenant-scoped secure logout flow and related UI hook, moves CSRF token handling to per-call tenant lookup, and replaces a callback-based 401 handling with direct forbidden-state updates. It introduces a LOGGED_OUT error code and i18n strings, extends the forbidden-state model with an error title and logged-out handling, and adds an App controller sign-out handler that invokes Auth.secureLogout. Miscellaneous adjustments include a small XML comment fix, repositioning of Api.init, and non-functional formatting edits. 🚥 Pre-merge checks | ✅ 1✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@webapp/services/Auth.service.ts`:
- Around line 54-87: secureLogout currently only clears storage and never
informs the caller or redirects; update secureLogout to (1) properly check
response.ok using response.status (or response.ok) and include that in the
thrown Error if not ok, (2) call the existing postLogoutClearance() after a
successful logout, and (3) change secureLogout to return a boolean (or resolved
value) indicating success/failure so callers (e.g., the controller that
fire-and-forgets this promise) can clear UI state and navigate; also call
postLogoutClearance() in the catch path before returning false (or rethrow if
you prefer) and optionally perform a client redirect (e.g., window.location.href
= '/login') only after clearance when desired.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e2a760ba-67a9-408b-9b20-de57a1d108f3
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (4)
webapp/controller/App.controller.tswebapp/resources/fragments/UserInfoPopover.fragment.xmlwebapp/services/Api.service.tswebapp/services/Auth.service.ts
Ayurshi-Singh
force-pushed
the
feature/Implement-logout
branch
from
17d8924 to
8edb124
Compare
Closes:#14
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Ayurshi-Singh
force-pushed
the
feature/Implement-logout
branch
from
8edb124 to
c5cbb8d
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 3
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 76b800d1-6b92-40f4-86c5-a3005854a53c
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (10)
webapp/common/Constants.tswebapp/common/Helpers.tswebapp/controller/App.controller.tswebapp/controller/Forbidden.controller.tswebapp/i18n/i18n_en.propertieswebapp/resources/fragments/UserInfoPopover.fragment.xmlwebapp/services/Api.service.tswebapp/services/Auth.service.tswebapp/utils/ForbiddenState.tswebapp/view/Forbidden.view.xml
✅ Files skipped from review due to trivial changes (4)
- webapp/common/Constants.ts
- webapp/view/Forbidden.view.xml
- webapp/common/Helpers.ts
- webapp/i18n/i18n_en.properties
🚧 Files skipped from review as they are similar to previous changes (1)
- webapp/resources/fragments/UserInfoPopover.fragment.xml
| public onSignOutPress(): void { | ||
| void Auth.secureLogout(this.tenantId); |
There was a problem hiding this comment.
Use the active tenant ID for logout instead of this.tenantId.
On Line 320, this.tenantId can still be '' here, which makes Auth.secureLogout look up CSRF- instead of CSRF-{tenant}. That can force logout into fallback behavior instead of a valid tenant-scoped logout.
💡 Proposed fix
public onSignOutPress(): void {
- void Auth.secureLogout(this.tenantId);
+ const tenantId =
+ (this.twoWayModel.getProperty('/selectedTenant') as string) ||
+ this.tenantId ||
+ /#\/([^/]+)/.exec(window.location.hash)?.[1] ||
+ '';
+ void Auth.secureLogout(tenantId);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public onSignOutPress(): void { | |
| void Auth.secureLogout(this.tenantId); | |
| public onSignOutPress(): void { | |
| const tenantId = | |
| (this.twoWayModel.getProperty('/selectedTenant') as string) || | |
| this.tenantId || | |
| /#\/([^/]+)/.exec(window.location.hash)?.[1] || | |
| ''; | |
| void Auth.secureLogout(tenantId); | |
| } |
| static postLogoutClearance(): void { | ||
| localStorage.clear(); | ||
| sessionStorage.clear(); | ||
| } |
There was a problem hiding this comment.
Avoid clearing all Web Storage during logout.
localStorage.clear() and sessionStorage.clear() remove all keys for the origin, including data not owned by this app/module.
💡 Proposed fix
static postLogoutClearance(): void {
- localStorage.clear();
- sessionStorage.clear();
+ sessionStorage.removeItem(Auth.loginSessionStorageKey);
+
+ const appPrefixes = ['myapp_', 'kms_'];
+ for (let i = localStorage.length - 1; i >= 0; i--) {
+ const key = localStorage.key(i);
+ if (key && appPrefixes.some((prefix) => key.startsWith(prefix))) {
+ localStorage.removeItem(key);
+ }
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| static postLogoutClearance(): void { | |
| localStorage.clear(); | |
| sessionStorage.clear(); | |
| } | |
| static postLogoutClearance(): void { | |
| sessionStorage.removeItem(Auth.loginSessionStorageKey); | |
| const appPrefixes = ['myapp_', 'kms_']; | |
| for (let i = localStorage.length - 1; i >= 0; i--) { | |
| const key = localStorage.key(i); | |
| if (key && appPrefixes.some((prefix) => key.startsWith(prefix))) { | |
| localStorage.removeItem(key); | |
| } | |
| } | |
| } |
|
|
||
| private constructor() { | ||
| this.model = new JSONModel({ | ||
| errorTitle: '', |
There was a problem hiding this comment.
Reset /errorTitle when clearing forbidden state.
Since /errorTitle is now part of the model (Line 17), clearForbiddenState() should clear it too to avoid stale titles on future transitions.
💡 Proposed fix
public clearForbiddenState(): void {
+ this.model.setProperty('/errorTitle', '');
this.model.setProperty('/errorCode', '');
this.model.setProperty('/errorMessage', '');
this.model.setProperty('/loginButtonVisible', false);
this.model.setProperty('/isForbidden', false);
}
2 participants
Closes:#14
Summary by CodeRabbit
New Features
Bug Fixes
Refactor