Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign up[apps] add a slack streamalert app #764
Conversation
…ith new data. Unit test of entry filtering added
Added unit tests to get 100 percent coverage for the slack app
|
Hey @GarretReece, we'll review soon |
|
hey @GarretReece sorry for the delay - going to be looking at this very soon!! |
|
I'll let @ryandeivert , the app expert, give the final approval, but this looks great to me. Nice work! |
| 3. Scroll to the ``Scopes`` section, click on the dropdown box under ``Select Permission Scopes``, and type ``admin`` to bring up the administrator scope (labeled ``Administer the workspace``). Select it, then click ``Save changes``. | ||
| 4. Scroll to the top of that same page and click on ``Install App to Workspace``. Click ``Authorize`` on the next dialog. You should be returned to the ``OAuth & Permissions`` page. | ||
| 5. The bearer token is the string labeled with ``OAuth Access Token`` and beginning with ``xoxp-``. It's what's needed to authorize the Slack StreamAlert app. | ||
|
|
austinbyers
Jun 13, 2018
Contributor
Thanks for the explicit instructions here!
Thanks for the explicit instructions here!
|
hey @GarretReece nice work!! I just did a first pass and this is largely looking awesome. a few nit-picky comments for you :) |
| @@ -1,3 +1,19 @@ | |||
| How to set up the slack app | |||
ryandeivert
Jun 13, 2018
Contributor
this information is great to have here, but could this also be added to a section in the docs/ so it lands on streamalert.io?
this information is great to have here, but could this also be added to a section in the docs/ so it lands on streamalert.io?
| LOGGER.exception('Received bad response from slack') | ||
| return False | ||
|
|
||
| if not u'ok' in response.keys() or not response[u'ok']: |
ryandeivert
Jun 13, 2018
Contributor
can you omit the unicode u prefix here (and elsewhere). technically it's fine, just overly verbose.
also, to be more consistent with elsewhere, the can this please be changed to:
if 'ok' not in response or not response['ok']...
or: if not response.get('ok') would even do
can you omit the unicode u prefix here (and elsewhere). technically it's fine, just overly verbose.
also, to be more consistent with elsewhere, the can this please be changed to:
if 'ok' not in response or not response['ok']...
or: if not response.get('ok') would even do
| results = self._filter_response_entries(response) | ||
|
|
||
| if not self._more_to_poll: | ||
| self._last_timestamp = int(time.time()) |
ryandeivert
Jun 13, 2018
Contributor
This value can be updated even if there the _more_to_poll prop is true. Updating it here will make sure it gets updated in the saved stated, and will prevent missing or duplicative data in the event of a function timeout.
This value can be updated even if there the _more_to_poll prop is true. Updating it here will make sure it gets updated in the saved stated, and will prevent missing or duplicative data in the event of a function timeout.
| def _sleep_seconds(cls): | ||
| """Return the number of seconds this polling function should sleep for | ||
| between requests to avoid failed requests. The Slack team.integrationLog API | ||
| has Tier 2 limiting, which is 20 requests per minute. |
ryandeivert
Jun 13, 2018
Contributor
good find!!
good find!!
| self._next_page += 1 | ||
| return response['paging']['pages'] > response['paging']['page'] | ||
|
|
||
|
|
ryandeivert
Jun 13, 2018
Contributor
nitpick- there's an extra line here
nitpick- there's an extra line here
| return False | ||
|
|
||
| def _check_for_more_to_poll(self, response): | ||
| '''if we hit the maximum possible number of returned entries, there may still be more |
ryandeivert
Jun 13, 2018
Contributor
Please use double-quotes (""") to surround docstrings
Please use double-quotes (""") to surround docstrings
| raise NotImplementedError("Subclasses must implement the _filter_response_entries method") | ||
|
|
||
| def _get_request_data(self): | ||
| '''The Slack API takes additional parameters to its endpoints in the body of the request. |
ryandeivert
Jun 13, 2018
Contributor
Please use double-quotes (""") to surround docstrings
Please use double-quotes (""") to surround docstrings
| response['paging']['pages'] == response['paging']['page']) | ||
|
|
||
| def _filter_response_entries(self, response): | ||
| """The slack endpoints don't provide a programatic way to filter for new results, |
ryandeivert
Jun 13, 2018
Contributor
spelling: programatic --> programmatic
spelling: programatic --> programmatic
| return 'slack' | ||
|
|
||
| @classmethod | ||
| def date_formatter(cls): |
ryandeivert
Jun 13, 2018
Contributor
If not special formatting is needed, you should be able to omit this method altogether (it's not an abstractmethod that is required to be implemented)
If not special formatting is needed, you should be able to omit this method altogether (it's not an abstractmethod that is required to be implemented)
| @staticmethod | ||
| def _get_sample_access_logs(): | ||
| """Sample logs collected from the slack api documentation""" | ||
| return { |
ryandeivert
Jun 13, 2018
Contributor
a bit of a nit, but to be consistent with the codebase: can you ensure all string literals use single quotes (') not double quotes? and omit the leading u prefix. there are a few other instances within this file as well.
a bit of a nit, but to be consistent with the codebase: can you ensure all string literals use single quotes (') not double quotes? and omit the leading u prefix. there are a few other instances within this file as well.
|
LGTM!! Awesome work @GarretReece. Thanks for doing a test deploy as well |
ryandeivert
changed the title
to: @airbnb/streamalert-maintainers
cc: @jacknagz
size: medium
Background
StreamAlert currently doesn't have an app that collects Slack logs.
Changes
Testing
Unit tests and schema validation tests included. App has been deployed to my test environment and works.