-
Notifications
You must be signed in to change notification settings - Fork 53
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
cEP-0015.md: corobo enhancement #113
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,257 @@ | ||
# corobo Enhancement (security, tests and configurability) | ||
|
||
| Metadata | | | ||
| -------- | -------------------------------------------------------- | | ||
| cEP | 15 | | ||
| Version | 1.0 | | ||
| Title | corobo Enhancement (security, tests and configurability) | | ||
| Authors | Nitanshu Vashistha <mailto:[email protected]> | | ||
| Status | Proposed | | ||
| Type | Process | | ||
|
||
## Abstract | ||
|
||
This cEP describes the details of enhancement of | ||
[corobo](https://github.com/coala/corobo) in terms of security, tests, | ||
configurability and the new plugins that are to be added to corobo as part | ||
of the | ||
[GSoC Project](https://summerofcode.withgoogle.com/projects/#6603667076022272). | ||
|
||
## Security | ||
|
||
Security has been one of the major concerns due to some past experiences. | ||
These are the changes that will be implemented to the existing plugins | ||
in order to harden the security. | ||
|
||
1. Give developers the right to invite newcomers to the organization. | ||
This will involve changing the bot command to `corobo invite <@username>`. | ||
|
||
2. Remove the automatic invite due to `Hello World` message. | ||
|
||
3. Remove `invite me` bot command. | ||
|
||
4. Stop newcomers from getting assigned to more than one newcomer issue. | ||
|
||
5. Make all LabHub plugins require being a member of the organization. This | ||
will be done by creating a `members_only` decorator to check if the user is | ||
a member of team or not. The decorator can be used on any bot command which | ||
needs to be under the accessibility of an organization member only. | ||
|
||
```py | ||
def members_only(func): | ||
@wraps(func) | ||
def wrapper(*args, **kwargs): | ||
# plugin instance | ||
instance = args[0] | ||
msg = args[1] | ||
user = msg.frm.nick | ||
for team in instance.team_mapping().values(): | ||
if team.is_member(user): | ||
yield from func(*args, **kwargs) | ||
return | ||
yield ('You need to be a member of this organization' | ||
' to use this command.') | ||
return wrapper | ||
``` | ||
|
||
6. Add callback message to warn users who are spamming the room. | ||
|
||
7. Add bot command to ban a user from all rooms at once. The goal of | ||
this feature is to prevent spamming as someone with many accounts | ||
can't be stopped to join a room. | ||
|
||
## Tests | ||
|
||
The default TestBase provided by Errbot is not enough for testing plugins like | ||
LabHub, which required intensive mocking. | ||
|
||
This will involve making changes upstream in | ||
[Errbot](https://github.com/errbotio/errbot/) and extend the existing testing | ||
infrastructure to implement better testing for plugins like LabHub. | ||
|
||
Errbot provides a pytest fixture `testbot` for testing purposes and to interact | ||
with the message queue. Extending the testbot and adding `inject_mock` method | ||
so that mocked objects can be injected into the plugin by passing a dictionary | ||
containing the `PluginName`, `field_name` and `Mocked Object`. The | ||
`inject_mock` method will inject a property on the plugin object and discard | ||
the assignments from the plugin itself. | ||
|
||
```py | ||
def test_command(testbot): | ||
mocks = {'PluginName': {'field_name1': fakeObj1, | ||
'field_name2': fakeObj2}} | ||
testbot.inject_mock(mocks) | ||
assert 'something' in testbot.exec_command('!my_command') | ||
``` | ||
|
||
## Configurability | ||
|
||
corobo has a potential to be used by other organizations for similar tasks like | ||
onboarding and automation. Currently, it is not configurable and many plugins | ||
are still very coala specific. Making it more configurable will allow other | ||
organizations to adapt corobo to cater their needs. | ||
|
||
Making existing coala specific plugins generic for other organizations and | ||
letting them configure the plugins as per their needs will ensure | ||
configurability. | ||
|
||
Errobot provides [plugin configuration](http://errbot.io/en/latest/user_guide/plugin_development/configuration.html) | ||
through the built-in `!config` command which can be used by other organizations | ||
to configure the plugins as per their needs. | ||
|
||
List of Plugins which are coala specific and can be generalized: | ||
|
||
1. `LabHub` plugin commands are meant to work for a coala specific team names | ||
maintainers, developers, newcomers and coala repositories. | ||
|
||
Issue Link: <https://github.com/coala/corobo/issues/382> | ||
|
||
2. `explain` currently uses hardcoded coala explanations. Configuring | ||
sub-directories `explain/genric` and `explain/<org name>` so that | ||
other organizations can include their culture-specific explanations. | ||
|
||
3. `searchdocs` uses API_DOCS and USER_DOCS links which are constant for coala. | ||
Other organizations will be able to initialize their own documentation | ||
links as per their requirement. | ||
|
||
Issue Link: <https://github.com/coala/corobo/issues/380> | ||
|
||
4. `community_stats` will be moved out from LabHub as a separate plugin since | ||
it can also be used by other organizations for analytics overview. | ||
|
||
Issue Link: <https://github.com/coala/corobo/issues/361> | ||
|
||
5. `wolfram_alpha` plugin currently uses an environment variable, it will be | ||
modified to use the configuration template. | ||
|
||
Issue Link: <https://github.com/coala/corobo/issues/383> | ||
|
||
6. `answer` plugin use `ANSWER_END` environment variable as an endpoint of the | ||
answers microservice. It'll be configured so that other organizations can | ||
also adapt to it. | ||
|
||
Issue Link: <https://github.com/coala/corobo/issues/381> | ||
|
||
### Configuration Sample | ||
|
||
```py | ||
class LabHub(BotPlugin): | ||
def get_configuration_template(self): | ||
return {'GH_TEAM_NAMES': ['newcomers', 'developers', 'maintainers']} | ||
|
||
@botcmd | ||
def mycommand(self, mess, args): | ||
# oh I need my TEAM ! | ||
gh_teams = self.config['GH_TEAM_NAMES'] | ||
``` | ||
|
||
``` | ||
<BOT_PREFIX> plugin config LabHub {'GH_TEAM_NAMES': ['newbies', 'devs', 'admins']} | ||
``` | ||
|
||
This comment was marked as resolved.
Sorry, something went wrong. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't quite remember why I made this choice, but I think it was the API calls, we didn't want to make a ton of API requests every time the plugin is deactivated and activated again.. Since errbot just instantiates the plugin once, it sounded like a better choice.. not the best choice to make I guess 😅 What are the downsides of setting the attribs in the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The issue of |
||
Using the configuration templates means that the bot admin will have to | ||
configure the plugin every time the bot starts and since corobo is continuously | ||
deployed on each commit, reconfiguring again and again using the chat via a | ||
admin is not preferable. | ||
|
||
This issue can be handled by using a Mixin to pre-configure the plugins through | ||
`config.py` file. By using `DEFAULT_CONFIGS` in `config.py` file to hold the | ||
configuration dictionaries for the plugins. Eg: | ||
|
||
```py | ||
DEFAULT_CONFIG = { | ||
'LabHub': { | ||
'GH_TOKEN': os.getenv('GITHUB_TOKEN'), | ||
'GL_TOKEN': os.getenv('GITLAB_TOKEN'), | ||
'GH_ORG_NAME': os.getenv('GH_ORG_NAME'), | ||
}, | ||
'WolframAlpha': { | ||
'WA_APP_ID': os.getenv('WA_TOKEN'), | ||
}, | ||
} | ||
``` | ||
|
||
Configuration of the plugins will be done before their activation and if a | ||
plugin is already configured, the `DefaultConfigMixin` will autoconfigure the | ||
plugin from `DEFAULT_CONFIGS`. | ||
|
||
```py | ||
class DefaultConfigMixin: | ||
@property | ||
def _default_config(self) -> Optional[Mapping]: | ||
if (self.bot_config.DEFAULT_CONFIG and self.name | ||
in self.bot_config.DEFAULT_CONFIG): | ||
return self.bot_config.DEFAULT_CONFIG[self.name] | ||
|
||
def __init__(self, bot, name=None) -> None: | ||
super().__init__(bot, name = name) | ||
default_config = self._default_config | ||
if default_config and not self.config: | ||
super().configure(default_config) | ||
|
||
def get_configuration_template(self) -> Mapping: | ||
default_config = self._default_config | ||
if default_config: | ||
return default_config | ||
``` | ||
|
||
```py | ||
class MyPlugin(BotPlugin, DefaultConfigMixin): | ||
... | ||
``` | ||
|
||
Other possible ways to configure corobo were discussed [here](https://github.com/coala/corobo/issues/574). | ||
|
||
### Use of `activate` instead of `__init__` to configure plugins: | ||
|
||
Currently, most of the plugins make use of the `__init__` method for instead of | ||
`activate` for configuration. Since `__init__` is executed at load time its | ||
failure will cause the plugin to not show up. Configuring the plugins through | ||
the `activate` method will remove this possibility. | ||
|
||
Eg: labhub.py | ||
|
||
```py | ||
def __init__(self, bot, name=None): | ||
super().__init__(bot, name) | ||
|
||
teams = dict() | ||
try: | ||
gh = github3.login(token=os.environ.get('GH_TOKEN')) | ||
assert gh is not None | ||
except AssertionError: | ||
self.log.error('Cannot create github object, please check GH_TOKEN') | ||
else: | ||
self.GH3_ORG = gh.organization(self.GH_ORG_NAME) | ||
for team in self.GH3_ORG.teams(): | ||
teams[team.name] = team | ||
|
||
self._teams = teams | ||
|
||
self.IGH = GitHub(GitHubToken(os.environ.get('GH_TOKEN'))) | ||
self.IGL = GitLab(GitLabPrivateToken(os.environ.get('GL_TOKEN'))) | ||
|
||
self.REPOS = dict() | ||
``` | ||
|
||
```py | ||
def activate(self): | ||
|
||
teams = dict() | ||
try: | ||
gh = github3.login(token=os.environ.get('GH_TOKEN')) | ||
assert gh is not None | ||
except AssertionError: | ||
self.log.error('Cannot create github object, please check GH_TOKEN') | ||
else: | ||
self.GH3_ORG = gh.organization(self.GH_ORG_NAME) | ||
for team in self.GH3_ORG.teams(): | ||
teams[team.name] = team | ||
|
||
self._teams = teams | ||
|
||
self.IGH = GitHub(GitHubToken(os.environ.get('GH_TOKEN'))) | ||
self.IGL = GitLab(GitLabPrivateToken(os.environ.get('GL_TOKEN'))) | ||
|
||
super(LabHub, self).activate() | ||
``` |
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jayvdb I understand the problem but the configuration_template is there to avoid the need for admins to configure settings in some kind of configuration file, allowing configuration to happen directly through chat commands.
I guess configuring keys with empty
{TOKEN: ''}
values if they exist from the global config file will deal with this type of situation.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jayvdb according to @gbin configurations are stored in ErrBot and last saved configuration are loaded after restarting the bot.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there is still the problem of first time the bot is started.
the configuration of the corobo plugins needs to initialised somehow.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure you here you have several choices: either a restore from a backup or you can insert values into storage from errbot's command line before startup. Errbot is not a stateless system but it can use existing external DBs as storage if you wish (SQL, redis, gcd etc.).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We had a discussion on this point.
It is possible to configure the plugins before deploying the bot.
We came to the conclusion that a python file containing the configurations will be mounted to the container from the host machine and we'll run a script from inside the container that will use the provisioning commands to configure the plugins from the configurations inside the python file before the bot is deployed. So that configs are stored in the storage.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please help me out here.
http://errbot.io/en/latest/user_guide/provisioning.html
says
Can the same config variable by configured by
!config
and byerrbot --storage-merge...
?Are they a unified system? If so, show a worked example using the same config variables, e.g.
GH_TEAM_NAMES
andGH_TOKEN
andGH_ORG_NAME
, and then also state unambiguously that the bot will react to!config
changes of those critical config variables, and reload correctly with the new settings. Also specifically wrt labhub at startup for repo forks, who do not have sufficient access for the org inGH_ORG_NAME
, the plugin needs to gracefully handle failures in those config values during startup, such as tokens with incorrect scopes. There are lots of issues about this, and it is limiting the ability of newcomers to get involved in this project.I havent seen a bot plugin which has the same config variable in storage and accessible via
!config
, so pardon my skepticism and growing impatience with this problem, which was raised here 4 weeks ago. Dumping into the cEP examples which show different mechanisms being used doesnt describe a solution ... it just shows errbot has lots of available options ... and pretends they can be used in a unified way when they can not.If the can not be used in a unified way, this cEP is confusing because it is talking about two different ways of configuring the bot and not clarifying which types of config settings will be done in each of the two ways.
One possible way to solve this is to consider how it might be possible to have the same config variable in storage and accessible via
!config
. It might be a hack in corobo, like coala/corobo#380 (comment) mentioned 4 weeks ago, or it might be an enhancement to errbot if upstream are interested in solving this problem.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The configs you can set with
!plugin config
can be set with the command line under the key 'core' like the documentation shows. They are completely equivalent so I think it is relevant for your requirement for provisioning your plugins ahead of time.Plugins have a place to validate that their config is well formed by overriding
check_configuration
and will send to the bot admin a message explaining why a plugin could not start in case of failure.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If they can be used together, that is great.
Below this cEP uses
Is that incorrect? Should
LabHub
becore
?As mentioned above, I would want to see at least the same config variables in the
!config
example above and theerrbot --storage-merge
example below.It would also be desirable to have a list of the config variables that this project will be implementing, so we can see and discuss them, and verify that the planned work is sufficient for the objective of the project.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
errbot --storage-get core
will return configs of all the loaded plugins including LabHub,while
errbot --storage-get LabHub
will return config only for LabHub plugin.Similar for
--storage-merge
and--storage-set
flags,