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.

Sign up for GitHub

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

Jump to bottom

Code Refactor #133

Open
wants to merge 26 commits into
base: master
Choose a base branch
from
Open

Code Refactor #133

wants to merge 26 commits into from

Conversation

silverbp
Copy link

@silverbp silverbp commented Mar 6, 2017 •
edited
Loading

This refactor includes (resolves #128 and #114):

  • pulling all the chatops specific code into a messaging_handler which is then pulled in once and also each messaging_handler is separated out into its own file for better organization.
  • changing it to support a listener per action-alias so that you can support hubot middleware plugins such as hubot-auth-middleware-ext (this was a pretty substantial change)
  • adding an env variable called ST2_ALIAS_PACK_RESTRICTION which is a comma separated list that restricts it to importing only from the packs in that list (this was easier than disabling the alises I didn't want which got reenabled I believe at one point)
  • if you define an extra/hubot_auth element in your action alias, it will take that data and add listener opts that the hubot-middleware-auth-ext plugin can pick up and use for authentication
  • used event emitters to better decouple code
  • you can hide chatops commands from help by setting a representation with no display

if you set something like this in your env file:

export ST2_ALIAS_PACK_RESTRICTION=ems
export HUBOT_AUTH_ADMIN=U111111,U222222 # chatops id, in this case slack id 
export HUBOT_AUTH_MIDDLEWARE_ENVIRONMENT=production # if you have multiple stackstorm instances in various envs you can use this to control which aliases will run in which envs

then configure your action-aliases to look like the following, it will add role/room authentication to the action-alias. (hubot-auth-middleware-ext also support different environments as well)

---

name: some_action_alias
pack: some_pack
description: "this alias does some work"
action_ref: "some_pack.hello_world"
formats:
  - "hello wolrd {{ name }}"
extra:
  hubot_auth:
    roles:
      - devops
    rooms:
      - general

roles and/or rooms are optional although it might be pointless if you don't have either of them and still define the hubot_auth part. (it won't do auth if you leave it all out)

https://github.com/HelloFax/hubot-auth-middleware (should note that the npm package has ext on the end of it, the directions are not accurate on the package name)

I renamed the npm package to hubot-stackstorm-auth for now so it's easier to use.

If you want to persist the roles, you'll need to install redis.

cd /opt/stackstorm/chatops
sudo npm install hubot-stackstorm-auth
sudo npm install hubot-auth
sudo npm install hubot-auth-middleware-ext
sudo npm install hubot-redis-brain

the external-scripts.json would then look like this:

[
  "hubot-redis-brain",
  "hubot-stackstorm-auth",
  "hubot-auth",
  "hubot-auth-middleware-ext",
  "hubot-help"
]

obviously installing hubot-stackstorm from this repo instead of npm.

Remaining things to do:

  • Add Duo Auth back in
  • Resolve merge conflicts which I believe is the Mattermost addition
  • Support reloading of the action-aliases without restarting the st2chatops service
  • Get Tests working

@emedvedev
Copy link
Contributor

Incredible! Awesome work.

I'll try to find the time to review this asap, and figure out a way to include it in the st2chatops package as well.

@silverbp
Copy link
Author

silverbp commented Mar 6, 2017

@emedvedev I noticed on the original code there was support for sending a signal to reload the commands and then there's also a timer that reloads them by default every 120 seconds, does something in the st2 stack trigger the chatops service to reload commands and/or do we need support for reloading commands both ways?

lakshmi-kannan
lakshmi-kannan reviewed Apr 4, 2017
View reviewed changes
package.json
"description": "A hubot plugin for integrating with StackStorm event-driven infrastructure automation platform.",
"version": "0.4.5",
"author": "StackStorm, Inc. <info@stackstorm.com>",
"name": "hubot-stackstorm-auth",
Copy link
Contributor

Choose a reason for hiding this comment

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

What is going on in this file? :)

Did you intend to open this PR here or your fork?

Copy link
Author

Choose a reason for hiding this comment

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

I have this automated to setup our stackstorm box and needed a different package name in npm. I can branch and change that back for the merge. Wasn't sure when this was going to get more attention.

@shreyas-shinde shreyas-shinde mentioned this pull request Apr 15, 2018
Open
@blag
Copy link
Contributor

blag commented Aug 8, 2018

@silverbp Both ways of (re) loading commands will need to be supported in this rewrite.

I like this, but can you rebase on current master?

@jinpingh jinpingh mentioned this pull request Jun 27, 2019
Open
@blag blag mentioned this pull request Jul 17, 2019
Merged
@blag
Copy link
Contributor

blag commented Jul 19, 2019

I have refactored the chat provider adapters in #186 using part of this PR as a blueprint. Hopefully this makes it easier for the community to add chat provider adapters for other platforms.

Hopefully we can refactor scripts/stackstorm.js (which is now src/stackstorm.js) soon.

@silverbp Thank you for taking the time to write this PR, I have learned a great deal about Javascript coding practices from reading your code.

Although we won't be using this PR directly when refactoring src/stackstorm.js, I will keep this PR open until we do.

@blag blag mentioned this pull request Jul 19, 2019
Merged
13 tasks
@m4dcoder m4dcoder requested review from lakshmi-kannan and blag and removed request for enykeev, m4dcoder and lakshmi-kannan July 25, 2019 21:29
@blag
Copy link
Contributor

blag commented Jul 25, 2019

The scripts/stackstorm.js module was moved, split up, and partially refactored in #187.

Further Work

Now that StackStormApi has been turned into a JS class, it should be easier to write unit tests for each of its public methods, and to update it to use new style JS classes with the class and extends keywords.

  • Change the stop() method to shutdown the previously opened st2stream instead of opening a new one and immediately shutting it down - requires manual testing to make sure it doesn't break anything, or add a comment that st2client.js caches the connection and reuses it, so opening the same connection simply reuses the old connection and correctly shuts it down
  • Add unit tests for StackStormApi public methods
  • Investigate moving things into the stackstorm.js wrapper
    • the process.on unhandled rejection handler registration
    • the robot.error handler registration
    • the robot.respond handler registration
    • the robot.router.post endpoint (see stackstorm.js:51 from this PR)
    • any other endpoints (I expect at least one to be added for ChatOps authentication as part of ChatOps RBAC)
  • Decouple stackstorm_api.js from stackstorm.js a bit, similar to how it is implemented here
  • Return the promises in loadCommands and start

@blag
Copy link
Contributor

blag commented Jul 25, 2019

I'm leaving this PR open to track refactoring progress. I'll close it once the refactoring inspired by this PR is complete.

@blag blag added the stale label Jul 31, 2019
@blag
Copy link
Contributor

blag commented Jul 31, 2019

Marking as stale only so Pull Reminder doesn't keep pinging me about this.

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

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

@lakshmi-kannan lakshmi-kannan lakshmi-kannan left review comments

@blag blag Awaiting requested review from blag

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
stale
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

RBAC for ChatOps in StackStorm
5 participants
@silverbp @emedvedev @blag @CLAassistant @lakshmi-kannan