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

Add bot_type concept #761

Closed
wants to merge 0 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@TomaszKolek
Contributor

TomaszKolek commented May 7, 2016

No description provided.

@smarx

This comment has been minimized.

Show comment
Hide comment
@smarx

smarx May 7, 2016

Automated message from Dropbox CLA bot

@TomaszKolek, it looks like you've already signed the Dropbox CLA. Thanks!

smarx commented May 7, 2016

Automated message from Dropbox CLA bot

@TomaszKolek, it looks like you've already signed the Dropbox CLA. Thanks!

@TomaszKolek

This comment has been minimized.

Show comment
Hide comment
@TomaszKolek

TomaszKolek May 7, 2016

Contributor

@timabbott Please look at the first point from #692.

You probably see it. Here we've got backward incompatible changes.
And of course it shouldn't be merged before next points of the issue.
Tell me please is it ok or not.

Contributor

TomaszKolek commented May 7, 2016

@timabbott Please look at the first point from #692.

You probably see it. Here we've got backward incompatible changes.
And of course it shouldn't be merged before next points of the issue.
Tell me please is it ok or not.

@TomaszKolek TomaszKolek changed the title from [WIP] add is_incoming_webhook field to user_profile to Add is_incoming_webhook field to user_profile May 7, 2016

Show outdated Hide outdated zerver/decorator.py Outdated
Show outdated Hide outdated zerver/tests/test_decorators.py Outdated
@timabbott

This comment has been minimized.

Show comment
Hide comment
@timabbott

timabbott May 7, 2016

Member

Cool! I made some suggestions for how to tweak this to make it backwards-compatible, and thus something we could merge essentially now (even before we add UI for controlling it).

Thinking a bit more, though, I guess we're likely to create a few more specialized integration types, so maybe we should be storing the bot type data as a bot_type enum style field (like how we do e.g. Recipient.STREAM). So I'm imagining:

bot_type = models.PositiveSmallIntegerField(null=True)

DEFAULT_BOT = 1 # We'd want to add code to the bot creation codepath to set this bit
INCOMING_WEBHOOK_BOT = 2

and then we can make a property function for is_incoming_webhook that checks user_profile.bot_type == UserProfile.INCOMING_WEBHOOK_BOT, to provide nice syntax for checking whether a bot is a webhook bot.

Does that seem reasonable?

Member

timabbott commented May 7, 2016

Cool! I made some suggestions for how to tweak this to make it backwards-compatible, and thus something we could merge essentially now (even before we add UI for controlling it).

Thinking a bit more, though, I guess we're likely to create a few more specialized integration types, so maybe we should be storing the bot type data as a bot_type enum style field (like how we do e.g. Recipient.STREAM). So I'm imagining:

bot_type = models.PositiveSmallIntegerField(null=True)

DEFAULT_BOT = 1 # We'd want to add code to the bot creation codepath to set this bit
INCOMING_WEBHOOK_BOT = 2

and then we can make a property function for is_incoming_webhook that checks user_profile.bot_type == UserProfile.INCOMING_WEBHOOK_BOT, to provide nice syntax for checking whether a bot is a webhook bot.

Does that seem reasonable?

@TomaszKolek

This comment has been minimized.

Show comment
Hide comment
@TomaszKolek

TomaszKolek May 8, 2016

Contributor

@timabbott:
Sounds good, but I've got some thoughts that there is something more we've not considered. I'll research it tomorrow.

What with is_bot field?
I think the best would be to make a GET property is_bot instead of field directly

Contributor

TomaszKolek commented May 8, 2016

@timabbott:
Sounds good, but I've got some thoughts that there is something more we've not considered. I'll research it tomorrow.

What with is_bot field?
I think the best would be to make a GET property is_bot instead of field directly

@timabbott

This comment has been minimized.

Show comment
Hide comment
@timabbott

timabbott May 9, 2016

Member

I think we could setup things to support eventually making is_bot property which just checks where bot_type is not None; I think all that would require is in the migration that adds this field, we set all bot_type=DEFAULT_BOT for all UserProfile objects where is_bot=True.

Member

timabbott commented May 9, 2016

I think we could setup things to support eventually making is_bot property which just checks where bot_type is not None; I think all that would require is in the migration that adds this field, we set all bot_type=DEFAULT_BOT for all UserProfile objects where is_bot=True.

@TomaszKolek

This comment has been minimized.

Show comment
Hide comment
@TomaszKolek

TomaszKolek May 9, 2016

Contributor

@timabbott
Because now we've got only one type of bots, tell me please we'd like to treat current both types as old "bot"

I mean what with e.g. queries where is_bot=True we'd like to filter only bot_type=default_bot or both?

Contributor

TomaszKolek commented May 9, 2016

@timabbott
Because now we've got only one type of bots, tell me please we'd like to treat current both types as old "bot"

I mean what with e.g. queries where is_bot=True we'd like to filter only bot_type=default_bot or both?

@timabbott

This comment has been minimized.

Show comment
Hide comment
@timabbott

timabbott May 9, 2016

Member

I think for essentially all the existing queries where we check is_bot == True, the filter should instead be bot_type is not None -- since for most of those queries the point is to check whether the user is a human user or a bot.

Member

timabbott commented May 9, 2016

I think for essentially all the existing queries where we check is_bot == True, the filter should instead be bot_type is not None -- since for most of those queries the point is to check whether the user is a human user or a bot.

@TomaszKolek

This comment has been minimized.

Show comment
Hide comment
@TomaszKolek

TomaszKolek May 10, 2016

Contributor

Tim:

I think for backwards compatibility, we should not check for is_incoming_webhook here.
Instead, what I had in mind, is requiring is_incoming_webhook=False in these two functions:

authenticate_log_and_execute_json
validate_api_key (used in authenticated_rest_api_view and authenticated_api_view) (thus preventing the `is_incoming_webhook bots from using the non-webhook parts of the API)

Tomasz:

Some webhooks use authenticated_rest_api_view (it uses validate_api_key) and above change will break it. It looks there is no reason to use authenticated_rest_api_view instead of api_key_only_webhook_view.

Copy of our conversation that we should carry on.

Contributor

TomaszKolek commented May 10, 2016

Tim:

I think for backwards compatibility, we should not check for is_incoming_webhook here.
Instead, what I had in mind, is requiring is_incoming_webhook=False in these two functions:

authenticate_log_and_execute_json
validate_api_key (used in authenticated_rest_api_view and authenticated_api_view) (thus preventing the `is_incoming_webhook bots from using the non-webhook parts of the API)

Tomasz:

Some webhooks use authenticated_rest_api_view (it uses validate_api_key) and above change will break it. It looks there is no reason to use authenticated_rest_api_view instead of api_key_only_webhook_view.

Copy of our conversation that we should carry on.

@TomaszKolek TomaszKolek changed the title from Add is_incoming_webhook field to user_profile to Add bot_type concept May 10, 2016

@timabbott

This comment has been minimized.

Show comment
Hide comment
@timabbott

timabbott May 10, 2016

Member

Would it work to just convert the webhooks using authenticated_rest_api_view to user the webhook decorator? If so, I think that would be better; then we can actually restrict webhook bots to only being able to send webhook messages (with authenticated_rest_api_view they could also read messages, etc.)

Member

timabbott commented May 10, 2016

Would it work to just convert the webhooks using authenticated_rest_api_view to user the webhook decorator? If so, I think that would be better; then we can actually restrict webhook bots to only being able to send webhook messages (with authenticated_rest_api_view they could also read messages, etc.)

@TomaszKolek

This comment has been minimized.

Show comment
Hide comment
@TomaszKolek

TomaszKolek May 10, 2016

Contributor

@timabbott Unfortunately not....
We'd have to change API for some webhooks ...
or add an argument to decorator like is_webhook
What do you think about second option?

Contributor

TomaszKolek commented May 10, 2016

@timabbott Unfortunately not....
We'd have to change API for some webhooks ...
or add an argument to decorator like is_webhook
What do you think about second option?

@timabbott

This comment has been minimized.

Show comment
Hide comment
@timabbott

timabbott May 10, 2016

Member

Yeah, adding an argument to the decorator sounds good to me!

Member

timabbott commented May 10, 2016

Yeah, adding an argument to the decorator sounds good to me!

@TomaszKolek

This comment has been minimized.

Show comment
Hide comment
@TomaszKolek

TomaszKolek May 11, 2016

Contributor

@timabbott
It seems to be working. Please take a look at migration:
If we'll decide that every bot=True by default will be bot_type=DEFAULT_BOT it means that after adding an argument to the decorator, bots from those webhooks will stop to work.

To be honest, I'm not sure what should be a solution. Two options:

  1. Change default value to INCOMING_WEBHOOK
  2. Change condition in checking if bot is incoming webhook to if bot is a bot.
Contributor

TomaszKolek commented May 11, 2016

@timabbott
It seems to be working. Please take a look at migration:
If we'll decide that every bot=True by default will be bot_type=DEFAULT_BOT it means that after adding an argument to the decorator, bots from those webhooks will stop to work.

To be honest, I'm not sure what should be a solution. Two options:

  1. Change default value to INCOMING_WEBHOOK
  2. Change condition in checking if bot is incoming webhook to if bot is a bot.
Show outdated Hide outdated zerver/decorator.py Outdated
@timabbott

This comment has been minimized.

Show comment
Hide comment
@timabbott

timabbott May 12, 2016

Member

I think this implementation looks pretty reasonable. It looks like you're missing updates to the call to create_users in zerver/management/commands/initialize_voyager_db.py ?

Member

timabbott commented May 12, 2016

I think this implementation looks pretty reasonable. It looks like you're missing updates to the call to create_users in zerver/management/commands/initialize_voyager_db.py ?

Show outdated Hide outdated zerver/decorator.py Outdated
Show outdated Hide outdated zerver/decorator.py Outdated
@TomaszKolek

This comment has been minimized.

Show comment
Hide comment
@TomaszKolek

TomaszKolek May 15, 2016

Contributor

@timabbott Ready for re-review.

I saw just now, that is something wrong with tests - I will try to fix it tomorrow.

Contributor

TomaszKolek commented May 15, 2016

@timabbott Ready for re-review.

I saw just now, that is something wrong with tests - I will try to fix it tomorrow.

@TomaszKolek

This comment has been minimized.

Show comment
Hide comment
@TomaszKolek

TomaszKolek May 16, 2016

Contributor

@timabbott Thanks for help with test problems.
Ready again ;-)

Contributor

TomaszKolek commented May 16, 2016

@timabbott Thanks for help with test problems.
Ready again ;-)

@timabbott

This comment has been minimized.

Show comment
Hide comment
@timabbott

timabbott May 17, 2016

Member

@TomaszKolek this generally looks pretty good; I don't see any obvious bugs on skimming this. However, I think this would be clearer if you broke this into a few commits to make it easier to review each individual change for correctness and the history clearer:

  • A commit adding the new bot_type field, including the migration to set the default value.
  • A commit adding the INCOMING_WEBHOOK bot type and enforcing that incoming webhook bots can only use the webhook views
  • A commit adding the is_webhook option for authenticated_rest_api_view and updating how that decorator works and its endpoints.
  • Possibly some additional commits adding test classes which aren't directly related to one of the below, e.g. adding the TestValidateApiKey method without the pieces specific to webhooks.

The precise divide isn't super important -- just do something coherent that makes it more obvious that your changes are correct and complete for each of the pieces of this overall change.

(See http://zulip.readthedocs.io/en/latest/code-style.html#commit-discipline for discussion of our approach to this and why we find it's really valuable to do finer-grained commits separating independent pieces of new features).

These should all stay in this PR -- just split the commit up using an interactive rebase. If you haven't doe this sort of thing before, my main tip is that I find it really useful to make heavy use of git checkout origin/master PATH, tools like gitk, and doing a git diff against your pre-rebase branch to make sure you don't lose any changes in the process.

Let me know if you have any trouble doing this!

Again, thanks for working on this, it's looking good, but I'd like to be sure since this is an important security-sensitive piece of the Zulip logic.

Member

timabbott commented May 17, 2016

@TomaszKolek this generally looks pretty good; I don't see any obvious bugs on skimming this. However, I think this would be clearer if you broke this into a few commits to make it easier to review each individual change for correctness and the history clearer:

  • A commit adding the new bot_type field, including the migration to set the default value.
  • A commit adding the INCOMING_WEBHOOK bot type and enforcing that incoming webhook bots can only use the webhook views
  • A commit adding the is_webhook option for authenticated_rest_api_view and updating how that decorator works and its endpoints.
  • Possibly some additional commits adding test classes which aren't directly related to one of the below, e.g. adding the TestValidateApiKey method without the pieces specific to webhooks.

The precise divide isn't super important -- just do something coherent that makes it more obvious that your changes are correct and complete for each of the pieces of this overall change.

(See http://zulip.readthedocs.io/en/latest/code-style.html#commit-discipline for discussion of our approach to this and why we find it's really valuable to do finer-grained commits separating independent pieces of new features).

These should all stay in this PR -- just split the commit up using an interactive rebase. If you haven't doe this sort of thing before, my main tip is that I find it really useful to make heavy use of git checkout origin/master PATH, tools like gitk, and doing a git diff against your pre-rebase branch to make sure you don't lose any changes in the process.

Let me know if you have any trouble doing this!

Again, thanks for working on this, it's looking good, but I'd like to be sure since this is an important security-sensitive piece of the Zulip logic.

@TomaszKolek

This comment has been minimized.

Show comment
Hide comment
@TomaszKolek

TomaszKolek May 18, 2016

Contributor

@timabbott I'm not sure I did it right way. But I'm sure I didn't miss anything

Contributor

TomaszKolek commented May 18, 2016

@timabbott I'm not sure I did it right way. But I'm sure I didn't miss anything

@timabbott

This comment has been minimized.

Show comment
Hide comment
@timabbott

timabbott May 18, 2016

Member

Cool, this is definitely a lot better; I have a few changes to suggest:

  • All the create_user type code around setting bot_type=UserProfile.DEFAULT_BOT by default should be moved from the last commit to the commit that introduces the bot_type variable.
  • The summary (first-line) parts of the commit messages for the last few commits are too long. You should come up with a simpler first-line commit message for them and potentially add the extra details in the body of the commit message. E.g. for "Add is_webhook option to authenticated_rest_api_view, authenticated_api_view and validate_api_key.", I would probably have written it as "Add is_webhook option to API authentication decorators.", and then maybe put some extra detail in a second paragraph explaining the purpose of the change (e.g. how we have some older webhook endpoints that use the old Basic auth model and thus use authenticated_rest_api_view, and we wanted to support those...)
  • Finally, in the second commit, where you introduce "INCOMING_WEBHOOK_BOT = 2", we should add a comment above that line explaining the purpose of this bot type -- e.g. that incoming webhook bots are limited to only sending messages via webhooks, and thus it is less of a security risk to expose their API keys since they can't be used to read messages.

Once those are resolved, I'm ready to merge this. Thanks @TomaszKolek!

(There will still need to be follow-up work to create a UI for managing this, but I think it makes sense to merge the backend infrastructure for the feature once it's done, and we can add the UI as a second stage).

Member

timabbott commented May 18, 2016

Cool, this is definitely a lot better; I have a few changes to suggest:

  • All the create_user type code around setting bot_type=UserProfile.DEFAULT_BOT by default should be moved from the last commit to the commit that introduces the bot_type variable.
  • The summary (first-line) parts of the commit messages for the last few commits are too long. You should come up with a simpler first-line commit message for them and potentially add the extra details in the body of the commit message. E.g. for "Add is_webhook option to authenticated_rest_api_view, authenticated_api_view and validate_api_key.", I would probably have written it as "Add is_webhook option to API authentication decorators.", and then maybe put some extra detail in a second paragraph explaining the purpose of the change (e.g. how we have some older webhook endpoints that use the old Basic auth model and thus use authenticated_rest_api_view, and we wanted to support those...)
  • Finally, in the second commit, where you introduce "INCOMING_WEBHOOK_BOT = 2", we should add a comment above that line explaining the purpose of this bot type -- e.g. that incoming webhook bots are limited to only sending messages via webhooks, and thus it is less of a security risk to expose their API keys since they can't be used to read messages.

Once those are resolved, I'm ready to merge this. Thanks @TomaszKolek!

(There will still need to be follow-up work to create a UI for managing this, but I think it makes sense to merge the backend infrastructure for the feature once it's done, and we can add the UI as a second stage).

@TomaszKolek TomaszKolek referenced this pull request May 19, 2016

Closed

Add bot_type field. #812

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment