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

zulip: sync stream membership #92

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

davidtwco
Copy link
Member

@davidtwco davidtwco commented Nov 4, 2024

See rust-lang/team#1604 for the other half of this.

t-compiler want to use our private Zulip stream for some communications but we've held off on doing this as the membership of that stream is manually managed, that is inconvenient and we sometimes forget. In testing this patch, I found the stream membership was still out-of-sync!

I've ran this in dry run mode and it works, so the only thing that needs testing is the API requests for adding and removing subscribers, and that the token that runs this will have permissions to make the change.

I've deliberately avoided adding any automation for the creation of streams, it assumes they exist as this is all that the compiler team needs. There's a reasonable amount of duplication with the types and functions for Zulip user groups, but given that it's largely modelling the upstream API, and we'll likely extend these types to manage more about streams, I think that's okay. Everything I've added should be reusable and compatible with managing streams fully, but I'll leave that for other patches like rust-lang/team#1244.

@MarcoIeni
Copy link
Member

MarcoIeni commented Nov 4, 2024

Hi, thanks for this PR! Can you fix the clippy error in CI?

@davidtwco
Copy link
Member Author

Hi, thanks for this PR! Can you fix the clippy error in CI?

I think the clippy errors are just because the rust-lang/team changes that it depends on aren't merged.

Copy link
Member

@MarcoIeni MarcoIeni left a comment

Choose a reason for hiding this comment

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

Looks good apart from a few minor comments 👍

src/zulip/api.rs Outdated Show resolved Hide resolved
src/zulip/api.rs Outdated Show resolved Hide resolved
src/zulip/mod.rs Show resolved Hide resolved
@MarcoIeni
Copy link
Member

I've deliberately avoided adding any automation for the creation of streams, it assumes they exist as this is all that the compiler team needs.

Sure, no worries, I created #93 👍

@pietroalbini
Copy link
Member

Thanks for making a PR for this! I wanted this for so long ❤️

Given that running this on a public stream would forcibly remove every non-team-member, I suggest we either error if we try to sync a public stream (as a safeguard), or in public streams we only add, not remove, people.

and that the token that runs this will have permissions to make the change.

This is going to be tricky, since that token cannot exist. Zulip allows only the subscribers of a stream to add or remove other subscribers, nobody else can do it (not even the owners of the Zulip instance). This means we'll need to first add a bot account (rust-lang-owner) to every private stream, and then create a token for that account.

@MarcoIeni
Copy link
Member

This means we'll need to first add a bot account (rust-lang-owner) to every private stream, and then create a token for that account.

Does this mean that if an infra admin logs into zulip as rust-lang-owner they can read any private stream without being noticed?
Also, I'm not sure if this is already the case.

@pietroalbini
Copy link
Member

Does this mean that if an infra admin logs into zulip as rust-lang-owner they can read any private stream without being noticed?

Yes, this would be based on trust that privileges are not abused.

Also, I'm not sure if this is already the case.

It's not.

@davidtwco
Copy link
Member Author

or in public streams we only add, not remove, people

Added support for this

@MarcoIeni
Copy link
Member

Yes, this would be based on trust that privileges are not abused.

Ok, let's make sure everybody is aware of this 👍

E.g. we could add a comment to the toml schema.

src/zulip/api.rs Outdated Show resolved Hide resolved
@MarcoIeni
Copy link
Member

MarcoIeni commented Nov 12, 2024

This means we'll need to first add a bot account (rust-lang-owner) to every private stream, and then create a token for that account.

This sounds like a separate PR that we need to merge before this. I guess we don't want to add rust-lang-owner in the toml files. Instead, we want to automatically add this account from the sync team code, listing all private streams and editing them automatically, right?

Another approach would be to only add the account to private streams that are present in the team toml files. I think it might be better for a privacy point of view.

What's the zulip id of the rust-lang-owner user? Can you link to the account? I couldn't find it. Maybe it doesn't exist yet 🤔

src/zulip/api.rs Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants