Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

settings: Add two realm settings to restrict direct messages. #28470

Merged
merged 3 commits into from
Jul 9, 2024

Conversation

Vector73
Copy link
Collaborator

@Vector73 Vector73 commented Jan 6, 2024

1st commit adds is_any_user_in_group function in lib/user_groups.py.

2nd commit adds two realm settings:

  • direct_message_initiator_group - ID of the user group whose members can initiate direct message thread.
  • direct_message_permission_group - ID of the user group of which at least member must be included as sender or recipient in personal and group direct messages.

3rd commit removes private_message_policy setting.

Fixes: #24467

Screenshots and screen captures:

Screenshots

Screenshot 2024-07-08 174756

Screenshot 2024-07-08 174711

Screenshot 2024-07-08 174658

Screenshot 2024-07-08 174645

Screenshot 2024-07-08 223314

Self-review checklist
  • Self-reviewed the changes for clarity and maintainability
    (variable names, code reuse, readability, etc.).

Communicate decisions, questions, and potential concerns.

  • Explains differences from previous plans (e.g., issue description).
  • Highlights technical choices and bugs encountered.
  • Calls out remaining decisions and concerns.
  • Automated tests verify logic where appropriate.

Individual commits are ready for review (see commit discipline).

  • Each commit is a coherent idea.
  • Commit message(s) explain reasoning and motivation for changes.

Completed manual review and testing of the following:

  • Visual appearance of the changes.
  • Responsiveness and internationalization.
  • Strings and tooltips.
  • End-to-end functionality of buttons, interactions and flows.
  • Corner cases, error conditions, and easily imagined bugs.

@zulipbot zulipbot added size: XL area: compose (banners & validation) Validation and banners in the compose box area: settings (admin/org) difficult Issues which we expect to be quite difficult new feature A proposed new feature for the product priority: high labels Jan 6, 2024
@zulipbot
Copy link
Member

zulipbot commented Jan 6, 2024

Hello @zulip/server-settings members, this pull request was labeled with the "area: settings (admin/org)" label, so you may want to check it out!

@Vector73 Vector73 force-pushed the restrict-dms branch 6 times, most recently from c3b7920 to 5c689cd Compare January 6, 2024 11:56
web/src/admin.js Outdated Show resolved Hide resolved
zerver/models/messages.py Outdated Show resolved Hide resolved
return UserMessage.objects.select_related().get(
user_profile=user_profile, message_id=message_id
)
return UserMessage.objects.get(user_profile=user_profile, message_id=message_id)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This first commit should have a commit message explaining why it's desirable and correct to remove this -- I know it's in the PR description, but it's the commits that are most handy for posterity.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added commit description.

@Vector73
Copy link
Collaborator Author

Vector73 commented Jul 8, 2024

  • Changed error strings in web client:
    direct_message_permission_group:
    if nobody_system_group, "Direct messages are disabled in this organization.";
    else, "This conversation does not include any users who can authorize it.".

direct_message_intitiator_group: "You are not allowed to start direct message conversations."

  • Changed setting label of:
    direct_message_permission_group to "Who can authorize a direct message conversation";
    direct_message_initiator_group to "Who can start a direct message conversation".

Also added Learn more link to /help/restrict-direct-messages in narrow and compose banners.

@alya
Copy link
Contributor

alya commented Jul 8, 2024

Let's put the help ? on the section heading rather than each of the options, since they go to the same place anyway. It'll be more consistent with other sections in this settings panel as well.

@alya
Copy link
Contributor

alya commented Jul 8, 2024

We aren't ready to allow non-system groups to be used for permissions settings, so the UI options should be limited to system groups + nobody for now (confirmed with @timabbott ).

@alya
Copy link
Contributor

alya commented Jul 8, 2024

Looking good other than that! I didn't test exhaustively, but did try a few variations of permission configurations.

@alya
Copy link
Contributor

alya commented Jul 8, 2024

I noted that if you are composing a DM to a new conversation while viewing another conversation, and you aren't allowed to start conversations, you get a server banner when you try to send, but not a warning banner before you send. @timabbott tells me this is not easy to improve upon, though, so it's fine to leave as-is.

Both types of banners for reference:

Screenshot 2024-07-08 at 12 35 01

@chrisbobbe
Copy link
Contributor

chrisbobbe commented Jul 8, 2024

Is there a distinct error code when POST /messages fails because of the new setting? I didn't see one when reading the api_docs/changelog.md changes.

I guess we don't necessarily need a distinct code as long as the error contains a user-facing string we can show the user. Does it?

@timabbott
Copy link
Member

This has a reasonable user-facing error string, so there's no special code for it. The web app displays that server-provided string in at least one-error handling case (though in most situations, that doesn't get used just because the web app won't let you try composing to recipients if it knows you don't have permission, there's a few ways to evade that).

My recommendation for mobile is just to have generic code for displaying generic 400 error sending failures to the user.

settings_config.private_message_policy_values.by_anyone.code
) {
const {name} = user_groups.get_user_group_from_id(realm.realm_direct_message_permission_group);
if (name !== "role:nobody") {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This check is OK for now but probably not the ideal way to check if nobody can authorize direct messages.

Can you do a follow-up commit that exports a function interface, user_groups.is_empty_group(realm.realm_direct_message_permission_group), replacing all the role:nobody checks in the web frontend?

That implementation should probably work by just walking the actual membership to see if it's empty.

web/src/user_groups.ts Outdated Show resolved Hide resolved
@timabbott timabbott force-pushed the restrict-dms branch 2 times, most recently from 447bf55 to 5e558e1 Compare July 9, 2024 01:18
@timabbott
Copy link
Member

Let's put the help ? on the section heading rather than each of the options, since they go to the same place anyway. It'll be more consistent with other sections in this settings panel as well.

Done.

@timabbott
Copy link
Member

timabbott commented Jul 9, 2024

We aren't ready to allow non-system groups to be used for permissions settings, so the UI options should be limited to system groups + nobody for now (confirmed with @timabbott ).

I think this is probably an issue that we need to audit all the group-based settings that we've merged recently for. I don't really remember what the right way to control this is; @sahil839 can you perhaps put up a follow-up PR for adjusting that?

I think otherwise this is good to merge, so I'm going to do that to keep the migration numbering going. @Vector73 please follow-up on #28470 (comment).

@alya FYI.

@timabbott timabbott enabled auto-merge (rebase) July 9, 2024 02:09
@timabbott timabbott merged commit 121043b into zulip:main Jul 9, 2024
13 of 14 checks passed
@sahil839
Copy link
Collaborator

sahil839 commented Jul 9, 2024

I think this is probably an issue that we need to audit all the group-based settings that we've merged recently for. I don't really remember what the right way to control this is; @sahil839 can you perhaps put up a follow-up PR for adjusting that?

We show user defined groups as options in a couple of newly added settings as we have the UI to allow a single user group to be used for a setting. I don't see any problem in allowing them to provide users the flexibility to create a user group with the intended members for now till we add the UI to allow a combination of users and groups.

@Vector73
Copy link
Collaborator Author

@Vector73 please follow-up on #28470 (comment).

Opened a follow-up PR #30810.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: compose (banners & validation) Validation and banners in the compose box area: settings (admin/org) difficult Issues which we expect to be quite difficult integration review Added by maintainers when a PR may be ready for integration. new feature A proposed new feature for the product priority: high size: XL
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add settings for restricting direct messages
6 participants