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.

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

[Feature] slack oauth support #245

Merged
merged 12 commits into from
Feb 25, 2021
14 changes: 14 additions & 0 deletions CONTRIBUTING.md
Original file line number Diff line number Diff line change
@@ -1,3 +1,17 @@
<!-- START doctoc generated TOC please keep comment here to allow auto update -->
<!-- DON'T EDIT THIS SECTION, INSTEAD RE-RUN doctoc TO UPDATE -->
**Table of Contents** *generated with [DocToc](https://github.com/thlorenz/doctoc)*

- [Contributing to Library](#contributing-to-library)
- [Repo Layout](#repo-layout)
- [Feature Requests](#feature-requests)
- [Testing](#testing)
- [Code Conventions](#code-conventions)
- [Contributing Documentation](#contributing-documentation)
- [Document Contributors](#document-contributors)

<!-- END doctoc generated TOC please keep comment here to allow auto update -->

# Contributing to Library

## Repo Layout
Expand Down
15 changes: 15 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ A collaborative newsroom documentation site, powered by Google Docs.
- [Doc parsing](#doc-parsing)
- [Listing the drive](#listing-the-drive)
- [Auth](#auth)
- [User Authentication](#user-authentication)

<!-- END doctoc generated TOC please keep comment here to allow auto update -->

Expand Down Expand Up @@ -205,3 +206,17 @@ The tree and file metadata are repopulated into memory on an interval (currently
### Auth

Authentication with the Google Drive v3 api is handled by the auth.js file, which exposes a single method `getAuth`. `getAuth` will either return an already instantiated authentication client or produce a fresh one. Calling `getAuth` multiple times will not produce a new authentication client if the credentials have expired; we should build this into the auth.js file later to automatically refresh the credentials on some sort of interval to prevent them from expiring.

### User Authentication

Library currently supports both Slack and Google Oauth methods. As Library sites are intended to be internal to your organization, Oauth with your organization is strongly encouraged. To use Slack Oauth, specify your Oauth strategy in your `.env` file, like so:
penina-nyt marked this conversation as resolved.
Show resolved Hide resolved
```
# Slack needs to be capitalized
OAUTH_STRATEGY=Slack
Copy link
Member

Choose a reason for hiding this comment

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

Great to note this in the docs, would be interested in hearing more about capitalization in this case vs the default (google).

```
You will need to provide Slack credentials, which you can do by creating a Library Oauth app in your Slack workspace. After creating the app, save the app's `CLIENT_ID` and `CLIENT_SECRET` in your `.env` file:
```
SLACK_CLIENT_ID=1234567890abcdefg
SLACK_CLIENT_SECRET=09876544321qwerty
```
You will need to add a callback URL to your Slack app to allow Slack to redirect back to your Library instance after the user is authenticated.
15 changes: 15 additions & 0 deletions custom/README.md
Original file line number Diff line number Diff line change
@@ -1,3 +1,18 @@
<!-- START doctoc generated TOC please keep comment here to allow auto update -->
<!-- DON'T EDIT THIS SECTION, INSTEAD RE-RUN doctoc TO UPDATE -->
**Table of Contents** *generated with [DocToc](https://github.com/thlorenz/doctoc)*

- [Customizing Library](#customizing-library)
- [The `custom` Directory](#the-custom-directory)
- [Styles](#styles)
- [Text, Language, and Branding](#text-language-and-branding)
- [Middleware](#middleware)
- [Cache Client](#cache-client)
- [Layouts](#layouts)
- [Authentication](#authentication)

<!-- END doctoc generated TOC please keep comment here to allow auto update -->

# Customizing Library

## The `custom` Directory
Expand Down
16 changes: 16 additions & 0 deletions package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@
"nock": "^13.0.3",
"nodemon": "^2.0.4",
"nyc": "^15.1.0",
"passport-slack-oauth2": "^1.0.2",
"sinon": "^9.0.2",
"supertest": "^4.0.2",
"valid-url": "^1.0.9"
Expand Down
55 changes: 42 additions & 13 deletions server/userAuth.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,20 +4,47 @@ const passport = require('passport')
const session = require('express-session')
const md5 = require('md5')
const GoogleStrategy = require('passport-google-oauth20')
const SlackStrategy = require('passport-slack-oauth2').Strategy

const log = require('./logger')
const {stringTemplate: template} = require('./utils')

const router = require('express-promise-router')()
const domains = new Set(process.env.APPROVED_DOMAINS.split(/,\s?/g))

passport.use(new GoogleStrategy.Strategy({
clientID: process.env.GOOGLE_CLIENT_ID,
clientSecret: process.env.GOOGLE_CLIENT_SECRET,
callbackURL: '/auth/redirect',
userProfileURL: 'https://www.googleapis.com/oauth2/v3/userinfo',
passReqToCallback: true
}, (request, accessToken, refreshToken, profile, done) => done(null, profile)))
const authStrategies = ['google', 'Slack']
let authStrategy = process.env.OAUTH_STRATEGY
Copy link
Member

Choose a reason for hiding this comment

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

I would advocate for throwing a .toLowerCase() on this to avoid potential confusion with capitalization, as @isaacwhite mentioned in his review.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

updated the ReadMe with the problems there - Passport wasn't recognizing a call to Passport.authenticate unless slack is capitalized


const callbackURL = process.env.REDIRECT_URL || '/auth/redirect'
if (!authStrategies.includes(authStrategy)) {
log.warn(`Invalid oauth strategy ${authStrategy} specific, defaulting to google auth`)
authStrategy = 'google'
}

const isSlackOauth = authStrategy === 'Slack'
if (isSlackOauth) {
passport.use(new SlackStrategy({
clientID: process.env.SLACK_CLIENT_ID,
clientSecret: process.env.SLACK_CLIENT_SECRET,
skipUserProfile: false,
callbackURL,
scope: ['identity.basic', 'identity.email', 'identity.avatar', 'identity.team', 'identity.email']
},
(accessToken, refreshToken, profile, done) => {
// optionally persist user data into a database
done(null, profile)
}
))
} else {
// default to google auth
passport.use(new GoogleStrategy.Strategy({
clientID: process.env.GOOGLE_CLIENT_ID,
clientSecret: process.env.GOOGLE_CLIENT_SECRET,
callbackURL,
userProfileURL: 'https://www.googleapis.com/oauth2/v3/userinfo',
passReqToCallback: true
}, (request, accessToken, refreshToken, profile, done) => done(null, profile)))
}

router.use(session({
secret: process.env.SESSION_SECRET,
Expand All @@ -33,27 +60,28 @@ router.use(passport.session())
passport.serializeUser((user, done) => done(null, user))
passport.deserializeUser((obj, done) => done(null, obj))

router.get('/login', passport.authenticate('google', {
const googleLoginOptions = {
scope: [
'https://www.googleapis.com/auth/userinfo.email',
'https://www.googleapis.com/auth/userinfo.profile'
],
prompt: 'select_account'
}))
}

router.get('/login', passport.authenticate(authStrategy, isSlackOauth ? {} : googleLoginOptions))

router.get('/logout', (req, res) => {
req.logout()
res.redirect('/')
})

router.get('/auth/redirect', passport.authenticate('google'), (req, res) => {
router.get('/auth/redirect', passport.authenticate(authStrategy, {failureRedirect: '/login'}), (req, res) => {
res.redirect(req.session.authRedirect || '/')
})

router.use((req, res, next) => {
const isDev = process.env.NODE_ENV === 'development'
const passportUser = (req.session.passport || {}).user || {}

if (isDev || (req.isAuthenticated() && isAuthorized(passportUser))) {
setUserInfo(req)
return next()
Expand Down Expand Up @@ -89,10 +117,11 @@ function setUserInfo(req) {
}
return
}
const email = isSlackOauth ? req.session.passport.user.email : req.session.passport.user.emails[0].value
req.userInfo = req.userInfo ? req.userInfo : {
email: req.session.passport.user.emails[0].value,
userId: req.session.passport.user.id,
analyticsUserId: md5(req.session.passport.user.id + 'library')
analyticsUserId: md5(req.session.passport.user.id + 'library'),
email
}
}

Expand Down
71 changes: 65 additions & 6 deletions test/functional/userAuth.test.js
Original file line number Diff line number Diff line change
@@ -1,10 +1,9 @@
'use strict'

const request = require('supertest')
const {assert} = require('chai')
const sinon = require('sinon')
const express = require('express')

const log = require('../../server/logger')
const app = require('../../server/index')

/*
Expand All @@ -24,18 +23,78 @@ const regexUser = {
}

const specificUser = {
emails: [{ value: 'demo.user@demo.site.edu' }],
emails: [{value: 'demo.user@demo.site.edu'}],
id: '12',
userId: '12'
}

const unauthorizedUser = {
emails: [{ value: 'unauth@unauthorized.com' }],
emails: [{value: 'unauth@unauthorized.com'}],
id: '13',
userId: '13'
}

describe('Authentication', () => {
describe('.env specified oauth strategy', () => {
Copy link
Member

Choose a reason for hiding this comment

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

Thanks for adding these solid tests!

const sandbox = sinon.createSandbox()
beforeEach(() => {
jest.resetModules()
sandbox.stub(express.request, 'isAuthenticated').returns(false)
})

afterEach(() => {
sandbox.restore()
})

it('should warn if there is an invalid strategy specified', () => {
process.env.OAUTH_STRATEGY = 'fakjkjfdz'
Copy link
Member

Choose a reason for hiding this comment

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

😆

const spy = sandbox.spy(log, 'warn')
const appWithInvalidOauth = require('../../server/index') // need to redo app setup
return request(appWithInvalidOauth)
.get('/login')
.expect(302)
.then((res) => {
assert.isTrue(spy.called, 'warn was not called')
assert.match(res.headers.location, /google/)
})
})

it('should default to google if there is no auth strategy specified', () => {
process.env.OAUTH_STRATEGY = undefined
const appWithoutOauth = require('../../server/index') // need to redo app setup
return request(appWithoutOauth)
.get('/login')
.expect(302)
.then((res) => {
assert.match(res.headers.location, /google/)
})
})

it('should use slack strategy if slack is specified', () => {
process.env.OAUTH_STRATEGY = 'Slack'
process.env.SLACK_CLIENT_ID = '1234567890'
process.env.SLACK_CLIENT_SECRET = '1234567890'
const appWithSlackAuth = require('../../server/index') // need to redo app setup
return request(appWithSlackAuth)
.get('/login')
.expect(302)
.then((res) => {
assert.match(res.headers.location, /slack/)
})
})

it('Slack has to be capitalized, sorry', () => {
process.env.OAUTH_STRATEGY = 'slack'
const appWithSlackAuth = require('../../server/index') // need to redo app setup
return request(appWithSlackAuth)
.get('/login')
.expect(302)
.then((res) => {
assert.match(res.headers.location, /google/)
})
})
})

describe('when not logged in', () => {
beforeAll(() => sinon.stub(express.request, 'isAuthenticated').returns(false))
afterAll(() => sinon.restore())
Expand Down Expand Up @@ -63,7 +122,7 @@ describe('Authentication', () => {

describe('when logging in with regex-approved domain', () => {
beforeAll(() => {
sinon.stub(app.request, 'session').value({ passport: { user: regexUser } })
sinon.stub(app.request, 'session').value({passport: {user: regexUser}})
sinon.stub(express.request, 'user').value(regexUser)
sinon.stub(express.request, 'userInfo').value(regexUser)
})
Expand All @@ -78,7 +137,7 @@ describe('Authentication', () => {

describe('when logging in with specified email address', () => {
beforeAll(() => {
sinon.stub(app.request, 'session').value({ passport: { user: specificUser } })
sinon.stub(app.request, 'session').value({passport: {user: specificUser}})
sinon.stub(express.request, 'user').value(specificUser)
sinon.stub(express.request, 'userInfo').value(specificUser)
})
Expand Down