[Feature] Issue-976 per device sessions#991
Conversation
… deactivateOtherSessions, updateLastActive methods
…n management methods
- Created DeviceSessionCard to view, revoke, and manage active sessions across devices. - Integrated DeviceSessionCard into the main Dashboard view. - Added a global Axios interceptor in api_instance.js to handle 401 Unauthorized errors, automatically clearing local storage and redirecting to the login page. - Fixed missing prop type warnings for <CardWrapper> (Dashboard) and <Button> (CurrentPlan). - Updated authMiddleware to explicitly command the browser to delete stale httpOnly session cookies whenever an invalid or revoked session is detected, significantly improving the security workflow across devices. - Added dummy use to bypass husky pre commit error
Modernized built-in imports by switching to the node:http prefix for standard compliance. Implemented optional chaining in auth controllers and API configurations for cleaner null-safety. Refactored array access in UserSessionService to use the modern .at(-1) syntax. Switched from window to globalThis in the UI api_instance to improve portability. Cleaned up unused imports and redundant logic in session management tests to reduce noise.
There was a problem hiding this comment.
Pull request overview
This PR adds per-device session management across the backend and UI, enabling users to view active sessions (device/OS/IP/last active) and revoke individual sessions or sign out from other/all devices.
Changes:
- UI: Adds a “Device Sessions” dashboard card for listing and revoking sessions.
- Backend: Introduces per-device session metadata (UA parsing, IP, lastActiveAt), session listing/revocation endpoints, and middleware “touch” updates.
- Platform behavior: Adds a global Axios 401 interceptor to force re-authentication and extends/updates Jest coverage for the new endpoints.
Reviewed changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/ui/src/page/dashboard/Dashboard.jsx | Adds a “Device Sessions” section to the dashboard. |
| packages/ui/src/components/devicesession/DeviceSessionCard.jsx | Implements session list UI and revoke/signout-others actions. |
| packages/ui/src/components/devicesession/DeviceSessionCard.module.css | Styles for the device session dashboard card. |
| packages/ui/src/components/currentplan/CurrentPlan.jsx | Sets Button variant to align with required Button API. |
| packages/ui/src/api/api_instance.js | Adds global 401 interceptor for forced logout behavior. |
| packages/app/utils/testconstants.js | Adds test endpoint constants for session management APIs. |
| packages/app/utils/mocks.js | Extends mock sessions with deviceInfo/ip/lastActiveAt and adds a second session ID. |
| packages/app/utils/constants.js | Adds session-related Messages and exports MAX_SESSIONS_PER_USER. |
| packages/app/services/userSession.js | Adds UA parsing, session cap enforcement, revoke/list/touch session methods. |
| packages/app/routes/auth.js | Registers session management routes under /auth. |
| packages/app/repositories/userSession.js | Adds per-user active session queries and scoped revocation/update helpers. |
| packages/app/package.json | Adds ua-parser-js dependency. |
| packages/app/models/userSession.js | Extends schema with deviceInfo/ip/lastActiveAt and adds an index for lookups. |
| packages/app/middlewares/auth.js | Clears cookies for invalid sessions and throttles lastActiveAt updates. |
| packages/app/controllers/auth.js | Always creates per-device sessions; adds list/revoke/signout-others/all controllers. |
| packages/app/tests/controllers/auth/signin.js | Updates signin tests for the new “always create session” behavior. |
| packages/app/tests/controllers/auth/sessions.js | Adds test coverage for session listing and revocation endpoints. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| return res.status(401).json({ | ||
| error: STATUS_CODES[403], | ||
| message: "Invalid credentials", | ||
| statusCode: 403, | ||
| }); |
There was a problem hiding this comment.
The response here is inconsistent: it returns HTTP 401 but sets error: STATUS_CODES[403] and statusCode: 403. This will confuse clients and can break error handling logic. Align the HTTP status, error, and statusCode values (either all 401 or all 403).
| res.cookie("mfaSessionId", mfaSession.sessionId, { | ||
| httpOnly: true, | ||
| sameSite: "strict", | ||
| expires: mfaSession.expiresAt, | ||
| domain: isProduction ? ".openlogo.fyi" : "localhost", | ||
| // sameSite: "strict", | ||
| // expires: mfaSession.expiresAt, | ||
| // domain: getIsProduction() ? ".openlogo.fyi" : "localhost", | ||
| }); |
There was a problem hiding this comment.
mfaSessionId is being set with only httpOnly: true because sameSite, expires, and domain are commented out. This changes cookie scope/expiry (session cookie instead of expiring with mfaSession.expiresAt) and will likely prevent res.clearCookie('mfaSessionId', { sameSite, domain, ... }) from matching later in siginWithMFAController, leaving stale MFA cookies behind. Restore consistent cookie options for mfaSessionId (and ideally reuse a shared cookie-options helper) so set/clear use the same attributes.
| // Mask IP address like 192.168.x.x | ||
| let maskedIp = session.ipAddress; | ||
| if (maskedIp?.includes(".")) { | ||
| const parts = maskedIp.split("."); | ||
| if (parts.length === 4) { | ||
| maskedIp = `${parts[0]}.${parts[1]}.x.x`; | ||
| } | ||
| } |
There was a problem hiding this comment.
IP masking currently only handles IPv4 dotted notation. req.ip / stored ipAddress can be IPv6 (or IPv4-mapped IPv6 like ::ffff:127.0.0.1), which will be returned unmasked and may expose more PII than intended. Extend masking to handle IPv6 (and IPv4-mapped IPv6) or omit IPs entirely from the response.
| const { userId } = req.userData; | ||
| const { sessionId } = req.params; | ||
| const currentSessionId = req.cookies.sessionId; | ||
|
|
There was a problem hiding this comment.
sessionId from req.params is used without validation. Since the codebase already uses SESSION_ID_REGEX for cookies, validate req.params.sessionId similarly and return 400 for malformed IDs to avoid unnecessary DB work and make the API behavior consistent.
| if (typeof sessionId !== "string" || !SESSION_ID_REGEX.test(sessionId)) { | |
| return res.status(400).json({ | |
| error: STATUS_CODES[400], | |
| message: STATUS_CODES[400], | |
| statusCode: 400, | |
| }); | |
| } |
|
|
||
| /** | ||
| * Maximum number of concurrent active sessions allowed per user. | ||
| * When this limit is reached, the oldest session is automatically revoked | ||
| * before a new one is created. | ||
| */ | ||
| const MAX_SESSIONS_PER_USER = 5; | ||
|
|
There was a problem hiding this comment.
MAX_SESSIONS_PER_USER is hard-coded here even though it’s also defined/exported in utils/constants.js. This duplication risks the service limit drifting from the shared constant. Prefer importing MAX_SESSIONS_PER_USER from ../utils/constants and using that single source of truth.
| /** | |
| * Maximum number of concurrent active sessions allowed per user. | |
| * When this limit is reached, the oldest session is automatically revoked | |
| * before a new one is created. | |
| */ | |
| const MAX_SESSIONS_PER_USER = 5; | |
| const { MAX_SESSIONS_PER_USER } = require("../utils/constants"); |
| if (globalThis.location.pathname !== "/") { | ||
| localStorage.clear(); |
There was a problem hiding this comment.
The 401 interceptor only clears localStorage when location.pathname !== '/'. If a 401 happens while already on / (e.g., background polling, home page calls), the app will keep stale local state. Consider always clearing local storage on 401, and only guard the redirect to avoid loops.
| if (globalThis.location.pathname !== "/") { | |
| localStorage.clear(); | |
| localStorage.clear(); | |
| if (globalThis.location.pathname !== "/") { |
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
…tings tab Made-with: Cursor
…ings profile Made-with: Cursor
…e/per-device-sessions
…ature/per-device-sessions
kadamsahil2511
changed the title
kadamsahil2511
changed the title
kadamsahil2511
changed the title
kadamsahil2511
changed the title
- Memoize useApi makeRequest/fetchRequest with a config ref so dependency arrays stay correct without new identities every render. - Resolve react-hooks/exhaustive-deps in Catalog, Graph, DeviceSessionCard, AuthContext, Dashboard, and OperatorDashboard; document intentional exceptions in ResetPassword and CreateLogo mount-only effects. - Trim trailing hyphens in sanitizeFilePath with a linear scan instead of a regex flagged for ReDoS risk. - Wrap ProfileInfo tests with ToastProvider for DeviceSessionCard. - Refresh pending/success badge styles in TwoFactorAuth and UserSettings. - Ignore .pnpm-store/ and remove packages/app/globalConfig.json. Made-with: Cursor
Made-with: Cursor
… case Added a default case to the headerCopy function to ensure proper handling of the "2fa" tab, improving the clarity and functionality of the user settings component.
Reintroduced the default case in the headerCopy function to ensure consistent handling of the "2fa" tab, enhancing the user settings component's functionality.
…rSettings Reintroduced the "2fa" case in the headerCopy function to ensure proper display of security settings, enhancing the user experience in the user settings component.
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 60 out of 61 changed files in this pull request and generated 9 comments.
Comments suppressed due to low confidence (2)
packages/ui/src/components/auth/Signin.jsx:17
redirectAfterLoginis being logged to the console. This looks like leftover debug logging and will pollute production logs in the browser devtools. Please remove theconsole.log.
packages/ui/src/components/auth/Signin.jsx:146onCloseis optional inSignIn.propTypes, but it’s called unconditionally on successful sign-in. IfSignInis ever used outside a modal (noonCloseprovided), this will throw. Either makeonCloserequired, provide a default no-op, or guard the call (if (onClose) onClose()).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const imagesData = images.map((image) => { | ||
| const imageData = image.data(); | ||
| const imagePath = `${image.extension}/${sanitizeFilePath(image.company_name)}.${image.extension}`; | ||
| const signedUrlResult = cloudFrontSignedURL(`/${imagePath}`); | ||
| imageData.imageUrl = signedUrlResult.success | ||
| ? signedUrlResult.data | ||
| : null; | ||
| return imageData; |
There was a problem hiding this comment.
The generated imagePath uses sanitizeFilePath(image.company_name), which lowercases the company name. Elsewhere (e.g., catalog upload/presign) the S3/CloudFront key uses the original uppercased companyName extracted from companyUri.toUpperCase(), so this will likely generate signed URLs for a non-existent object (case-sensitive key mismatch). Use the same key derivation as upload (${extension}/${company_name}.${extension} with the stored company_name), or make sanitization preserve the exact stored casing/format used for uploads.
| # Code Review: PR #959 - Settings Page & 2FA | ||
|
|
||
| ## Commits Reviewed | ||
|
|
||
| 10 commits from Tarun Aditya and MukeshAbhi across 23 files (+1476/-684) | ||
|
|
||
| --- | ||
|
|
||
| ## Critical Bugs | ||
|
|
||
| ### 1. `TwoFactorAuth.jsx:27-33` - verifyRequest uses stale state ✅ FIXED | ||
|
|
||
| Removed `data` from useApi config. Now passing `token` dynamically in `handleFinishSetup`. ✅ FIXED | ||
|
|
||
| --- | ||
|
|
||
| ### 2. `Pin.jsx:41` - onClose called without null check ✅ FIXED | ||
|
|
||
| Made `PropTypes.func.isRequired` so missing `onClose` will show a warning. | ||
|
|
||
| --- | ||
|
|
||
| ### 3. `Pin.jsx:60-63` - Backspace doesn't clear filled input ✅ FIXED | ||
|
|
||
| Now clears current digit on backspace before moving focus. ✅ FIXED | ||
|
|
||
| --- | ||
|
|
||
| ## Medium Severity | ||
|
|
||
| ### 4. `TwoFactorAuth.jsx:10` - Incorrect initial mode state ✅ FIXED | ||
|
|
||
| Mode now initializes to `"INITIAL"` instead of `"VERIFIED"`. | ||
|
|
||
| --- | ||
|
|
||
| ### 5. `TwoFactorAuth.jsx:65-76` - No CSRF protection for enabling 2FA ⚠️ STILL TRUE | ||
|
|
||
| Enabling 2FA requires no confirmation (unlike disabling which requires password). An attacker could trick a logged-in user into enabling 2FA via a malicious page. Consider requiring password confirmation or re-authentication for security-sensitive operations. | ||
|
|
||
| --- | ||
|
|
||
| ## Minor Issues | ||
|
|
||
| ### 6. Debug logs left in production code ✅ FIXED | ||
|
|
||
| - `Signin.jsx:16` - ✅ Removed | ||
| - `Pin.jsx:31` - ✅ Removed | ||
|
|
||
| --- | ||
|
|
||
| ### 7. Tests removed but not replaced ⚠️ STILL TRUE | ||
|
|
||
| `UserInfo.test.jsx` and `Signin.test.jsx` were deleted. The new MFA-enabled SignIn and new Pin component have no tests. SonarQube flagged "0.0% Coverage on New Code". | ||
|
|
||
| --- | ||
|
|
||
| ## Summary | ||
|
|
||
| | Severity | Issue | Status | | ||
| | -------- | ---------------------------- | ------------- | | ||
| | Critical | 1. verifyRequest stale state | ✅ FIXED | | ||
| | Critical | 2. onClose null check | ✅ FIXED | | ||
| | Critical | 3. Backspace doesn't clear | ✅ FIXED | | ||
| | Medium | 4. Incorrect initial mode | ✅ FIXED | | ||
| | Medium | 5. No CSRF on 2FA enable | ⚠️ STILL TRUE | | ||
| | Minor | 6. Debug logs | ✅ FIXED | | ||
| | Minor | 7. Tests removed | ⚠️ STILL TRUE | | ||
|
|
||
| **Remaining issues:** | ||
|
|
||
| - **#5** - Security concern (not a bug, but worth addressing) | ||
| - **#7** - Tests not replaced |
There was a problem hiding this comment.
This file appears to be personal/internal review tracking (e.g., "own issues found"), not product documentation. Keeping it in the repository (especially at the root) can confuse contributors and bloat the codebase. Consider removing it from the PR or relocating it under docs/ with a clearer ownership/purpose.
| # Code Review: PR #959 - Settings Page & 2FA | |
| ## Commits Reviewed | |
| 10 commits from Tarun Aditya and MukeshAbhi across 23 files (+1476/-684) | |
| --- | |
| ## Critical Bugs | |
| ### 1. `TwoFactorAuth.jsx:27-33` - verifyRequest uses stale state ✅ FIXED | |
| Removed `data` from useApi config. Now passing `token` dynamically in `handleFinishSetup`. ✅ FIXED | |
| --- | |
| ### 2. `Pin.jsx:41` - onClose called without null check ✅ FIXED | |
| Made `PropTypes.func.isRequired` so missing `onClose` will show a warning. | |
| --- | |
| ### 3. `Pin.jsx:60-63` - Backspace doesn't clear filled input ✅ FIXED | |
| Now clears current digit on backspace before moving focus. ✅ FIXED | |
| --- | |
| ## Medium Severity | |
| ### 4. `TwoFactorAuth.jsx:10` - Incorrect initial mode state ✅ FIXED | |
| Mode now initializes to `"INITIAL"` instead of `"VERIFIED"`. | |
| --- | |
| ### 5. `TwoFactorAuth.jsx:65-76` - No CSRF protection for enabling 2FA ⚠️ STILL TRUE | |
| Enabling 2FA requires no confirmation (unlike disabling which requires password). An attacker could trick a logged-in user into enabling 2FA via a malicious page. Consider requiring password confirmation or re-authentication for security-sensitive operations. | |
| --- | |
| ## Minor Issues | |
| ### 6. Debug logs left in production code ✅ FIXED | |
| - `Signin.jsx:16` - ✅ Removed | |
| - `Pin.jsx:31` - ✅ Removed | |
| --- | |
| ### 7. Tests removed but not replaced ⚠️ STILL TRUE | |
| `UserInfo.test.jsx` and `Signin.test.jsx` were deleted. The new MFA-enabled SignIn and new Pin component have no tests. SonarQube flagged "0.0% Coverage on New Code". | |
| --- | |
| ## Summary | |
| | Severity | Issue | Status | | |
| | -------- | ---------------------------- | ------------- | | |
| | Critical | 1. verifyRequest stale state | ✅ FIXED | | |
| | Critical | 2. onClose null check | ✅ FIXED | | |
| | Critical | 3. Backspace doesn't clear | ✅ FIXED | | |
| | Medium | 4. Incorrect initial mode | ✅ FIXED | | |
| | Medium | 5. No CSRF on 2FA enable | ⚠️ STILL TRUE | | |
| | Minor | 6. Debug logs | ✅ FIXED | | |
| | Minor | 7. Tests removed | ⚠️ STILL TRUE | | |
| **Remaining issues:** | |
| - **#5** - Security concern (not a bug, but worth addressing) | |
| - **#7** - Tests not replaced | |
| # Repository note | |
| This repository should not store personal or PR-specific review tracking in committed files. | |
| Please keep review notes, issue triage, and per-PR status updates in the pull request discussion, issue tracker, or dedicated project documentation maintained for that purpose. |
| <p className={styles["session-details"]}> | ||
| IP: {session.ipAddress} • Last active:{" "} | ||
| {session.lastActiveAt | ||
| ? formatDate(session.lastActiveAt) | ||
| : "Unknown"} | ||
| </p> |
There was a problem hiding this comment.
The UI renders IP: {session.ipAddress}, but the sessions API response constructed in listSessionsController doesn't include ipAddress, and the session model/schema doesn't define it. This will show IP: undefined for all sessions. Either add ipAddress to the session persistence + API response, or remove the IP line from the UI until the backend supports it.
| * UserSession Model | ||
| * Represents an active authenticated session for a device. | ||
| * Tracks session activity, TTL, and soft logout. | ||
| * Represents an active authenticated session for a specific device/browser. | ||
| * Tracks session activity, device metadata, IP address, TTL, and soft logout. | ||
| * 'isActive' marks whether the session is currently valid. |
There was a problem hiding this comment.
The model docstring says sessions track ipAddress, but the schema doesn't define an ipAddress field. This also blocks the UI/API from returning an IP for per-device sessions. Add an ipAddress field to the schema (and ensure it’s populated when creating/touching sessions), or update the docstring to match the actual stored fields.
| if (success) { | ||
| setPin(new Array(6).fill("")); | ||
| setIsAuthenticated(true); | ||
| navigate("/dashboard"); | ||
| onClose(); |
There was a problem hiding this comment.
Pin always navigates to /dashboard on success, which ignores redirectAfterLogin supported by SignIn. This will break flows where the caller expects to redirect somewhere else after authentication. Pass redirectAfterLogin into Pin (or move navigation back into SignIn after MFA verification) so redirects are consistent.
| <input | ||
| key={input.id} | ||
| ref={(el) => (inputsRef.current[input.index] = el)} | ||
| type="text" | ||
| inputMode="numeric" | ||
| maxLength="1" | ||
| className={styles["pin-input"]} | ||
| value={pin[input.index]} | ||
| onChange={(e) => handleChange(e.target.value, input.index)} | ||
| onKeyDown={(e) => handleKeyDown(e, input.index)} | ||
| /> |
There was a problem hiding this comment.
The PIN inputs have no associated label/accessible name (no aria-label/aria-labelledby), which makes the MFA prompt difficult to use with screen readers. Add an accessible label for each digit input (e.g., "Digit 1 of 6"), and consider autoComplete="one-time-code" to improve UX on mobile/password managers.
| # Copilot Reviews for Settings Page PR #959 | ||
|
|
||
| All review comments from PR #959 are documented below: | ||
|
|
||
| --- | ||
|
|
||
| ## 1. Button.module.css (Line 18) | ||
|
|
||
| **Comment:** The disabled button styling no longer visually differentiates disabled vs enabled states (opacity was removed) and `color: var(--white)` becomes dark in `[data-theme="dark"]` because `--white` is overridden. Consider using `--white-fixed` for disabled text (and for primary/danger variants) and reintroducing a visual disabled treatment (e.g., opacity or muted background) so disabled buttons remain obvious, especially on touch devices. | ||
|
|
||
| **URL:** https://github.com/TeamShiksha/openlogo/pull/959#discussion_r2950801891 | ||
|
|
||
| --- | ||
|
|
||
| ## 2. TwoFactorAuth.jsx - Perpetual Loading State (Line 58) | ||
|
|
||
| **Comment:** `isLoading` is set to `true` in `checkMFAStatus` but never set back to `false`, so the component can remain in a perpetual loading state. Add a `finally` (or ensure both success/error paths) to reset `isLoading` after the request completes. | ||
|
|
||
| **URL:** https://github.com/TeamShiksha/openlogo/pull/959#discussion_r2950801913 | ||
|
|
||
| --- | ||
|
|
||
| ## 3. TwoFactorAuth.jsx - Incorrect Initial Mode (Line 10) | ||
|
|
||
| **Comment:** Initial state sets `mode` to `"VERIFIED"` while `isMFAEnabled` defaults to `false`, which results in an empty body until the status request runs. Initialize `mode` to `"INITIAL"` (or introduce an explicit loading mode) so the UI renders deterministically on first paint. | ||
|
|
||
| **URL:** https://github.com/TeamShiksha/openlogo/pull/959#discussion_r2950801931 | ||
|
|
||
| --- | ||
|
|
||
| ## 4. TwoFactorAuth.module.css - Typo in Class Name (Line 341) | ||
|
|
||
| **Comment:** Typo in class name `disableCancleBtn` ("Cancle"). Rename to `disableCancelBtn` (and update references) to avoid propagating misspellings into public/consumed CSS module APIs. | ||
|
|
||
| **URL:** https://github.com/TeamShiksha/openlogo/pull/959#discussion_r2950801944 | ||
|
|
||
| --- | ||
|
|
||
| ## 5. UserSettings.jsx - Missing CSS Classes (Line 62) | ||
|
|
||
| **Comment:** `styles.heading` and `styles.subtext` are referenced here, but `UserSettings.module.css` doesn't define `.heading` or `.subtext`, so these classNames will be `undefined` and the "Coming Soon" UI won't be styled as intended. Either add the missing CSS classes or switch to existing class names in the module. | ||
|
|
||
| **URL:** https://github.com/TeamShiksha/openlogo/pull/959#discussion_r2950801950 | ||
|
|
||
| --- | ||
|
|
||
| ## 6. Pin.jsx - Optional onClose Called Unconditionally (Line 41) | ||
|
|
||
| **Comment:** `onClose` is optional in `propTypes`, but it's called unconditionally on successful verification. If a caller omits `onClose`, this will throw at runtime. Either make `onClose` required or guard the call (or provide a default no-op). | ||
|
|
||
| **URL:** https://github.com/TeamShiksha/openlogo/pull/959#discussion_r2950801959 | ||
|
|
||
| --- | ||
|
|
||
| ## 7. Signin.jsx - Redirect After Login Hardcoded (Line 253) | ||
|
|
There was a problem hiding this comment.
These markdown files look like internal PR review notes rather than end-user/developer documentation. Committing them to the repo root will add long-term noise/maintenance overhead. Consider removing them from the PR, or moving them under docs/ with a clear purpose (or keeping them in the PR discussion instead).
| # Copilot Reviews for Settings Page PR #959 | |
| All review comments from PR #959 are documented below: | |
| --- | |
| ## 1. Button.module.css (Line 18) | |
| **Comment:** The disabled button styling no longer visually differentiates disabled vs enabled states (opacity was removed) and `color: var(--white)` becomes dark in `[data-theme="dark"]` because `--white` is overridden. Consider using `--white-fixed` for disabled text (and for primary/danger variants) and reintroducing a visual disabled treatment (e.g., opacity or muted background) so disabled buttons remain obvious, especially on touch devices. | |
| **URL:** https://github.com/TeamShiksha/openlogo/pull/959#discussion_r2950801891 | |
| --- | |
| ## 2. TwoFactorAuth.jsx - Perpetual Loading State (Line 58) | |
| **Comment:** `isLoading` is set to `true` in `checkMFAStatus` but never set back to `false`, so the component can remain in a perpetual loading state. Add a `finally` (or ensure both success/error paths) to reset `isLoading` after the request completes. | |
| **URL:** https://github.com/TeamShiksha/openlogo/pull/959#discussion_r2950801913 | |
| --- | |
| ## 3. TwoFactorAuth.jsx - Incorrect Initial Mode (Line 10) | |
| **Comment:** Initial state sets `mode` to `"VERIFIED"` while `isMFAEnabled` defaults to `false`, which results in an empty body until the status request runs. Initialize `mode` to `"INITIAL"` (or introduce an explicit loading mode) so the UI renders deterministically on first paint. | |
| **URL:** https://github.com/TeamShiksha/openlogo/pull/959#discussion_r2950801931 | |
| --- | |
| ## 4. TwoFactorAuth.module.css - Typo in Class Name (Line 341) | |
| **Comment:** Typo in class name `disableCancleBtn` ("Cancle"). Rename to `disableCancelBtn` (and update references) to avoid propagating misspellings into public/consumed CSS module APIs. | |
| **URL:** https://github.com/TeamShiksha/openlogo/pull/959#discussion_r2950801944 | |
| --- | |
| ## 5. UserSettings.jsx - Missing CSS Classes (Line 62) | |
| **Comment:** `styles.heading` and `styles.subtext` are referenced here, but `UserSettings.module.css` doesn't define `.heading` or `.subtext`, so these classNames will be `undefined` and the "Coming Soon" UI won't be styled as intended. Either add the missing CSS classes or switch to existing class names in the module. | |
| **URL:** https://github.com/TeamShiksha/openlogo/pull/959#discussion_r2950801950 | |
| --- | |
| ## 6. Pin.jsx - Optional onClose Called Unconditionally (Line 41) | |
| **Comment:** `onClose` is optional in `propTypes`, but it's called unconditionally on successful verification. If a caller omits `onClose`, this will throw at runtime. Either make `onClose` required or guard the call (or provide a default no-op). | |
| **URL:** https://github.com/TeamShiksha/openlogo/pull/959#discussion_r2950801959 | |
| --- | |
| ## 7. Signin.jsx - Redirect After Login Hardcoded (Line 253) | |
| # Settings Page Implementation Checklist | |
| This document captures durable engineering follow-ups for the settings page and related authentication flows. | |
| ## UI and accessibility | |
| - Ensure disabled buttons remain visually distinct from enabled states across themes. | |
| - Prefer stable color tokens such as `--white-fixed` when theme-specific variables can change contrast unexpectedly. | |
| - Keep disabled treatments obvious on touch devices by using muted styling, opacity, or both. | |
| ## Loading and state management | |
| - Always reset `isLoading` after asynchronous status checks complete, including error paths. | |
| - Use `finally` or equivalent control flow to avoid perpetual loading states. | |
| - Initialize view state consistently so the UI renders deterministically on first paint. | |
| ## Naming and styling consistency | |
| - Fix misspelled CSS module class names before they become part of consumed styling APIs. | |
| - Ensure every referenced CSS module class is defined, or update components to use existing class names. | |
| ## Component safety | |
| - Guard optional callback props such as `onClose` before invoking them, or provide a default no-op. | |
| - Make props required only when the component cannot function safely without them. | |
| ## Authentication flow behavior | |
| - Avoid hardcoded post-login redirects when route behavior should depend on user state or app configuration. | |
| - Review settings-related authentication screens for predictable loading, error, and success transitions. |
| .current-plan { | ||
| color: rgb(17 24 39); | ||
| font-size: var(--large-text); | ||
| line-height: 28px; | ||
| font-weight: 600; | ||
| margin: 0px 0px 4px; | ||
| padding-top: 0; | ||
| } |
There was a problem hiding this comment.
CurrentPlan.jsx still uses styles["plan-container"], but this CSS module removes/renames .plan-container (replaced by .current-plan). This will make the wrapper class undefined and likely break the Current Plan layout. Either restore .plan-container (preferred for backward compatibility) or update the component to use the new class name consistently.
| const formattedSessions = sessions.map((session) => { | ||
| return { | ||
| id: session.sessionId, | ||
| deviceInfo: session.deviceInfo, | ||
| createdAt: session.createdAt, | ||
| lastActiveAt: session.lastActiveAt, | ||
| isCurrent: session.sessionId === currentSessionId, | ||
| }; |
There was a problem hiding this comment.
listSessionsController formats sessions for the UI but omits ipAddress. Since the frontend displays the IP per session, the response should include it (once stored in the session model), or the frontend should be adjusted to not expect it.
Description
This PR is with respect to issue #976 introduces full Per-Device Session Management, allowing users to securely monitor and control their active sessions across different browsers and devices.
Key Implementation by @kadamsahil2511 Details:
DeviceSessionCard): Added a dynamic dashboard component displaying active sessions complete with device, OS, and IP address logging. Users can selectively revoke individual sessions or abruptly "Sign Out From All Other Devices".api_instance.js): Configured a global Axios interceptor. If any API call returns a401 Unauthorized(indicating the session was forcefully killed on another device), the application instantly catches it, purgeslocalStorage, and safely redirects the user back to the login screen.authMiddleware.js): Updated the primary authentication middleware to aggressively invokeclearCookieagainst stalehttpOnlysessions the exact moment they are marked invalid, protecting against persistent stray cookies.Key Implementation by @DeepAkdotcom Details:
userSessionmodel to trackdeviceInfo,ipAddress, andlastActiveAtfor per-device management.ua-parser-jsto parse browser/OS and device types from user agents.GET /auth/sessions).DELETE /auth/sessions/:sessionId).POST /auth/signout/others).authMiddlewareto monitor user activity without impacting performance.__tests__/controllers/auth/sessions.jsto ensure 100% logic coverage for multi-device management.Video File
https://youtu.be/oKagt6vK0_w
Checklist