chore: Add Mattermost webhook handler #10237
Conversation
Dschoordsch
force-pushed
the
feat/10219/mattermostPlugin
branch
from
5a14b06
to
390f5d7
Compare
|
@Dschoordsch Do you need something from me for this PR? |
|
@dbumblis-parabol I don't think so. I think you were automatically added because I've added a new environment variable. We will not use this in our SaaS though. |
| res | ||
| }) | ||
|
|
||
| const {query, variables, email} = body ?? {} |
There was a problem hiding this comment.
For this particular setup we blindly trust the mattermost instance to send valid emails and assume emails on our end are also verified. This can only work with private instances at the moment, so it's the admins responsibility to setup the infrastructure accordingly.
Dschoordsch
force-pushed
the
feat/10219/mattermostPlugin
branch
from
5e39983
to
b3eee10
Compare
Also we're using no the public graphql interface with a user token generated from the submitted email from mattermost. So we still trust mattermost, but don't want to have the rights escalated so the rest of the permission checks still work. The queries need to be compiled to work in production.
Dschoordsch
force-pushed
the
feat/10219/mattermostPlugin
branch
from
b3eee10
to
fb26a66
Compare
| import sendToSentry from '../../utils/sendToSentry' | ||
|
|
||
| const MATTERMOST_SECRET = process.env.MATTERMOST_SECRET | ||
| const PORT = __PRODUCTION__ ? process.env.PORT : process.env.SOCKET_PORT |
There was a problem hiding this comment.
is this conditional the reason we can't reuse https://github.com/ParabolInc/parabol/blob/f221880dac24332874939b6c0a3f7328f4b76023/packages/server/appOrigin.ts?
There was a problem hiding this comment.
Yes, same conditional as in
parabol/packages/server/server.ts
Line 50 in 2c49dce
| } | ||
| > = { | ||
| meetingTemplates: { | ||
| query: ` |
There was a problem hiding this comment.
+1 it'd be nice to validate these strings so if the schema changes we're alerted https://www.loom.com/share/effc307dce5b42e5992e36c7e7c9aee6?sid=bf22c73b-cac7-48ba-a110-d79bb0a3cdc3
we could add this eslint plugin to do the work for us: https://the-guild.dev/graphql/eslint/docs
note:
i had to update my graphql.config.js file to the following:
module.exports = {
schema: './packages/server/graphql/public/schema.graphql',
documents: './packages/server/**/*'
}There was a problem hiding this comment.
Yes, that would be nice. I couldn't get the linting rule running in a reasonable amount of time, so I will add it in a separate PR.
| @@ -55,7 +55,7 @@ const rewriteIndexHTML = () => { | |||
| github: process.env.GITHUB_CLIENT_ID, | |||
| google: process.env.GOOGLE_OAUTH_CLIENT_ID, | |||
| googleAnalytics: process.env.GA_TRACKING_ID, | |||
| mattermostDisabled: process.env.MATTERMOST_DISABLED === 'true', | |||
| mattermostDisabled: !!process.env.MATTERMOST_SECRET || process.env.MATTERMOST_DISABLED === 'true', | |||
There was a problem hiding this comment.
+1 i'm probably not thinking about this correctly.
if mattermost is disabled, how do they turn it on?
There was a problem hiding this comment.
This env is used to hide the row in the team integrations UI. With these variables set, the plugin will be available on the whole instance. Notification settings will be configured in Mattermost.
This version will not use the sharedSecret integration provider I've added in another PR. That would be a cleaner solution for the future, but I'll go a simpler route for now.
Description
Fixes #10219
We verify that messages from mattermost are signed correctly but then totally trust the mattermost instance that they verify the email.
Demo
[If possible, please include a screenshot or gif/video, it'll make it easier for reviewers to understand the scope of the changes and how the change is supposed to work. If you're introducing something new or changing the existing patterns, please share a Loom and explain what decisions you've made and under what circumstances]
Testing scenarios
[Please list all the testing scenarios a reviewer has to check before approving the PR]
Scenario A
Scenario B
Final checklist