Skip to content

Roles #1789

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

Closed
wants to merge 22 commits into from
Closed

Roles #1789

wants to merge 22 commits into from

Conversation

Bibo-Joshi
Copy link
Member

@Bibo-Joshi Bibo-Joshi commented Feb 21, 2020

This PR adds the functionality of restricting access to handlers to

  • specified users (by id)
  • specified chats (by id)
  • chat admins
  • chat creators

Builds upon #1757 (see below)
Closes #1151, #1562

Example usage:

updater = Updater('TOKEN')
dp = updater.dispatcher
roles = updater.dispatcher.roles

roles.add_admin('authors_user_id')
roles.add_role(name='my_role_1')
my_role_1 = roles['my_role_1']
roles.add_role(name='my_role_2', parent_roles=roles['my_role_1'])
my_role_2 = roles['my_role_2']

def add_to_my_role_2(update, context):
    user = update.effective_user
    context.roles['my_role_2'].add_member(user.id)
    
def add_to_my_role_1(update, context):
    user_id = update.message.text
    context.roles['my_role_1'].add_member(user_id)

....

# Anyone can add themself to my_role_2
dp.add_handler(TypeHandler(Update, add_to_my_role_2))

# Only the admin can add users to my_role_1
dp.add_handler(MessageHandler(Filters.message, add_to_my_role_1, roles=roles.ADMINS))

# This will be accessable by my_role_2, my_role_1 and the admin
dp.add_handler(SomeHandler(..., roles=my_role_2))

# This will be accessable by my_role_1 and the admin
dp.add_handler(SomeHandler(..., roles=my_role_1))

# This will be accessable by anyone except my_role_1 and my_role_2
dp.add_handler(SomeHandler(..., roles=~my_role_1))

# This will be accessable by users, who are in my_role_2 bot not in my_role_1
dp.add_handler(SomeHandler(..., roles=~my_role_1 & my_role_2))

# This will be accessable only by the admins of the group the update was sent in
dp.add_handler(SomeHandler(..., roles=roles.CHAT_ADMINS))

# This will be accessable only by the creator of the group the update was sent in
dp.add_handler(SomeHandler(..., roles=roles.CHAT_CREATOR))

# You can compare the roles regarding hierarchy:
roles.ADMINS >=  roles['my_role_1'] #True
roles.ADMINS >=  roles['my_role_2'] #True
roles['my_role_1'] < roles['my_role_2'] #False
roles.ADMINS >= Role(...) #False, since neither of those is a parent of the other

How does it work

Added Classes

Role

  • Inherits from Filters.user (This is, where Allow updating user_ids/usernames of a running filters.user #1757 comes in. Can be changed to inherinting from BaseFilter if the reviewer want that)
  • Can be combined by bitwise operations (This is why I'm Inheriting from Filters)
  • Can be compared w.r.t. hierarchy. E.g. in above example: my_role_1 > my_role_2
  • Dynamically add/kick members

Roles

A container for multiple roles

  • Inherits from dict to allow accessing the roles by roles['role_name']
  • Roles.ADMINS is parent to every role
  • Roles.CHAT_ADMINS, Roles.CHAT_CREATOR as shortcuts for restricting acces in group chats
  • Dynamically add/remove admins and roles

Integration with handlers

The base class Handler now has a __new__ method that makes sure, that check_update is called only, if the roles checked out. Alternatives approaches would be

  • Make Handler.check_update check the roles directly and call super().check_updater in all the specific handlers. This means that custom handlers would have to change their check_update implementation. Then again, they may have to be adjusted to accept the roles argument anyway
  • Add a metho Handler.check_update_with_roles that calles Handler.check_update after checking the roles. In the current code base, replace all calls to check_update with check_update_with_role. This would leave current implementation of check_update untouched.

Integration with dispatcher

  • Dispatcher.roles is the Roles instance managing the roles
  • Dispatcher.roles is available as context.roles to allow editing roles in callbacks
  • store_roles is added to persistence to allow keeping track of roles over restarts. I know that adding stuff to persistence should be done with care. But if we want dynamic access control (and I do), we need persistence here.

ToDo

  • Add roles to new PollAnswerHandler. As Poll updates have neither user nor chat, we don't need roles with PollHandler

  • Cache chat admins and creators (timeout is customizable via roles.CHAT_ADMINS.timeout = ...)

  • Handle private chats correctly with roles.CHAT_ADMINS/CHAT_CREATOR (this is as easy if checking chat.id == user.id)

  • When merged, we should create a wiki page. Important points to mention there:

    • Hard coding dp.roles.add_admin(id) will always add the user to admins on start up with persistence, even if they are removed by dynamic access control. Thus, only the author of the bot should be hard coded as admin.
    • Admins should only be in roles.ADMINS, else they might be excluded by roles=~roles['my_role'], if they are in my_role
    • Hard coding dp.roles.add_role(name='my_group') will lead to ValueError on restart with persistence because after restoring the data, the role my_group will already be set. Use 'my_group' in dp.roles to check for that before adding the group.
  • Depending on how long it will take to merge this:

@Bibo-Joshi Bibo-Joshi added this to the 12.5 milestone Feb 21, 2020
@Bibo-Joshi
Copy link
Member Author

Codacy mainly complains about missing docs for methods which are intended for internal use only. IMHO we can ignore that.

@Bibo-Joshi Bibo-Joshi requested a review from Poolitzer February 21, 2020 23:00
@Bibo-Joshi Bibo-Joshi mentioned this pull request Mar 23, 2020
6 tasks
Bibo-Joshi and others added 21 commits April 7, 2020 14:44
Add roles to all handlers

make roles name a property to ensure its up to date

Add roles to PrefixHandler

Make Roles inherit from dict

Adjust tests for roles

Adjust handlers __new__ to work with conversationhandler

Remove roles from StringCommand/RegexHandler - doesnt make sense there

Remove roles from TypeHandler - doesnt make sense there

add handler tests
Remove child_roles from role
Rework equality stuff once again

Improve equality tests for role
Fixes for chat_ids handling

Adjust CH test for chat_ids
Adjust tests for py3.5 (dicts are not ordered there)

Adjust tests for py3.5 V2

Just skip on py3.5 …
Add docs about cache timeout for Roles.CHAT_ADMINS
Use __repr__ instead of name property
Update BasePersistence docstring

Refine handler tests
# Conflicts:
#	telegram/ext/dispatcher.py
#	tests/test_persistence.py
@Bibo-Joshi
Copy link
Member Author

As suggested by @tsnoam I had a look at third party ACL and RBAC implementations. Though there are some libraries that seem well maintained, they mostly seem

  • tailored for website applications
  • not to allow for easy dynamic editing of roles
  • have rather involved persistence integrations

My impression may well be based on lack of expertise, but to me it seems cleaner to have a custom implementation tailored to our needs than to rely on those third party implementations.

@Bibo-Joshi Bibo-Joshi removed this from the 12.6 milestone Jun 12, 2020
@tsnoam
Copy link
Member

tsnoam commented Oct 15, 2020

just a thought:
would it be possible to implement roles using decorators?
user who wants a handler with a certain role, can just decorate it with something to do that magic.

classic for a contrib repo, btw ;-)

@Bibo-Joshi
Copy link
Member Author

classic for a contrib repo, btw ;-)

decorators? yeah, sounds about right

just a thought:
would it be possible to implement roles using decorators?
user who wants a handler with a certain role, can just decorate it with something to do that magic.

one would need to decorate every callback … We just made a point not to use decorators on callbacks with deprecating @run_async :D
but one could think of a Handler-Wrapper:

class RolesHandlerWrapper:
    def __init__(base_class, roles, *args, **kwargs):
        super().__init__(*args, **kwargs)
        self.roles = roles

    def check_update(update):
       if self.roles(update):
            return super().check_update(update)
       return False

@tsnoam
Copy link
Member

tsnoam commented Oct 15, 2020

@Bibo-Joshi
I don't like decorators.
But contrib? That can be a jungle AFAIC.

I like your idea for a HandlerWrapper. That can be contrib as well.

Bottom line: contrib.

@Bibo-Joshi
Copy link
Member Author

Closing to be revisited after #1912 is done. v13 changed so much, I can probably start from scratch anyway :D

@Bibo-Joshi Bibo-Joshi closed this Oct 15, 2020
@github-actions github-actions bot locked and limited conversation to collaborators Oct 17, 2020
@Bibo-Joshi Bibo-Joshi deleted the roles branch October 27, 2020 16:44
@Bibo-Joshi Bibo-Joshi added 🔌 enhancement pr description: enhancement and removed enhancement labels Nov 3, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
🔌 enhancement pr description: enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature suggestion: Authentication
2 participants