-
Notifications
You must be signed in to change notification settings - Fork 145
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
Changes from all commits
ff1a9d0
e0daebf
2d75894
bb4ba28
8ff9e20
f33059e
ee8b225
0d23042
c0db9ea
8d3dded
d614a44
f9fcb09
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
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') | ||
|
||
/* | ||
|
@@ -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', () => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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' | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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()) | ||
|
@@ -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) | ||
}) | ||
|
There was a problem hiding this comment.
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.There was a problem hiding this comment.
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