From 6f74845a82306cd73ef1d6459226c99c200b4d85 Mon Sep 17 00:00:00 2001 From: Penina Kessler <64811387+penina-nyt@users.noreply.github.com> Date: Thu, 25 Feb 2021 11:03:01 -0500 Subject: [PATCH] [Feature] slack oauth support (#245) --- CONTRIBUTING.md | 14 +++++++ README.md | 15 +++++++ custom/README.md | 15 +++++++ package-lock.json | 16 +++++++ package.json | 1 + server/userAuth.js | 57 ++++++++++++++++++------- test/functional/userAuth.test.js | 71 +++++++++++++++++++++++++++++--- 7 files changed, 169 insertions(+), 20 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 0eb20e44..453ff015 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -1,3 +1,17 @@ + + +**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) + + + # Contributing to Library ## Repo Layout diff --git a/README.md b/README.md index c5956b8e..305ad145 100644 --- a/README.md +++ b/README.md @@ -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) @@ -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 usually intended to be internal to a set of limited users, Oauth with your organization is strongly encouraged. To use Slack Oauth, specify your Oauth strategy in your `.env` file, like so: +``` +# Slack needs to be capitalized as per the Passport.js slack oauth docs http://www.passportjs.org/packages/passport-slack-oauth2/ +OAUTH_STRATEGY=Slack +``` +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. diff --git a/custom/README.md b/custom/README.md index 95f4a8e1..b51e3c92 100644 --- a/custom/README.md +++ b/custom/README.md @@ -1,3 +1,18 @@ + + +**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) + + + # Customizing Library ## The `custom` Directory diff --git a/package-lock.json b/package-lock.json index df526a56..b3e318c3 100644 --- a/package-lock.json +++ b/package-lock.json @@ -9999,6 +9999,16 @@ "utils-merge": "1.x.x" } }, + "passport-slack-oauth2": { + "version": "1.0.2", + "resolved": "https://registry.npmjs.org/passport-slack-oauth2/-/passport-slack-oauth2-1.0.2.tgz", + "integrity": "sha512-/+SNK754aMTxGbbBtFcAQQRSuwZ+McXnJbB7qIPUFJx870YANZfVQX31gcWfMEmm6ObcFYYp1n6jbU5/XajrAQ==", + "dev": true, + "requires": { + "passport-oauth2": "^1.4.0", + "pkginfo": "^0.4.1" + } + }, "passport-strategy": { "version": "1.0.0", "resolved": "https://registry.npmjs.org/passport-strategy/-/passport-strategy-1.0.0.tgz", @@ -10112,6 +10122,12 @@ } } }, + "pkginfo": { + "version": "0.4.1", + "resolved": "https://registry.npmjs.org/pkginfo/-/pkginfo-0.4.1.tgz", + "integrity": "sha1-tUGO8EOd5UJfxJlQQtztFPsqhP8=", + "dev": true + }, "posix-character-classes": { "version": "0.1.1", "resolved": "https://registry.npmjs.org/posix-character-classes/-/posix-character-classes-0.1.1.tgz", diff --git a/package.json b/package.json index 8d6dd2f3..692dd741 100644 --- a/package.json +++ b/package.json @@ -44,6 +44,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" diff --git a/server/userAuth.js b/server/userAuth.js index cd56c0cc..cacd5a91 100644 --- a/server/userAuth.js +++ b/server/userAuth.js @@ -4,6 +4,7 @@ const passport = require('passport') const session = require('express-session') const crypto = require('crypto') const GoogleStrategy = require('passport-google-oauth20') +const SlackStrategy = require('passport-slack-oauth2').Strategy const log = require('./logger') const {stringTemplate: template} = require('./utils') @@ -11,15 +12,41 @@ const {stringTemplate: template} = require('./utils') const router = require('express-promise-router')() const domains = new Set(process.env.APPROVED_DOMAINS.split(/,\s?/g)) -const md5 = (data) => crypto.createHash('md5').update(data).digest('hex') +const authStrategies = ['google', 'Slack'] +let authStrategy = process.env.OAUTH_STRATEGY + +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' +} -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 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))) +} + +const md5 = (data) => crypto.createHash('md5').update(data).digest('hex') router.use(session({ secret: process.env.SESSION_SECRET, @@ -35,27 +62,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() @@ -91,10 +119,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 } } diff --git a/test/functional/userAuth.test.js b/test/functional/userAuth.test.js index 9ff8dd0b..15b52295 100644 --- a/test/functional/userAuth.test.js +++ b/test/functional/userAuth.test.js @@ -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') /* @@ -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', () => { + 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' + 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()) @@ -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) }) @@ -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) })