fix: missing validation for external members#37050
Conversation
|
Looks like this PR is ready to merge! 🎉 |
🦋 Changeset detectedLatest commit: 1819393 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 |
WalkthroughImplements client-side validation preventing adding external users to non-federated rooms in both Create Channel modals, adds corresponding i18n strings, and bumps @rocket.chat/i18n and @rocket.chat/meteor patch versions. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Modal as CreateChannelModal
participant Validator as MembersValidation
participant i18n as Translations
User->>Modal: Enter members list
Modal->>Validator: hasExternalMembers(members)
alt Not federated AND hasExternalMembers
Validator-->>Modal: Error key "You_cannot_add_external_users_to_non_federated_room"
Modal->>i18n: t(errorKey)
i18n-->>Modal: "You cannot add external users to a non-federated room"
Modal-->>User: Show FieldError and disable submission
else Federated OR no external members
Validator-->>Modal: OK
Modal-->>User: Allow submission
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
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 |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #37050 +/- ##
===========================================
- Coverage 67.34% 67.32% -0.02%
===========================================
Files 3339 3339
Lines 113178 113203 +25
Branches 20535 20533 -2
===========================================
- Hits 76217 76212 -5
- Misses 34355 34384 +29
- Partials 2606 2607 +1
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/meteor/client/NavBarV2/NavBarPagesGroup/actions/CreateChannelModal.tsx (1)
366-377: Fix aria-describedby to point to an existing hint only when shown (Broadcast).Currently the toggle always references
${broadcastId}-hinteven when the hint isn’t rendered, which breaks a11y.Apply this diff:
- <ToggleSwitch - aria-describedby={`${broadcastId}-hint`} - aria-labelledby={`${broadcastId}-label`} + <ToggleSwitch + aria-describedby={broadcast ? `${broadcastId}-hint` : undefined} + aria-labelledby={`${broadcastId}-label`} id={broadcastId} ref={ref} checked={value} disabled={!!federated} onChange={onChange} />
🧹 Nitpick comments (6)
.changeset/rotten-dolphins-sort.md (1)
6-6: Tighten the changeset description.Small grammar/style nit and clarify scope (both modals).
Apply this diff:
-Adds a validation to external users when creating a channel that is not federated +Adds validation preventing adding external users to a non‑federated room in the Create Channel modals.apps/meteor/client/NavBarV2/NavBarPagesGroup/actions/CreateChannelModal.tsx (2)
63-64: Make external-member detection more robust (Matrix ID format) and DRY.Current check matches any string starting with '@' and may give false positives. Prefer a Matrix-like ID pattern and consider extracting to a shared util used by both modals.
Apply this diff:
-const hasExternalMembers = (members: string[]): boolean => members.some((member) => member.startsWith('@')); +// External users are represented by Matrix-style IDs like "@user:domain" +const hasExternalMembers = (members: string[]): boolean => + members.some((member) => typeof member === 'string' && /^@[^:]+:.+$/i.test(member.trim()));Optionally move this helper to a shared module and import it in both CreateChannelModal files to avoid duplication.
250-254: Wire up a11y attributes for Members errors.Link the error to the input and expose invalid state.
Apply this diff:
- render(({ field: { onChange, value } }): ReactElement => ( - <UserAutoCompleteMultipleFederated id={addMembersId} value={value} onChange={onChange} placeholder={t('Add_people')} /> + render(({ field: { onChange, value, ref } }): ReactElement => ( + <UserAutoCompleteMultipleFederated + id={addMembersId} + ref={ref} + value={value} + onChange={onChange} + placeholder={t('Add_people')} + aria-invalid={errors.members ? 'true' : 'false'} + aria-describedby={errors.members ? `${addMembersId}-error` : undefined} + /> )} />apps/meteor/client/sidebar/header/CreateChannel/CreateChannelModal.tsx (2)
65-66: Make external-member detection more robust and share across modals.Use a Matrix-style ID regex and avoid duplicating logic between files.
Apply this diff:
-const hasExternalMembers = (members: string[]): boolean => members.some((member) => member.startsWith('@')); +const hasExternalMembers = (members: string[]): boolean => + members.some((member) => typeof member === 'string' && /^@[^:]+:.+$/i.test(member.trim()));Consider extracting this helper to a shared util.
254-258: Add a11y wiring for Members error state.Connect error text to the input and set aria-invalid.
Apply this diff:
- render(({ field: { onChange, value } }): ReactElement => ( - <UserAutoCompleteMultipleFederated id={addMembersId} value={value} onChange={onChange} placeholder={t('Add_people')} /> + render(({ field: { onChange, value, ref } }): ReactElement => ( + <UserAutoCompleteMultipleFederated + id={addMembersId} + ref={ref} + value={value} + onChange={onChange} + placeholder={t('Add_people')} + aria-invalid={errors.members ? 'true' : 'false'} + aria-describedby={errors.members ? `${addMembersId}-error` : undefined} + /> )} />packages/i18n/src/locales/en.i18n.json (1)
7031-7031: Align terminology for non-federated room user errors The existing keyerror-cant-add-federated-usersisn’t used in UI; choose one term—either keep “external users” (remove the unused translation key) or switch references inapps/meteor/client/sidebar/header/CreateChannel/CreateChannelModal.tsxandapps/meteor/client/NavBarV2/NavBarPagesGroup/actions/CreateChannelModal.tsxtoerror-cant-add-federated-usersto maintain consistency.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
.changeset/rotten-dolphins-sort.md(1 hunks)apps/meteor/client/NavBarV2/NavBarPagesGroup/actions/CreateChannelModal.tsx(2 hunks)apps/meteor/client/sidebar/header/CreateChannel/CreateChannelModal.tsx(2 hunks)packages/i18n/src/locales/en.i18n.json(1 hunks)
🔇 Additional comments (3)
.changeset/rotten-dolphins-sort.md (1)
2-3: Confirm necessity of both package bumps.Do we need to bump both @rocket.chat/i18n and @rocket.chat/meteor for this client-side change? If no server or package code changed that depends on these, consider limiting the bump to only what’s required.
apps/meteor/client/sidebar/header/CreateChannel/CreateChannelModal.tsx (1)
246-249: Server-side guard verification.Client-side validation is good, but please confirm the API refuses external members on non-federated room creation to prevent bypass from other clients.
packages/i18n/src/locales/en.i18n.json (1)
7030-7030: Fix placeholder and add pluralization for correctness
- Update en.i18n.json entry:
- "__usersCount__people_will_be_invited": "{{usersCount}} people will be invited" + "__usersCount__people_will_be_invited": { + "one": "{{count}} person will be invited", + "other": "{{count}} people will be invited" + }
- Verify and update all call sites to pass
countinstead ofusersCount.
| rules={{ | ||
| validate: (members) => | ||
| !federated && hasExternalMembers(members) ? t('You_cannot_add_external_users_to_non_federated_room') : true, | ||
| }} |
There was a problem hiding this comment.
Disable “Create” when invalid per FDR-161; revalidate on federated toggle for immediate feedback.
Validation alone blocks submit but doesn’t disable the button. Add a computed flag and re-trigger validation on federated changes.
Apply this diff:
const {
register,
formState: { errors },
handleSubmit,
control,
setValue,
watch,
+ trigger,
} = useForm({
mode: 'onBlur',
defaultValues: {
members: [],
@@
const { isPrivate, broadcast, readOnly, federated, encrypted } = watch();
+ const members = watch('members');
@@
useEffect(() => {
if (federated) {
// if room is federated, it cannot be encrypted or broadcast or readOnly
setValue('encrypted', false);
setValue('broadcast', false);
setValue('readOnly', false);
}
}, [federated, setValue]);
+
+ // Revalidate members when federated toggles so errors/hints update immediately
+ useEffect(() => {
+ void trigger('members');
+ }, [federated, trigger]);
@@
+ const shouldDisableCreate = !federated && hasExternalMembers(members);And disable the Create button:
- <Button type='submit' primary data-qa-type='create-channel-confirm-button'>
+ <Button type='submit' primary data-qa-type='create-channel-confirm-button' disabled={shouldDisableCreate}>
{t('Create')}
</Button>📝 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.
| rules={{ | |
| validate: (members) => | |
| !federated && hasExternalMembers(members) ? t('You_cannot_add_external_users_to_non_federated_room') : true, | |
| }} | |
| // ── Inside CreateChannelModal.tsx ── | |
| const { | |
| register, | |
| formState: { errors }, | |
| handleSubmit, | |
| control, | |
| setValue, | |
| watch, | |
| trigger, | |
| } = useForm({ | |
| mode: 'onBlur', | |
| defaultValues: { | |
| members: [], | |
| // …other defaults | |
| }, | |
| }); | |
| const { isPrivate, broadcast, readOnly, federated, encrypted } = watch(); | |
| const members = watch('members'); | |
| useEffect(() => { | |
| if (federated) { | |
| // if room is federated, it cannot be encrypted or broadcast or readOnly | |
| setValue('encrypted', false); | |
| setValue('broadcast', false); | |
| setValue('readOnly', false); | |
| } | |
| }, [federated, setValue]); | |
| // Revalidate members when federated toggles so errors/hints update immediately | |
| useEffect(() => { | |
| void trigger('members'); | |
| }, [federated, trigger]); | |
| const shouldDisableCreate = !federated && hasExternalMembers(members); | |
| // …later in the JSX form | |
| <Button | |
| type='submit' | |
| primary | |
| data-qa-type='create-channel-confirm-button' | |
| disabled={shouldDisableCreate} | |
| > | |
| {t('Create')} | |
| </Button> |
🤖 Prompt for AI Agents
In apps/meteor/client/NavBarV2/NavBarPagesGroup/actions/CreateChannelModal.tsx
around lines 242–245, the form validator prevents submission but the "Create"
button remains enabled and validation is not re-run when the "federated" toggle
changes; add a computed "isFormValid" (derived from react-hook-form's
formState.isValid or by running trigger()) and wire a useEffect to re-trigger
validation whenever the federated value changes, then pass isFormValid to
disable the Create button (disabled={!isFormValid}). Ensure trigger is awaited
or formState is subscribed to so the button updates immediately on federated
toggle.
| rules={{ | ||
| validate: (members) => | ||
| !federated && hasExternalMembers(members) ? t('You_cannot_add_external_users_to_non_federated_room') : true, | ||
| }} |
There was a problem hiding this comment.
Disable “Create” when invalid per FDR-161; revalidate on federated toggle.
Mirror the UX from the other modal: disable the submit button when external members are present and federated is off; revalidate on toggle.
Apply this diff:
const {
register,
formState: { errors },
handleSubmit,
control,
setValue,
watch,
+ trigger,
} = useForm({
mode: 'onBlur',
@@
const { isPrivate, broadcast, readOnly, federated, encrypted } = watch();
+ const members = watch('members');
@@
useEffect(() => {
if (federated) {
// if room is federated, it cannot be encrypted or broadcast or readOnly
setValue('encrypted', false);
setValue('broadcast', false);
setValue('readOnly', false);
}
}, [federated, setValue]);
+
+ useEffect(() => {
+ void trigger('members');
+ }, [federated, trigger]);
@@
+ const shouldDisableCreate = !federated && hasExternalMembers(members);And disable the Create button:
- <Button type='submit' primary data-qa-type='create-channel-confirm-button'>
+ <Button type='submit' primary data-qa-type='create-channel-confirm-button' disabled={shouldDisableCreate}>
{t('Create')}
</Button>📝 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.
| rules={{ | |
| validate: (members) => | |
| !federated && hasExternalMembers(members) ? t('You_cannot_add_external_users_to_non_federated_room') : true, | |
| }} | |
| // …inside your CreateChannelModal component… | |
| const { | |
| register, | |
| formState: { errors }, | |
| handleSubmit, | |
| control, | |
| setValue, | |
| watch, | |
| trigger, | |
| } = useForm({ | |
| mode: 'onBlur', | |
| // …other options… | |
| }); | |
| const { isPrivate, broadcast, readOnly, federated, encrypted } = watch(); | |
| const members = watch('members'); | |
| useEffect(() => { | |
| if (federated) { | |
| // if room is federated, it cannot be encrypted or broadcast or readOnly | |
| setValue('encrypted', false); | |
| setValue('broadcast', false); | |
| setValue('readOnly', false); | |
| } | |
| }, [federated, setValue]); | |
| useEffect(() => { | |
| // revalidate members whenever federated flips | |
| void trigger('members'); | |
| }, [federated, trigger]); | |
| const shouldDisableCreate = !federated && hasExternalMembers(members); | |
| // …later, in your JSX… | |
| <Button | |
| type='submit' | |
| primary | |
| data-qa-type='create-channel-confirm-button' | |
| disabled={shouldDisableCreate} | |
| > | |
| {t('Create')} | |
| </Button> |
🤖 Prompt for AI Agents
In apps/meteor/client/sidebar/header/CreateChannel/CreateChannelModal.tsx around
lines 246-249, the form validation already prevents adding external members when
federated is false, but the Create button isn't disabled and the rule isn't
re-run when the federated toggle changes; update the submit button to be
disabled when the form is invalid (e.g. disabled={!formState.isValid ||
formState.isSubmitting} or disabled={!watchFederated &&
hasExternalMembers(watch('members'))}) and, when the federated toggle changes,
call the form revalidation (e.g. await trigger(['members']) or trigger() after
updating the federated value) so the rule is re-evaluated and the button state
updates immediately.
aleksandernsilva
left a comment
aleksandernsilva
left a comment
There was a problem hiding this comment.
Could we also add tests covering this scenario?
| )} | ||
| /> | ||
| {errors.members && ( | ||
| <FieldError aria-live='assertive' id={`${addMembersId}-error`}> |
There was a problem hiding this comment.
I don't see this error id being referenced anywhere. I believe the auto complete is missing an aria-describedby.
| )} | ||
| /> | ||
| {errors.members && ( | ||
| <FieldError aria-live='assertive' id={`${addMembersId}-error`}> |
ggazzo
added
the
stat: QA assured
dionisio-bot
bot
added
the
stat: ready to merge
3 participants
FDR-161
Proposed changes (including videos or screenshots)
Issue(s)
Steps to test or reproduce
Further comments
Summary by CodeRabbit
New Features
Bug Fixes
Chores