# Code review request for \`slackdoor\` plugin

**URL:** https://meta.discourse.org/t/code-review-request-for-slackdoor-plugin/44253
**Category:** Development
**Created:** [May 14, 2016, 8:28pm UTC](https://meta.discourse.org/t/code-review-request-for-slackdoor-plugin/44253 "2016-05-14T20:28:04Z")
**Posts on this page:** 11
**Page:** 1

<div class="post-metadata">

### Author: ![mcwumbly](https://sea3.discourse-cdn.com/meta/user_avatar/meta.discourse.org/mcwumbly/32/103861_2.png) [@mcwumbly](https://meta.discourse.org/u/mcwumbly)
#### Post date: [May 14, 2016, 8:28pm UTC](https://meta.discourse.org/t/code-review-request-for-slackdoor-plugin/44253/1 "2016-05-14T20:28:05Z")

</div>

Hey there…

I’m looking for some extra pairs of eyes on the plugin I’ve implemented for this feature:

[Allow Slack to unfurl/expand links to “login-required” Discourse instance](https://meta.discourse.org/t/allow-slack-to-unfurl-expand-links-to-login-required-discourse-instance/39547)

Slack posts a request to `yourdomain.com/slackdoor` over https (assuming your site has it enabled) with an `x-www-form-urlencoded` body that includes a token, which is generated on their side when you set up the [outgoing webook](https://api.slack.com/outgoing-webhooks).

```plaintext
token=XXXXXXXXXXXXXXXXXX
team_id=T0001
team_domain=example
channel_id=C2147483705
channel_name=test
timestamp=1355517523.000005
user_id=U2147483697
user_name=Steve
text=googlebot: What is the air-speed velocity of an unladen swallow?
trigger_word=googlebot:

```

The request can’t include an API Key or username, but a required SiteSetting for username determines which user is used to access the topic.

These are all checked `before_filter` methods in the Controller class:

> <https://github.com/mcwumbly/discourse-slackdoor/blob/master/plugin.rb#L38-L49>

In order to bypass the default authentication logic, I have overridden these methods in the plugin’s controller:

> <https://github.com/mcwumbly/discourse-slackdoor/blob/master/plugin.rb#L86-L95>

Check out this file for the full plugin: [discourse-slackdoor/plugin.rb at master · mcwumbly/discourse-slackdoor · GitHub](https://github.com/mcwumbly/discourse-slackdoor/blob/master/plugin.rb)

Please let me know if you see any issues with this approach… Thanks!

---

<div class="post-metadata">

### Author: ![Nick](https://sea3.discourse-cdn.com/meta/user_avatar/meta.discourse.org/nick/32/235642_2.png) [@Nick](https://meta.discourse.org/u/Nick)
#### Post date: [May 16, 2016, 7:41pm UTC](https://meta.discourse.org/t/code-review-request-for-slackdoor-plugin/44253/2 "2016-05-16T19:41:55Z")

</div>

Hey there, I’m taking a look at your code now! Good work so far. I’ll have some feedback up tonight. As a heads-up, I’ll be working on an official Slack plugin (official as in sponsored by Discourse, not by Slack). It’s intended to be very fully-featured and go a bit deep. I don’t want to undermine the great work you’ve done so far, so getting you involved would be great if you’d like.

---

<div class="post-metadata">

### Author: ![mcwumbly](https://sea3.discourse-cdn.com/meta/user_avatar/meta.discourse.org/mcwumbly/32/103861_2.png) [@mcwumbly](https://meta.discourse.org/u/mcwumbly)
#### Post date: [May 17, 2016, 4:17am UTC](https://meta.discourse.org/t/code-review-request-for-slackdoor-plugin/44253/3 "2016-05-17T04:17:25Z")

</div>

Cool, thanks Nick.

If this functionality is supported by the plugin you’re working on, I would be happy to switch over to something officially supported by the team.

In the mean time, this is working well, but my main concern right now is whether I’m inadvertently introducing some risk security-wise that isn’t obvious, so if you see anything along those lines that you’d recommend changing or taking a closer look at, please let me know.

---

<div class="post-metadata">

### Author: ![fantasticfears](https://sea3.discourse-cdn.com/meta/user_avatar/meta.discourse.org/fantasticfears/32/119608_2.png) [@fantasticfears](https://meta.discourse.org/u/fantasticfears)
#### Post date: [May 17, 2016, 5:24am UTC](https://meta.discourse.org/t/code-review-request-for-slackdoor-plugin/44253/4 "2016-05-17T05:24:48Z")

</div>

Accepting incoming webhooks, that’s something I am interested. But it’ll be general. Pre-defined actions, some policy to process the webhooks. I probably will use that for testing webhooks among several Discourse instances.

> [@mcwumbly](#):
>
> The request can’t include an API Key or username, but a required SiteSetting for username determines which user is used to access the topic.

> <https://github.com/mcwumbly/discourse-slackdoor/blob/master/plugin.rb#L86-L95>

It’s better to use `skip_before_{filter,action}`, both simple and clear. The risk comes from you don’t know how to authenticate an incoming request. If one Discourse instance is private, this endpoint would open a door for the world by guessing possible url patterns.

> <https://github.com/mcwumbly/discourse-slackdoor/blob/master/plugin.rb#L38-L49>

If checking SiteSetting is needed, batching all of them in one function could be enough

---

<div class="post-metadata">

### Author: ![Nick](https://sea3.discourse-cdn.com/meta/user_avatar/meta.discourse.org/nick/32/235642_2.png) [@Nick](https://meta.discourse.org/u/Nick)
#### Post date: [May 17, 2016, 7:07pm UTC](https://meta.discourse.org/t/code-review-request-for-slackdoor-plugin/44253/5 "2016-05-17T19:07:49Z")

</div>

I don’t see any major security problems I don’t think - but I am only looking at the obvious, myself. @fantasticfears covered some things I didn’t know myself.

As long as the token they give is nice and cryptographically strong and HTTPS is being used, I think the bases are covered.

It’d be nice to have a separate namespace for external services / webhooks in the future and a manager for these, as @fantasticfears seems to be saying (if I am understanding you correctly).

---

<div class="post-metadata">

### Author: ![fantasticfears](https://sea3.discourse-cdn.com/meta/user_avatar/meta.discourse.org/fantasticfears/32/119608_2.png) [@fantasticfears](https://meta.discourse.org/u/fantasticfears)
#### Post date: [May 17, 2016, 8:46pm UTC](https://meta.discourse.org/t/code-review-request-for-slackdoor-plugin/44253/6 "2016-05-17T20:46:16Z")

</div>

Me is saying developing web application should consider authentication/authorization by all means.

```ruby
Rails.application.routes.recognize_path("/t/whatever/1234/56"

```

The entry point in this case is a url. By forging a pair of topic\_id/post\_id, I could query any post where I want to see.  
I could know post excerpt, topic title, poster username. And even more, private message and staff category is also on my palm.  
HTTPS can only help to transmit the data safely. It can’t prevent information spreading out if you intend to do so

---

<div class="post-metadata">

### Author: ![mcwumbly](https://sea3.discourse-cdn.com/meta/user_avatar/meta.discourse.org/mcwumbly/32/103861_2.png) [@mcwumbly](https://meta.discourse.org/u/mcwumbly)
#### Post date: [May 18, 2016, 4:12am UTC](https://meta.discourse.org/t/code-review-request-for-slackdoor-plugin/44253/7 "2016-05-18T04:12:42Z")

</div>

The user configured in the SiteSettings can limit the exposure to private messages and staff categories.

And the token configured in the SiteSetting had to match the one posted in the body.

Does that address your concerns? Or do you think there is a gap somewhere still that I’m missing?

---

<div class="post-metadata">

### Author: ![fantasticfears](https://sea3.discourse-cdn.com/meta/user_avatar/meta.discourse.org/fantasticfears/32/119608_2.png) [@fantasticfears](https://meta.discourse.org/u/fantasticfears)
#### Post date: [May 18, 2016, 5:07am UTC](https://meta.discourse.org/t/code-review-request-for-slackdoor-plugin/44253/8 "2016-05-18T05:07:18Z")

</div>

Sorry I missed the `TopicView`! 😲 cc @Nick

Sure that would do the authorization. And `token` configured on Slack end can be used as a certificate. HTTPS would prevent all kinds of passive attack.

---

<div class="post-metadata">

### Author: ![mcwumbly](https://sea3.discourse-cdn.com/meta/user_avatar/meta.discourse.org/mcwumbly/32/103861_2.png) [@mcwumbly](https://meta.discourse.org/u/mcwumbly)
#### Post date: [May 18, 2016, 5:12am UTC](https://meta.discourse.org/t/code-review-request-for-slackdoor-plugin/44253/9 "2016-05-18T05:12:38Z")

</div>

Cool… Thanks so much for taking the time to go through this and give your feedback @fantasticfears.

I’ll put some time into addressing your other code-cleanliness feedback and get a README together.

That should put it in a state where others can use it while development gets underway for a more fully-featured Slack plugin.

@Nick, please feel free to use anything in here that you find useful, or reach out to me if you want feedback on the work you’re doing as you get into it.

---

<div class="post-metadata">

### Author: ![Nick](https://sea3.discourse-cdn.com/meta/user_avatar/meta.discourse.org/nick/32/235642_2.png) [@Nick](https://meta.discourse.org/u/Nick)
#### Post date: [May 18, 2016, 5:51pm UTC](https://meta.discourse.org/t/code-review-request-for-slackdoor-plugin/44253/10 "2016-05-18T17:51:20Z")

</div>

> [@fantasticfears](#):
>
> Sorry I missed the TopicView! 😲 cc @Nick

Hah. Glad 😄 Thought I missed something myself. All clear.

> [@mcwumbly](#):
>
> @Nick, please feel free to use anything in here that you find useful, or reach out to me if you want feedback on the work you’re doing as you get into it.

Great to hear! I will. Thank you for the work you’ve put in. When I have the ground-floor set up I’ll link it in this thread.

---

<div class="post-metadata">

### Author: ![mcwumbly](https://sea3.discourse-cdn.com/meta/user_avatar/meta.discourse.org/mcwumbly/32/103861_2.png) [@mcwumbly](https://meta.discourse.org/u/mcwumbly)
#### Post date: [May 22, 2016, 8:33pm UTC](https://meta.discourse.org/t/code-review-request-for-slackdoor-plugin/44253/11 "2016-05-22T20:33:49Z")

</div>

The canonical plugin topic is now here: [Discourse Slackdoor Plugin](https://meta.discourse.org/t/discourse-slackdoor-plugin/44632)