Revalidate community order channel ownership lazily at order creation time#801
Revalidate community order channel ownership lazily at order creation time#801Luquitasjeffrey wants to merge 17 commits into
Conversation
…r channels * Revalidate community order channel ownership by community owner lazily, at order creation time
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughCommunity schema gains uniqueness/sparse constraints and allows empty order_channels; getOrderChannel(order, bot?) validates resolved channel against community owner admin status; publishers close and save orders when channel resolution fails; add interactive migration and tests to clear duplicated Telegram metadata; add locale messages. ChangesOrder Channel Validation with Community Constraints
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
models/community.ts (1)
19-25:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winUnique constraint on embedded OrderChannel.name prevents multi-tenancy.
The
unique: trueconstraint onOrderChannelSchema.name(line 20) creates a global unique index across allorder_channelsin all communities. This means only one community can use any given channel name (e.g.,"@bitcoinchannel"), even though different communities should independently manage their own channels.In Mongoose, unique indexes on embedded array schema fields are enforced at the collection level, not scoped to the parent document. Remove
unique: truefrom line 20 and implement validation at the application level if deduplication within a single community is needed.🤖 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 `@models/community.ts` around lines 19 - 25, Remove the global unique index on the embedded order channel name by deleting the unique: true option on OrderChannelSchema's name field (IOrderChannel.name) so Mongoose doesn't create a collection-level unique index; then add application-level validation to enforce uniqueness scoped to a single community (e.g., check for duplicate channel names within the community document in the add/update handler or pre-save hook on the Community model) and return a validation error if a duplicate name is attempted within that community.
🧹 Nitpick comments (2)
util/index.ts (2)
337-350: ⚡ Quick winAdd logging when channel validation fails.
When the community owner is missing (line 338-340) or channel validation fails (line 347-348), the function returns
undefinedwithout logging the reason. This makes debugging difficult and provides no audit trail for security-related validation failures.📊 Proposed enhancement
const communityOwner = await User.findById(community.creator_id); if (!communityOwner) { + logger.error(`Community owner not found for community ${community._id}, creator_id: ${community.creator_id}`); return undefined; } if (bot && channel) { // Validate order channel if the caller of this function passed the bot instance to perform the validation // If it was not passed as a parameter the order channel can be trusted because its for ui purposes (listorders for example) // This validation is performed lazily when publishing the order to the community order channel const isChannelOk = await isGroupAdmin(channel, communityOwner, bot); if (!isChannelOk.success) { + logger.error(`Channel validation failed for community ${community._id}, channel: ${channel}, reason: ${isChannelOk.message}`); return undefined; } }🤖 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 `@util/index.ts` around lines 337 - 350, Add explicit logging for the two early-return cases: when User.findById(community.creator_id) yields no communityOwner and when isGroupAdmin(channel, communityOwner, bot) returns isChannelOk.success === false. Update the block around communityOwner lookup (the User.findById call and the subsequent if (!communityOwner) return) to log a clear message including community.id and community.creator_id; and update the channel validation branch around isGroupAdmin (the isChannelOk check) to log the validation result, channel id, community.id and communityOwner id before returning. Use the module's existing logger (e.g., processLogger or logger) and include enough context to trace which community and channel failed.
331-335: ⚡ Quick winRemove unnecessary
asynckeyword fromforEachcallback.The
asynckeyword on line 331 serves no purpose since the callback contains noawaitexpressions. This is misleading and suggests asynchronous operations that don't exist.♻️ Proposed fix
} else { - community.order_channels.forEach(async (c: IOrderChannel) => { + community.order_channels.forEach((c: IOrderChannel) => { if (c.type === order.type) { channel = c.name; } }); }🤖 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 `@util/index.ts` around lines 331 - 335, The forEach callback passed to community.order_channels.forEach is unnecessarily declared async (callback signature: async (c: IOrderChannel) => { ... }) even though it contains no await; remove the async keyword so the callback becomes (c: IOrderChannel) => { if (c.type === order.type) { channel = c.name; } } to avoid misleading readers and accidental unhandled promise 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 `@bot/messages.ts`:
- Around line 789-794: When getOrderChannel returns undefined, notify the user
before closing the order: instead of only setting order.status = 'CLOSED' and
throwing, send a localized user-facing message (use ctx.reply or
ctx.telegram.sendMessage and your i18n helper with the key
order_channel_validation_failed) then mark the order CLOSED and persist it
(order.save()) and finally throw or return; also add the new localization string
order_channel_validation_failed to locales/en.yaml (and other languages) with
the provided text so users see "Unable to publish your order. The community
channel could not be validated. Please contact the community administrator."
In `@models/community.ts`:
- Line 67: The schema fields `group` and `dispute_channel` in
models/community.ts currently use `unique: true`, which creates non-sparse
MongoDB unique indexes and will fail if existing documents have null/undefined
values; update both field definitions to either add `sparse: true` (allowing
multiple nulls) or remove `unique` and instead create an explicit
sparse/case-insensitive index (similar to `group_idx` in models/indexes.ts), and
if you choose to keep uniqueness, add a short migration that populates or
deduplicates existing null/empty values before app startup (createIndexes in
app.ts) so index creation cannot fail.
---
Outside diff comments:
In `@models/community.ts`:
- Around line 19-25: Remove the global unique index on the embedded order
channel name by deleting the unique: true option on OrderChannelSchema's name
field (IOrderChannel.name) so Mongoose doesn't create a collection-level unique
index; then add application-level validation to enforce uniqueness scoped to a
single community (e.g., check for duplicate channel names within the community
document in the add/update handler or pre-save hook on the Community model) and
return a validation error if a duplicate name is attempted within that
community.
---
Nitpick comments:
In `@util/index.ts`:
- Around line 337-350: Add explicit logging for the two early-return cases: when
User.findById(community.creator_id) yields no communityOwner and when
isGroupAdmin(channel, communityOwner, bot) returns isChannelOk.success ===
false. Update the block around communityOwner lookup (the User.findById call and
the subsequent if (!communityOwner) return) to log a clear message including
community.id and community.creator_id; and update the channel validation branch
around isGroupAdmin (the isChannelOk check) to log the validation result,
channel id, community.id and communityOwner id before returning. Use the
module's existing logger (e.g., processLogger or logger) and include enough
context to trace which community and channel failed.
- Around line 331-335: The forEach callback passed to
community.order_channels.forEach is unnecessarily declared async (callback
signature: async (c: IOrderChannel) => { ... }) even though it contains no
await; remove the async keyword so the callback becomes (c: IOrderChannel) => {
if (c.type === order.type) { channel = c.name; } } to avoid misleading readers
and accidental unhandled promise 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 5da0bc3f-766e-4043-a74f-1b327dc8a4fb
📒 Files selected for processing (3)
bot/messages.tsmodels/community.tsutil/index.ts
…s and update schema to support sparse unique indexes
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
scripts/migrate_duplicated_community_channels_and_groups.ts (2)
142-146: ⚡ Quick winConsider adding error handling for save operations.
If any
c.save()call fails, the script will crash without completing the migration. Consider wrapping saves in try-catch or using Promise.allSettled to handle partial failures gracefully.♻️ Proposed improvement
console.log('Saving changes...'); - for (const c of communities) { - if (affectedCommunities.has(c._id.toString())) { - await c.save(); + const savePromises = communities + .filter(c => affectedCommunities.has(c._id.toString())) + .map(c => c.save().catch(err => { + console.error(`Failed to save community ${c._id}:`, err); + return err; + })); + + const results = await Promise.allSettled(savePromises); + const failures = results.filter(r => r.status === 'rejected'); + if (failures.length > 0) { + console.error(`${failures.length} communities failed to save.`); - } } console.log('Done.');🤖 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 `@scripts/migrate_duplicated_community_channels_and_groups.ts` around lines 142 - 146, Wrap each c.save() in error handling so a failed save doesn't crash the whole migration: inside the for (const c of communities) loop check affectedCommunities.has(c._id.toString()) then try { await c.save(); } catch (err) { log the error and continue } or accumulate save promises and use Promise.allSettled to inspect results and log failures; reference the loop variables communities, affectedCommunities, and the c.save() call when implementing this change.
81-81: ⚡ Quick winAvoid type assertion
undefined as anywhich bypasses type safety.Setting required fields to
undefinedusingas anycircumvents TypeScript's type checking. Consider updating the Community model schema to make these fields optional, or use a more explicit approach.♻️ Alternative approach
If the model supports it, use explicit typing:
- c.group = undefined as any; + c.group = undefined;Or consider using
nullif the schema allows it, which is more explicit about intentional absence.Also applies to: 94-94, 117-117
🤖 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 `@scripts/migrate_duplicated_community_channels_and_groups.ts` at line 81, The code is using `c.group = undefined as any` (and similar at lines 94 and 117), which bypasses TypeScript safety; instead update the Community type/schema to allow the field to be optional or nullable (e.g., change the Community interface/property to `group?: Group | null` or `group: Group | null`) and then set `c.group = null` (or `delete c.group`) when you mean to remove it, or cast the object to a Partial<Community> only where you intentionally mutate optional fields; locate assignments to `c.group` in this file and replace `undefined as any` with a null/optional-safe approach.
🤖 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 `@scripts/migrate_duplicated_community_channels_and_groups.ts`:
- Around line 116-118: The loop in the migration currently leaves objects with
undefined names in c.order_channels; instead clear the array entirely so the
community "loses all order_channels". Replace the logic that iterates over
c.order_channels (the for (const ch of c.order_channels) { ch.name = undefined
as any; } block) with code that empties the array (e.g., set c.order_channels =
[] or c.order_channels.length = 0) so tests and the console message ("loses all
order_channels") reflect a cleared list.
In `@tests/scripts/migrate_duplicated_community_channels_and_groups.spec.ts`:
- Line 15: The variable consoleLogStub is declared but never used, causing
ESLint failures; either remove the unused declaration entirely if you don't need
to silence logs, or keep the stub but rename it to _consoleLogStub (or prefix
with an underscore) so ESLint recognizes it as intentionally unused, and if
silencing is intended ensure you actually assign it to stub console.log in the
"Silence console" section (use consoleLogStub = sinon.stub(console, 'log') or
similar) and restore it after tests.
---
Nitpick comments:
In `@scripts/migrate_duplicated_community_channels_and_groups.ts`:
- Around line 142-146: Wrap each c.save() in error handling so a failed save
doesn't crash the whole migration: inside the for (const c of communities) loop
check affectedCommunities.has(c._id.toString()) then try { await c.save(); }
catch (err) { log the error and continue } or accumulate save promises and use
Promise.allSettled to inspect results and log failures; reference the loop
variables communities, affectedCommunities, and the c.save() call when
implementing this change.
- Line 81: The code is using `c.group = undefined as any` (and similar at lines
94 and 117), which bypasses TypeScript safety; instead update the Community
type/schema to allow the field to be optional or nullable (e.g., change the
Community interface/property to `group?: Group | null` or `group: Group | null`)
and then set `c.group = null` (or `delete c.group`) when you mean to remove it,
or cast the object to a Partial<Community> only where you intentionally mutate
optional fields; locate assignments to `c.group` in this file and replace
`undefined as any` with a null/optional-safe approach.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: e713a016-fcca-4390-a8b0-43d313163aac
📒 Files selected for processing (3)
models/community.tsscripts/migrate_duplicated_community_channels_and_groups.tstests/scripts/migrate_duplicated_community_channels_and_groups.spec.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- models/community.ts
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 `@bot/messages.ts`:
- Around line 735-741: Persist the order closure before attempting Telegram
notifications: set order.status = 'CLOSED' and await order.save() immediately
(using the same order.save call currently on Line 739/799) so the DB reflects
closure even if messaging fails, then call bot.telegram.sendMessage(...) in a
try/catch and log or ignore send errors; update both places around the
sendMessage -> order.save sequence to ensure closure is always persisted first
and notifications are isolated.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 927c6e9f-7d53-4841-862d-43714426c412
📒 Files selected for processing (12)
bot/messages.tslocales/de.yamllocales/en.yamllocales/es.yamllocales/fa.yamllocales/fr.yamllocales/it.yamllocales/ko.yamllocales/pt.yamllocales/ru.yamllocales/uk.yamltests/scripts/migrate_duplicated_community_channels_and_groups.spec.ts
✅ Files skipped from review due to trivial changes (6)
- locales/uk.yaml
- locales/es.yaml
- locales/en.yaml
- locales/it.yaml
- locales/de.yaml
- locales/ru.yaml
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/scripts/migrate_duplicated_community_channels_and_groups.spec.ts
…y-channel-ownership-lazily
…alidation fails, so order state is always persisted
![mostronatorcoder[bot]](https://avatars.githubusercontent.com/in/2922838?s=60&v=4){kind=small}
There was a problem hiding this comment.
The publish-time ownership revalidation looks correct and does address the reported bug. I also agree with enforcing global uniqueness for group and dispute_channel, since allowing multiple communities to share those identifiers would create ambiguity and operational issues.
My remaining concern is specifically about order_channels.name:
-
order_channels.nameis defined inside an embedded array schema, and addingunique: true, sparse: truethere is a much trickier change than the top-levelgroup/dispute_channelconstraints. Even if global uniqueness is the intended business rule, I would like this handled more explicitly because unique indexes on embedded array fields can be harder to reason about and maintain. -
The migration does not cleanly remove conflicting order channels. It sets each conflicted
ch.name = undefined, which leaves partially-cleared embedded documents behind. If a community is losing conflicting order channels, those entries should be removed cleanly instead of leaving invalid/incomplete objects in the array. -
Please add logging when channel validation fails due to missing community owner or failed admin check. This path is security-relevant and currently has very little observability.
-
Minor:
forEach(async ...)ingetOrderChannelshould be replaced with a normal synchronous iteration.
Summary: I agree with the direction of the fix and with making group / dispute_channel globally unique. I am only requesting changes on the order_channels implementation details before approving.
…hould set order_channels to an empty array
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 `@models/community.ts`:
- Line 20: Change the IOrderChannel interface so its name property is optional
(declare name?: string in IOrderChannel) to match the mongoose schema; then
update all call sites that assume a non-null name (e.g., the usages in
util/index.ts that access .name directly) to safely handle undefined by adding
null-checks or using optional chaining and fallback values where appropriate so
runtime accesses no longer rely on a guaranteed string.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 91b6d3ad-3c83-490f-a3fc-22cf5b362649
📒 Files selected for processing (14)
bot/messages.tslocales/de.yamllocales/en.yamllocales/es.yamllocales/fa.yamllocales/fr.yamllocales/it.yamllocales/ko.yamllocales/pt.yamllocales/ru.yamllocales/uk.yamlmodels/community.tsscripts/migrate_duplicated_community_channels_and_groups.tstests/scripts/migrate_duplicated_community_channels_and_groups.spec.ts
✅ Files skipped from review due to trivial changes (8)
- locales/es.yaml
- locales/fa.yaml
- locales/en.yaml
- locales/fr.yaml
- locales/de.yaml
- locales/ko.yaml
- locales/ru.yaml
- locales/pt.yaml
🚧 Files skipped from review as they are similar to previous changes (4)
- bot/messages.ts
- locales/it.yaml
- scripts/migrate_duplicated_community_channels_and_groups.ts
- tests/scripts/migrate_duplicated_community_channels_and_groups.spec.ts
…nnecesary async in the foreach loop that iterates to find the suitable order channel to publish the order
There was a problem hiding this comment.
This now looks good to me.
The publish-time ownership revalidation correctly addresses the reported bug, the migration now clears conflicting order_channels cleanly, and the unnecessary async iteration was removed. Adding logging on channel validation failure also improves observability in a security-relevant path.
Minor non-blocking note: it would still be useful to log when the community owner cannot be found before returning undefined, for completeness and easier debugging of inconsistent community state.
![ermeme[bot]](https://avatars.githubusercontent.com/in/3690068?s=60&v=4){kind=small}
| }; | ||
|
|
||
| const getOrderChannel = async (order: IOrder) => { | ||
| const getOrderChannel = async (order: IOrder, bot?: Telegram) => { |
There was a problem hiding this comment.
🔴 Critical: getOrderChannel() still falls back to process.env.CHANNEL when a community has no matching order channel. This PR now allows empty order_channels, so a community that loses its channels can silently publish to the global channel instead of failing closed. For community orders, no resolved channel should mean undefined/error, not fallback to the public channel.
| console.log(`Found ${duplicateGroups.length} duplicated groups.`); | ||
| console.log(`Found ${duplicateDispute.length} duplicated dispute channels.`); | ||
| console.log( | ||
| `Found ${duplicateOrderChannels.length} duplicated order channels.`, |
There was a problem hiding this comment.
isGroupAdmin() fails, so duplicated group / dispute_channel / order_channels.name values can remain for admin-owned communities. That means the schema change can still fail to apply cleanly or leave the collection in a state that violates the new constraints.
1 participant
Summary by CodeRabbit
Bug Fixes
Improvements
Tools
Tests
Localization