Skip to content

Support scopes defined both in strategy constructor and authenticate()#99

Closed
anabellaspinelli wants to merge 1 commit into
jaredhanson:masterfrom
anabellaspinelli:add-scopes-instead-of-override
Closed

Support scopes defined both in strategy constructor and authenticate()#99
anabellaspinelli wants to merge 1 commit into
jaredhanson:masterfrom
anabellaspinelli:add-scopes-instead-of-override

Conversation

@anabellaspinelli

Copy link
Copy Markdown

I'm adding this feature for cases in which strategy users want to have default scope set up in their Strategy constructor (maybe something like profile) and then customize other scopes based on user selection.

Right now, the scope that comes in the authenticate() options object overrides whatever was set up in the constructor.

Tested it in the following scenarios:

  • Scopes defined both in strategy constructor and authenticate options obj
  • Scopes defined only in strategy constructor
  • Scopes defined only in authenticate options obj
  • No scopes are defined

@coveralls

coveralls commented May 3, 2018

Copy link
Copy Markdown

Coverage Status

Coverage remained the same at 100.0% when pulling b126181 on anabellaspinelli:add-scopes-instead-of-override into a948096 on jaredhanson:master.

@anabellaspinelli

Copy link
Copy Markdown
Author

@jaredhanson @tomhughes @Tug @natalan

Hello!

This change (or something like it) would be really helpful for me and (Typeform), the company I work for.

I noticed there are a lot of pending PRs. Is this the correct way to request a review?

Thank you!

@rwky

rwky commented Jul 7, 2018

Copy link
Copy Markdown

@anabellaspinelli if you make a PR against https://github.com/passport-next/passport-oauth2 is will get looked into.

@anabellaspinelli

Copy link
Copy Markdown
Author

Thank you @rwky I will!

Just out of curiosity, what's the purpose of the fork?

@rwky

rwky commented Jul 7, 2018

Copy link
Copy Markdown

One of the companies I work for uses several passport modules and maintained our own forks, since the upstream modules aren't maintained I decided to fork them publicly in an organization so even if I get hit by a bus they won't go unmaintained.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants