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

Disable appropriate features for guest. #9714

Conversation

shubhamdhama
Copy link
Member

@shubhamdhama shubhamdhama commented Jun 8, 2018

This PR addresses #8385's point:

For each feature we disable, we'll want to (1) Add the decorator/backend changes and (2) hide the relevant UI for guest users.

Opened as WIP to just check I'm disabling the right features.
Testing Plan:

  • Backend tests
  • Manual checking of the corresponding component

WIP list of features getting disabled:

  • invite: Make invite other users inaccessible to guest users. Image: emoji upload
  • emoji: Make uploading new realm emoji inaccessible for guest users. Image: invite
  • streams: Hide create stream UI from guest users.
  • bots: Hide UI for adding new bots for guest users.

Questions:

  • deactivate_bot_backend? Because guest user can't create any bot.
  • Do we want an API key of a guest accessible? I guess "Yes" because API keys can still be used as "Human bots", but I guess there are various other ways a user can access API key so what should be the appropriate behavior here?
  • We haven't thought about the get_members_backend function. Having normal access over this endpoint means guest user have access to information from all members of the realm. So here we can either make this to function to return users only subscribers of streams to which the guest user is subscribed or do we have plans to hide bots and users table from guest user.
  • While thinking about the above we should also think about the changes in get_raw_user_data function.
  • Any plan for Default streams for guest users.

@shubhamdhama shubhamdhama force-pushed the guest-make-end-point-inaccessible branch from 2cec6ac to ee53cc1 Compare June 8, 2018 13:34
@zulipbot zulipbot added size: M and removed size: S labels Jun 8, 2018
@timabbott
Copy link
Sponsor Member

I think guest users should be able to deactivate bots that they own. The context here is that if a non-guest user transfers ownership of a bot to a guest user (which I can imagine supporting for a few special situations), then the guest user should be able to disable it.

I guess stream creation should be on your list.

Guest users' API keys should be available, yes. You need it for clients like zulip-terminal.

@shubhamdhama shubhamdhama force-pushed the guest-make-end-point-inaccessible branch from ee53cc1 to f321757 Compare June 12, 2018 15:53
@zulipbot zulipbot added size: L and removed size: M labels Jun 12, 2018
@shubhamdhama shubhamdhama force-pushed the guest-make-end-point-inaccessible branch from f321757 to 7fdbbd7 Compare June 12, 2018 16:13
@shubhamdhama shubhamdhama force-pushed the guest-make-end-point-inaccessible branch from 7fdbbd7 to 5265307 Compare June 13, 2018 13:15
@shubhamdhama shubhamdhama changed the title [WIP]Disable appropriate features for guest. Disable appropriate features for guest. Jun 13, 2018
if (page_params.is_admin) {
can_add_emojis = true;
} else if (!(page_params.realm_add_emoji_by_admins_only || page_params.is_guest)) {
can_add_emojis = true;
Copy link
Contributor

Choose a reason for hiding this comment

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

I know this is only 5 lines of code, but I think it's worth extracting to a function, which will allow you to flatten the code a bit and add direct tests to it:

exports.can_add_emoji = function () {
    if (page_params.is_guest) {
        return false;
    }

    if (page_params.is_admin) {
        return true;
    }

    // for normal users, we depend on the setting
    return !page_params.realm_add_emoji_by_admins_only;
};

Does that make sense?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah... I was even not sure whether to extract this or not because most of this operations are done inline in the object code of options itself. Thanks :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, makes sense. Most of the existing code is probably fine to leave as is, but if you see any opportunities for quick cleanup, it's nice to be able to test all these permission things directly.

@showell
Copy link
Contributor

showell commented Jun 13, 2018

@shubhamdhama This looks great! I have one inline comment.

@shubhamdhama shubhamdhama force-pushed the guest-make-end-point-inaccessible branch from 5265307 to 4b08426 Compare June 13, 2018 15:39
@zulipbot zulipbot added size: XL and removed size: L labels Jun 13, 2018
@shubhamdhama
Copy link
Member Author

Thanks, Steve, your review reminded me of an old settings_bots change and while cleaning it, I realized that we may still want to display "Your bots" section to guest users to show the bots whose ownership is changed(Though I don't have deep knowledge in the context of bots ownership change for guest user). So I dropped the previous commit of completely hiding the "Your bots" section and just extended the cleanup code for guest users.
Also, we may need to think about hiding tips for bots or just changing the strings for guest user(currently I've made them hidden).

@timabbott
Copy link
Sponsor Member

This is great, merged, thanks @shubhamdhama!

I agree we should think about the various tips for realm_emoji / bots; it may be worth just making a list of this sort of thing to come back to in a next pass (e.g. it seems possible we want a custom tip for guest users). I think the current list is just those two.

@timabbott timabbott closed this Jun 16, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants