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

CLI permissions #4

Open
txomon opened this issue Jul 10, 2018 · 1 comment
Open

CLI permissions #4

txomon opened this issue Jul 10, 2018 · 1 comment
Assignees
Labels
kind/CLI Improvements to the cli interface to users size/M T-shit size for tasks that take less than a week type/enhancement New feature or improvement that doesn't add value to final user
Projects

Comments

@txomon
Copy link
Owner

txomon commented Jul 10, 2018

Use case and definition of done

There is a need to avoid having everyone running commands through the chat. We need to have a way to only let certain commands be run by users.

The MVP needs to allow 2 execution levels, and be possible to add a few others with little to no effort later, as other features are available.


Implementation suggestion

The suggestion before was to use Yosai, but after an analysis of the structure of the project, we would only be using the Authz bits, which run in full sync mode, besides that making use of it for 20 lines of code is not worth it.

There is https://github.com/txomon/whoiam that has a demo on overloading the AWS IAM for authz. This would be a side project for an interface, but underlying authz mechanics could be easily used here. However, the cost vs the benefit of development is really high, making this solution unsuitable.

A simple solution to me would be to have a authz level assigned to each user, and that via the level we filter some commands out. Also, this should rather be in abot I think than in mosbot, therefore justifying a simpler solution for our usecase.

@txomon txomon added kind/CLI Improvements to the cli interface to users type/enhancement New feature or improvement that doesn't add value to final user labels Jul 10, 2018
@txomon txomon added this to To do in Mosbot Jul 10, 2018
@txomon txomon added the size/M T-shit size for tasks that take less than a week label Jul 12, 2018
@txomon txomon moved this from To do to In progress in Mosbot Jul 12, 2018
@txomon txomon self-assigned this Jul 12, 2018
@txomon
Copy link
Owner Author

txomon commented Jul 15, 2018

So I have been looking at possible implementations and I have realised there is a limitation at the moment with the interface we are using for commands.

Click apps SoA

There are two limitations coming from Click.

First one is that they don't have the need to limit the access to commands, and they don't know who it is executing the command, meaning that there is only a user executing it, and it's available through the os API for the program internally to query. Therefore, they don't support having any kind of permissions in their API (nor there is any reason for them to).

To solve this, Click provides a context object that can be passed around the commands. A solution would be to have aBot set the event of the command into the context, and let a decorator implemented in MosBot retrieve the event. However, this is where we reach the second click limitation. They are using the LocalProxy implementation from Flask, making it terrible for us, because that implementation is broken in asyncio. It uses the current thread as the key to retrieve a context, and we would have collisions between coroutines in the same ioloop. Also, it doesn't seem like click is really actively accepting improvements pallets/click#938 (comment).

Solutions

These are the solutions that have ocurred to me

Fork click

One of the solutions we could go through is to fork click, and delete most of it's functionality that doesn't apply to us, but we would be missing any future updates (IDK if that's important, I'm quite frustrated with them)

Continue patching click

Abot already has an extension of click, by overriding most of the classes with custom mixins that provide the functionality we need. This solution doens't really make me happy and I would avoid it even more than the fork. I hate extending things that might break when they do a change we are not aware of.

Use contextvars

Python3.7 has a PEP that goes along the lines of what I did before with setting up a coroutine level context rather than a thread one. However the limitation of that one is was that context didn't propagate to new tasks from their parents. Python3.7 comes with builtin context variables, that in theory solve this problem. I don't like the idea of generating our own context by hand, but the alternatives are not better at all.

We would be having a flask-like requests variable (or current event, or whatever) that would return the current event for whatever function tries to get the value through that context var. Problem is that we need to run python3.7 for that.


Does anyone have better ideas? Because this is literally everything I could think about :/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/CLI Improvements to the cli interface to users size/M T-shit size for tasks that take less than a week type/enhancement New feature or improvement that doesn't add value to final user
Projects
Mosbot
  
In progress
Development

No branches or pull requests

1 participant