feat(auth): Add telemetry for dormant and deactivated account login attempts#39798
Open
himanshu2006 wants to merge 2 commits intoRocketChat:developfrom
Open
feat(auth): Add telemetry for dormant and deactivated account login attempts#39798himanshu2006 wants to merge 2 commits intoRocketChat:developfrom
himanshu2006 wants to merge 2 commits intoRocketChat:developfrom
Conversation
Contributor
|
Looks like this PR is not ready to merge, because of the following issues:
Please fix the issues and try again If you have any trouble, please check the PR guidelines |
🦋 Changeset detectedLatest commit: 082d965 The changes in this PR will be included in the next version bump. This PR includes changesets to release 41 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThe changes enrich failed-login telemetry: log payloads now include derived error Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested labels
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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
added
type: feature
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
1 issue found across 4 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name=".changeset/blue-moose-know.md">
<violation number="1" location=".changeset/blue-moose-know.md:5">
P1: Changeset description does not match the PR's stated auth-telemetry feature. The changeset says 'perf(cron): Batch process temporary upload cleanup' but the PR is adding auth telemetry for dormant/deactivated account login attempts. Update the changeset text to match the actual feature being deployed.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
himanshu2006
force-pushed
the
feat/auth-dormant-telemetry
branch
from
2ef5ade to
cc38d92
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
apps/meteor/app/authentication/server/lib/logLoginAttempts.ts (2)
47-47: Remove inline comment per coding guidelines.As per coding guidelines for TypeScript files: "Avoid code comments in the implementation."
Suggested fix
- ...(reason && { reason }), // Include the error reason so the warning makes sense + ...(reason && { reason }),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/app/authentication/server/lib/logLoginAttempts.ts` at line 47, Remove the inline implementation comment from the object spread expression so the TypeScript code follows the guideline; locate the expression ...(reason && { reason }) inside logLoginAttempts (in apps/meteor/app/authentication/server/lib/logLoginAttempts.ts) and delete the trailing comment text ("// Include the error reason so the warning makes sense") leaving only the expression.
29-39: Inconsistent indentation: tabs vs spaces.Lines 30-39 use 4-space indentation while the rest of the file uses tabs. This creates visual inconsistency and may cause linter failures.
Proposed fix: normalize to tabs
let isDeactivated = false; - let daysInactive = 0; - const reason = login.error?.reason || login.error?.message; - - if (login.user) { - isDeactivated = login.user.active === false; - if (login.user.lastLogin) { - const msInactive = Date.now() - new Date(login.user.lastLogin).getTime(); - daysInactive = Math.floor(msInactive / (1000 * 60 * 60 * 24)); - } - } + let daysInactive = 0; + const reason = login.error?.reason || login.error?.message; + + if (login.user) { + isDeactivated = login.user.active === false; + if (login.user.lastLogin) { + const msInactive = Date.now() - new Date(login.user.lastLogin).getTime(); + daysInactive = Math.floor(msInactive / (1000 * 60 * 60 * 24)); + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/app/authentication/server/lib/logLoginAttempts.ts` around lines 29 - 39, Normalize the indentation in the shown block to use tabs (matching the rest of the file) so lines that declare and set isDeactivated, daysInactive, reason and the nested login.user checks use tabs rather than 4 spaces; locate the block inside the logLoginAttempts module (the section referencing login.user, isDeactivated, daysInactive, reason) and replace the 4-space indents with the file's tab characters to satisfy the linter and maintain consistent formatting.apps/meteor/server/cron/temporaryUploadsCleanup.ts (1)
15-15: Avoidanytype; use proper typing.The
currentBatchparameter should be typed with the actual document shape rather thanany[].Proposed fix
- const processBatch = async (currentBatch: any[]) => { + const processBatch = async (currentBatch: { _id: string }[]) => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/server/cron/temporaryUploadsCleanup.ts` at line 15, The parameter currentBatch in processBatch is typed as any[]; replace it with a concrete document type (e.g., TemporaryUpload or TemporaryUploadDoc) that matches the shape used inside the function (fields accessed there), by importing or declaring the interface and updating the signature to currentBatch: TemporaryUpload[] (or Array<TemporaryUploadDoc>); ensure the declared type includes optionality for any nullable fields used in processBatch so TypeScript errors are resolved and no any remains.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/meteor/app/authentication/server/lib/logLoginAttempts.ts`:
- Around line 47-49: The spreads create conflicting "warning" keys when both
isDeactivated and daysInactive>=180 are true; update the log construction in
logLoginAttempts to compute a single warning value first (e.g., const warning =
isDeactivated ? 'Login attempt on deactivated account' : (daysInactive >= 180 ?
'Login attempt on dormant account' : undefined)), then spread ...(warning && {
warning }) alongside ...(isDeactivated && { accountStatus: 'deactivated' }) and
...(reason && { reason }) so the deactivated warning is prioritized and not
overwritten.
---
Nitpick comments:
In `@apps/meteor/app/authentication/server/lib/logLoginAttempts.ts`:
- Line 47: Remove the inline implementation comment from the object spread
expression so the TypeScript code follows the guideline; locate the expression
...(reason && { reason }) inside logLoginAttempts (in
apps/meteor/app/authentication/server/lib/logLoginAttempts.ts) and delete the
trailing comment text ("// Include the error reason so the warning makes sense")
leaving only the expression.
- Around line 29-39: Normalize the indentation in the shown block to use tabs
(matching the rest of the file) so lines that declare and set isDeactivated,
daysInactive, reason and the nested login.user checks use tabs rather than 4
spaces; locate the block inside the logLoginAttempts module (the section
referencing login.user, isDeactivated, daysInactive, reason) and replace the
4-space indents with the file's tab characters to satisfy the linter and
maintain consistent formatting.
In `@apps/meteor/server/cron/temporaryUploadsCleanup.ts`:
- Line 15: The parameter currentBatch in processBatch is typed as any[]; replace
it with a concrete document type (e.g., TemporaryUpload or TemporaryUploadDoc)
that matches the shape used inside the function (fields accessed there), by
importing or declaring the interface and updating the signature to currentBatch:
TemporaryUpload[] (or Array<TemporaryUploadDoc>); ensure the declared type
includes optionality for any nullable fields used in processBatch so TypeScript
errors are resolved and no any remains.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e0011ce8-40cc-436e-af07-22520bf47c15
📒 Files selected for processing (4)
.changeset/blue-moose-know.md.changeset/serious-bugs-yell.mdapps/meteor/app/authentication/server/lib/logLoginAttempts.tsapps/meteor/server/cron/temporaryUploadsCleanup.ts
📜 Review details
⏰ 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). (1)
- GitHub Check: cubic · AI code reviewer
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation
Files:
apps/meteor/server/cron/temporaryUploadsCleanup.tsapps/meteor/app/authentication/server/lib/logLoginAttempts.ts
🧠 Learnings (13)
📓 Common learnings
Learnt from: amitb0ra
Repo: RocketChat/Rocket.Chat PR: 39647
File: apps/meteor/app/api/server/v1/users.ts:891-899
Timestamp: 2026-03-15T14:31:23.493Z
Learning: In RocketChat/Rocket.Chat, `IUser.inactiveReason` in `packages/core-typings/src/IUser.ts` is typed as `'deactivated' | 'pending_approval' | 'idle_too_long'` (optional, no `null`), but the database stores `null` for newly created users. The Typia-generated `$ref: '#/components/schemas/IUser'` schema therefore correctly rejects `null` for `inactiveReason`. This causes the test "should create a new user with default roles" to fail when response validation is active (TEST_MODE). The fix is to add `| null` to `inactiveReason` in core-typings and rebuild Typia schemas in a separate PR. Do not flag this test failure as a bug introduced by the users.create OpenAPI migration (PR `#39647`). Do not suggest inlining a custom schema to work around it, as migration rules require using `$ref` when a Typia schema exists.
📚 Learning: 2026-02-24T19:05:56.710Z
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 0
File: :0-0
Timestamp: 2026-02-24T19:05:56.710Z
Learning: Rocket.Chat repo context: When a workspace manifest on develop already pins a dependency version (e.g., packages/web-ui-registration → "rocket.chat/ui-contexts": "27.0.1"), a lockfile change in a feature PR that upgrades only that dependency’s resolution is considered a manifest-driven sync and can be kept, preferably as a small "chore: sync yarn.lock with manifests" commit.
Applied to files:
.changeset/serious-bugs-yell.md
📚 Learning: 2026-02-24T19:09:09.561Z
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 38974
File: apps/meteor/app/api/server/v1/im.ts:220-221
Timestamp: 2026-02-24T19:09:09.561Z
Learning: In RocketChat/Rocket.Chat OpenAPI migration PRs for apps/meteor/app/api/server/v1 endpoints, maintainers prefer to avoid any logic changes; style-only cleanups (like removing inline comments) may be deferred to follow-ups to keep scope tight.
Applied to files:
.changeset/serious-bugs-yell.md.changeset/blue-moose-know.md
📚 Learning: 2026-02-24T19:05:56.710Z
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 0
File: :0-0
Timestamp: 2026-02-24T19:05:56.710Z
Learning: In Rocket.Chat PRs, keep feature PRs free of unrelated lockfile-only dependency bumps; prefer reverting lockfile drift or isolating such bumps into a separate "chore" commit/PR, and always use yarn install --immutable with the Yarn version pinned in package.json via Corepack.
Applied to files:
.changeset/serious-bugs-yell.md
📚 Learning: 2026-01-17T01:51:47.764Z
Learnt from: tassoevan
Repo: RocketChat/Rocket.Chat PR: 38219
File: packages/core-typings/src/cloud/Announcement.ts:5-6
Timestamp: 2026-01-17T01:51:47.764Z
Learning: In packages/core-typings/src/cloud/Announcement.ts, the AnnouncementSchema.createdBy field intentionally overrides IBannerSchema.createdBy (object with _id and optional username) with a string enum ['cloud', 'system'] to match existing runtime behavior. This is documented as technical debt with a FIXME comment at apps/meteor/app/cloud/server/functions/syncWorkspace/handleCommsSync.ts:53 and should not be flagged as an error until the runtime behavior is corrected.
Applied to files:
.changeset/serious-bugs-yell.mdapps/meteor/server/cron/temporaryUploadsCleanup.ts
📚 Learning: 2026-03-16T21:50:37.589Z
Learnt from: amitb0ra
Repo: RocketChat/Rocket.Chat PR: 39676
File: .changeset/migrate-users-register-openapi.md:3-3
Timestamp: 2026-03-16T21:50:37.589Z
Learning: For changes related to OpenAPI migrations in Rocket.Chat/OpenAPI, when removing endpoint types and validators from rocket.chat/rest-typings (e.g., UserRegisterParamsPOST, /v1/users.register) document this as a minor changeset (not breaking) per RocketChat/Rocket.Chat-Open-API#150 Rule 7. Note that the endpoint type is re-exposed via a module augmentation .d.ts in the consuming package (e.g., packages/web-ui-registration/src/users-register.d.ts). In reviews, ensure the changeset clearly states: this is a non-breaking change, the major version should not be bumped, and the changeset reflects a minor version bump. Do not treat this as a breaking change during OpenAPI migrations.
Applied to files:
.changeset/serious-bugs-yell.md.changeset/blue-moose-know.md
📚 Learning: 2026-01-27T20:57:56.529Z
Learnt from: nazabucciarelli
Repo: RocketChat/Rocket.Chat PR: 38294
File: apps/meteor/server/hooks/sauMonitorHooks.ts:0-0
Timestamp: 2026-01-27T20:57:56.529Z
Learning: In Rocket.Chat, the `accounts.login` event listened to by DeviceManagementService is only broadcast when running in microservices mode (via DDPStreamer), not in monolith mode. The `Accounts.onLogin` hook in sauMonitorHooks.ts runs in monolith deployments. These are mutually exclusive deployment modes, so there's no duplication of event emissions between these two code paths.
Applied to files:
.changeset/serious-bugs-yell.md
📚 Learning: 2026-01-15T22:03:35.587Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 38071
File: apps/meteor/app/apps/server/bridges/listeners.ts:257-271
Timestamp: 2026-01-15T22:03:35.587Z
Learning: In the file upload pipeline (apps/meteor/app/apps/server/bridges/listeners.ts), temporary files are created by the server in the same filesystem, so symlinks between temp files are safe and don't require cross-filesystem fallbacks.
Applied to files:
apps/meteor/server/cron/temporaryUploadsCleanup.ts
📚 Learning: 2026-03-19T13:59:40.678Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 38357
File: apps/meteor/app/apps/server/converters/uploads.ts:45-49
Timestamp: 2026-03-19T13:59:40.678Z
Learning: In `apps/meteor/app/apps/server/converters/uploads.ts`, the `room` async handler in `convertToApp` uses non-null assertions (`upload.rid!` and `result!`) intentionally. The data flow guarantees that any upload reaching this point must have a `rid`; if it does not, throwing an error is the desired behavior (fail-fast / data integrity guard). Do not flag these non-null assertions as unsafe during code review.
Applied to files:
apps/meteor/server/cron/temporaryUploadsCleanup.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Ensure tests run reliably in parallel without shared state conflicts
Applied to files:
apps/meteor/server/cron/temporaryUploadsCleanup.ts
📚 Learning: 2026-03-11T18:17:53.972Z
Learnt from: dougfabris
Repo: RocketChat/Rocket.Chat PR: 39425
File: apps/meteor/client/lib/chats/flows/processMessageUploads.ts:112-119
Timestamp: 2026-03-11T18:17:53.972Z
Learning: In `apps/meteor/client/lib/chats/flows/processMessageUploads.ts`, when sending multiple file uploads, each file is confirmed via its own `/rooms.mediaConfirm/${rid}/${fileId}` call and produces a separate message. Only the first file's confirm payload carries the composed message text (`msg`); all subsequent files receive `msg: ''`. This one-message-per-file behavior is intentional by design — do not flag it as a bug or suggest batching into a single message.
Applied to files:
apps/meteor/server/cron/temporaryUploadsCleanup.ts
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In the Rocket.Chat repository, do not reference Biome lint rules in code review feedback. Biome is not used even if biome.json exists; only reference Biome rules if there is explicit, project-wide usage documented. For TypeScript files, review lint implications without Biome guidance unless the project enables Biome rules.
Applied to files:
apps/meteor/server/cron/temporaryUploadsCleanup.tsapps/meteor/app/authentication/server/lib/logLoginAttempts.ts
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In this repository (RocketChat/Rocket.Chat), Biome lint rules are not used even if a biome.json exists. When reviewing TypeScript files (e.g., packages/ui-voip/src/providers/useMediaSession.ts), ensure lint suggestions do not reference Biome-specific rules. Rely on general ESLint/TypeScript lint rules and project conventions instead.
Applied to files:
apps/meteor/server/cron/temporaryUploadsCleanup.tsapps/meteor/app/authentication/server/lib/logLoginAttempts.ts
🔇 Additional comments (4)
.changeset/serious-bugs-yell.md (1)
1-5: LGTM!The changeset correctly documents a patch-level enhancement for the failed login telemetry feature.
.changeset/blue-moose-know.md (1)
1-5: LGTM!The changeset correctly documents the performance improvement for the temporary uploads cleanup cron job.
apps/meteor/server/cron/temporaryUploadsCleanup.ts (2)
9-9: Cursor timeout concern is not applicable here.Line 7 already materializes the cursor into an array via
.toArray(), so line 9's iteration operates on the array, not an active cursor. There is no cursor timeout risk in this code path.> Likely an incorrect or invalid review comment.
11-16: Remove this review comment. The code does not implement concurrent batch deletions. The current implementation intemporaryUploadsCleanup.tsalready deletes files sequentially usingfor await, which aligns with the sequential deletion pattern you correctly identified inservice.ts.> Likely an incorrect or invalid review comment.
coderabbitai
bot
added
type: feature
himanshu2006
force-pushed
the
feat/auth-dormant-telemetry
branch
from
62af31c to
bd02473
Compare
coderabbitai
bot
added
type: feature
himanshu2006
force-pushed
the
feat/auth-dormant-telemetry
branch
from
cb65f58 to
c5ef0cd
Compare
Author
|
@coderabbitai resume |
Contributor
✅ Actions performedReviews resumed. |
coderabbitai
bot
removed
type: feature
himanshu2006
force-pushed
the
feat/auth-dormant-telemetry
branch
from
c5ef0cd to
082d965
Compare
coderabbitai
bot
added
the
type: feature
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
1 participant
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.
Proposed changes
Currently,
logFailedLoginAttemptslogs basic connection details (IP, User-Agent) but lacks specific telemetry regarding the targeted account's active state, and swallows the actual error reason.In preparation for the GSoC 2026 project "Warning and Reporting for Login Attempts from Inactive Users", this PR injects lightweight, context-aware telemetry into the authentication flow.
Specifically, it flags failed login attempts targeting:
user.active === false)lastLoginactivity for 180+ days)It also exposes the underlying
login.error.reasonto the logger so server admins have the necessary context for the failure rather than a generic "Failed login detected" message.Issue(s)
N/A - GSoC 2026 Preparatory Work / Proactive Code Audit.
Steps to test or reproduce
reasonfield) but no extra bloat.accountStatus: 'deactivated'and the associated warning.lastLogindate to >180 days in the past. Attempt to log in with a bad password. Verify the server logs outputdaysInactive: Xand the dormant account warning.Further comments
To ensure this telemetry adds zero bloat to standard failed login logs, I utilized the conditional spread pattern (
...(condition && { key: value })). If a normal, active user types a bad password, the resulting log object remains perfectly clean and unchanged, preventing unnecessary database/log clutter.Summary by CodeRabbit
New Features
Chores