-
Notifications
You must be signed in to change notification settings - Fork 144
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
Conversation
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.
This is a great addition, thanks @penina-nyt!
There were two very minor questions I noticed, but not blockers to merge. Happy to discuss more via chat or in the PR!
README.md
Outdated
# Slack needs to be capitalized | ||
OAUTH_STRATEGY=Slack |
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.
Great to note this in the docs, would be interested in hearing more about capitalization in this case vs the default (google
).
Co-authored-by: Isaac White <isaacwhite@users.noreply.github.com>
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.
Looks great, @penina-nyt! Thanks for the contribution.
Excited to have this as part of the app! This definitely makes me wonder what the future of auth in Library looks like, and how "plugins" or customizations could play a part.
passReqToCallback: true | ||
}, (request, accessToken, refreshToken, profile, done) => done(null, profile))) | ||
const authStrategies = ['google', 'Slack'] | ||
let authStrategy = process.env.OAUTH_STRATEGY |
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
}) | ||
|
||
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 comment
The reason will be displayed to describe this comment to others. Learn more.
😆
id: '13', | ||
userId: '13' | ||
} | ||
|
||
describe('Authentication', () => { | ||
describe('.env specified oauth strategy', () => { |
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.
Thanks for adding these solid tests!
Description of Change
added support for slack authentication. interested parties can set up a slack app on their workspace as documented in the readme, and set their credentials as environment variables in their deploy pipeline and the
.env
file respectivelyRelated Issue
#81
Motivation and Context
Checklist
npm run lint
and updated code style accordinglynpm run test
passes