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

Update strategy to work with new "Sign in with Linkedin" feature #63

Closed
wants to merge 15 commits into from

Conversation

SokratisVidros
Copy link

Following recent updates regarding new "Sign in with Linkedin" feature, I've updated the corresponding passport-linkedin-oauth2 library.

For more information please refer to the official documentation

@YuriScarbaci
Copy link

This only allows "r_liteprofile" scope when authenticating, in the official documentation you linked you can see that you can still ask the r_basicprofile permission when authenticating. Also I think that the repository is unmanaged, you may wish to Fork it instead of doing pull request

@SokratisVidros
Copy link
Author

Hi @YuriScarbaci,

If I am not mistaken r_liteprofile will be the only available scope for the "Sign in with Linkedin" feature.
The other scopes are still available but they can't be given to Linkedin OAuth apps in a self-service way. As far as I know, you have to talk to Linkedin support and enable them. I will double check with Linkedin, perform couple of tests and come back to you.

From docs

New members logging in to your service for the first time will need to follow the Authenticating with OAuth 2.0 Guide. When requesting an authorization code in Step 2 of the OAuth 2.0 Guide, make sure to request the r_liteprofile and/or r_emailaddress scopes!

Lastly, please note that this PR comes from a forked repo.

@YuriScarbaci
Copy link

YuriScarbaci commented Dec 28, 2018

@SokratisVidros

in the same docs you shared you have another link pointing to the 0Auth2 flow , if you further crawl such page by clicking on the link

Follow one of the two authorization flows in Permissions to get started.

Permissions and from there you go to Member Authorization

you will find yourself in a page stating suchs:

the member either accepts or denies your application's permission request

Note that if you ever change the scope permissions that your application requires, your application's users must re-authenticate to ensure that they have explicitly granted your application all of the permissions that it requests on their behalf.

which in itself implies multiples scope are available, furthermore if you look the "Authorization" endpoint you can see the following stated in the scope parameter:

A URL-encoded, space-delimited list of member permissions your application is requesting on behalf of the user. If you do not specify a scope in your call, the default application permissions you defined in your application configuration are used. For example, scope=r_fullprofile%20r_emailaddress%20w_share. See Permissions and Best Practices for Application Development for additional information.

in the given example of the parameter description you can see r_fullprofile && r_emailaddress && w_share, ofcourse r_basicprofile should be included.

Furthermore, the quote you sent me states:

When requesting an authorization code in Step 2 of the OAuth 2.0 Guide, make sure to request the r_liteprofile and/or r_emailaddress scopes!

but r_liteprofile in this case is just "the minimum scope" to achieve the "minimum" fields, r_basicprofile includes r_liteprofile and doesn't requires any partnerships with linkedin (the only two "partnership-only" fields are refreshToken and r_fullprofile, based on linkedin own documentation).

I'm currently investigating this because I need to migrate from 0Auth1 to 0Auth2 and still be able to take some fields not included in r_liteprofile

By any means if you find something else that I missed out that states that sign-in with linkedin won't allow r_basicprofile scope I would love if you could address it to me.

@SokratisVidros
Copy link
Author

@YuriScarbaci that was my initial understanding too. Then we had a call with Linkedin and we were informed that most of the scopes will be deprecated.

Afterwards, an email from the Linkedin dev platform followed stating:

Sign In with LinkedIn: Sign In with LinkedIn enables members to choose a more convenient way to log-in to third party apps and allows those apps to learn more about their new user. This API will only recognize a new “Lite Profile” permission, which supports a reduced set of member profile fields.

Lastly, please have a look at:
https://docs.microsoft.com/en-us/linkedin/consumer/integrations/self-serve/migration-faq?context=linkedin/consumer/context?trk=eml_mktg_gco_dev_api_comms

Moving forward, the available v2 APIs include:
r_liteprofile (replaces r_basicprofile)
r_emailaddress
w_member_social (replaces w_share)

@YuriScarbaci
Copy link

I totally missed out that document, tought the migration was between 0Auth1 and 0Auth2, with 0Auth2 having 2 possible versions, both valid (v1 and v2)...

I see, so from now on "sign in with linkedin" can only be used to get those lite profile fields...

Since this repository is long abandoned it would be (in my opinion) a good idea to create a new package based on your last fork, to add to the npm registry, to allow installation of your code via npm install and be able to further receive assistance in mantaining it...

If you choose to release an official npm package with your PR I will without doubt be one of your user/contribuitor.

Thanks a lot for your effort and time!

@YuriScarbaci
Copy link

In particular I think the "refreshToken" mechanics needs to be overhauled since passport actually expect a refreshToken but linkedIn only emits refresh token for special partners, that's why having an official npm package would be neat...

@SokratisVidros
Copy link
Author

SokratisVidros commented Dec 28, 2018

@YuriScarbaci My plan is to release an NPM package next week unless Auth0 is willing to merge the changes in this one.

@YuriScarbaci
Copy link

Perfect,
If you do release such a package I would love if you could mention me so I get a notification (or am I missing any other way to get a notification? I'm already "watching" your PR repo btw)

Once again thanks for your effort and time!

@YuriScarbaci
Copy link

Last question, do you plan to support requesting the additional w_member_social scope to allow the application to share as if the authenticated user itself shared the content?

@SokratisVidros
Copy link
Author

Hmmm, I will try to add that too.

@@ -7,141 +7,109 @@ function Strategy(options, verify) {
options = options || {};
Copy link

Choose a reason for hiding this comment

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

should we move it in arguments as options={}, verify=false?

return memo;
}, []);
}

Copy link

Choose a reason for hiding this comment

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

var profilePicture = _.defaults(json.profilePicture, {'displayImage~': { elements:[]}); 

if (profilePicture.elements.length> 0){
profilePicture.elements.reduce(...)
}

@YuriScarbaci
Copy link

Any news on this?

@allenwei
Copy link

looks like Linkedin is trying to force us upgrade to v2 api by raise error randomly. Our user random got "failed to fetch user profile" error. like this one #65

@CatalinaMoisuc
Copy link

CatalinaMoisuc commented Jan 11, 2019

@SokratisVidros did you manage to release that NPM package? under which license will that be, MIT?
Or will this PR be merged?

@SokratisVidros
Copy link
Author

I will try to have it ready by tomorrow. I apologise for the delay, but the past week was hectic.

@CatalinaMoisuc
Copy link

@SokratisVidros that would be awesome! Thank you.

@YuriScarbaci
Copy link

@SokratisVidros Thank you a lot :)

@SokratisVidros
Copy link
Author

https://www.npmjs.com/package/@sokratis/passport-linkedin-oauth2

sebadoom added a commit to sebadoom/passport-linkedin-oauth2 that referenced this pull request Jan 29, 2019
Squashed commit of the following:

commit 397c0c0
Author: Sokratis Vidros <sokratis.vidros@gmail.com>
Date:   Wed Jan 23 01:03:46 2019 +0200

    2.0.2

commit f52d878
Author: Sokratis Vidros <sokratis.vidros@gmail.com>
Date:   Wed Jan 23 01:03:35 2019 +0200

    Fix typo in README.md

commit 4edb71f
Author: Sokratis Vidros <sokratis.vidros@gmail.com>
Date:   Wed Jan 23 01:03:10 2019 +0200

    Align latest profile.photos structure with previous versions

commit e0ff695
Author: Sokratis Vidros <sokratis.vidros@gmail.com>
Date:   Wed Jan 16 12:11:30 2019 +0200

    2.0.1

commit 3e6e6fb
Author: Sokratis Vidros <sokratis.vidros@gmail.com>
Date:   Wed Jan 16 12:11:22 2019 +0200

    Update README.md

commit ee6c9eb
Author: Sokratis Vidros <sokratis.vidros@gmail.com>
Date:   Wed Jan 16 12:10:50 2019 +0200

    Fix typo regarding scopes in unit tests

commit 21c7afd
Author: Sokratis Vidros <sokratis.vidros@gmail.com>
Date:   Thu Dec 27 13:39:28 2018 +0200

    2.0.0

commit 9beb887
Author: Sokratis Vidros <sokratis.vidros@gmail.com>
Date:   Wed Jan 16 11:25:19 2019 +0200

    Update package name and description

commit b65d03c
Author: Sokratis Vidros <sokratis.vidros@gmail.com>
Date:   Thu Dec 27 13:31:52 2018 +0200

    Update README.md

commit e4c2aee
Author: Sokratis Vidros <sokratis.vidros@gmail.com>
Date:   Thu Dec 27 13:29:25 2018 +0200

    Bump dev dependencies and update package.json contributors

commit f2853dd
Author: Sokratis Vidros <sokratis.vidros@gmail.com>
Date:   Thu Dec 27 13:27:26 2018 +0200

    Update library to work with new Sign in with Linkedin

    More info at https://docs.microsoft.com/en-us/linkedin/consumer/integrations/self-serve/sign-in-with-linkedin?context=linkedin/consumer/context?trk=eml_mktg_gco_dev_api_comms

commit 190bcd5
Author: Sokratis Vidros <sokratis.vidros@gmail.com>
Date:   Thu Dec 27 11:40:25 2018 +0200

    Teach example server to read Linkedin client credentials from env
@satyavh
Copy link

satyavh commented Feb 15, 2019

@klouvas when are you planning to merge and release this? The LinkedIn deadline is approaching...

@SokratisVidros
Copy link
Author

@satyavh The package is already published separately https://www.npmjs.com/package/@sokratis/passport-linkedin-oauth2

}

profile._profileRaw = body;
profile._profileJson = json;

Choose a reason for hiding this comment

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

was wondering: why this breaking change?

Copy link
Author

Choose a reason for hiding this comment

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

In order to conform with the Linkedin OAuth API, two separate requests need to be executed in order to retrieve the user profile and then the email.

Having code consistency in mind as well as taking into account the JS convention that says that keys starting with _ are considered private, I kept both API raw and JSON responses under the following structure:

profile._profileRaw = body;
profile._profileJson = json;
profile._emailRaw = body;
profile._emailJson = json;

Hope that helps.

Choose a reason for hiding this comment

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

but passport already saved those under profile._raw and profile._json

As I'm using profile._json to get more data I had a bug when switching to your fork. That's how and why I'm calling it breaking. But it's fine as long as it is documented :)

stavros-wb and others added 3 commits April 24, 2019 18:06
When a client has requested the `r_emailaddress` scope from LinkedIn, then
we do one more request. Instead of firing the requests sequentially we should
make both of them at the same time and merge them upon their responses.

These requests take aprox. 1 minute when run sequentially, so half a
minute when they're run in parallel
@javidjamae
Copy link

javidjamae commented May 16, 2019

@SokratisVidros Thanks for doing this! We're upgrading to your library now. Do you plan on continuing to maintain it over time?

@YuriScarbaci You said:

Since this repository is long abandoned it would be (in my opinion) a good idea to create a new package based on your last fork, to add to the npm registry...

If this repository is abandoned, it is not clear in the README at all. Out of curiosity, how do you know it's abandoned? It doesn't look like you're a committer / contributor.

If it is truly abandoned, who would be the right person to update the README to indicate what the official recommendation is for people who are currently relying on the repository?

If it is not, then are there any plans to merge in this PR at some point?

@jfromaniello @siacomuzzi ??

Thanks!

@SokratisVidros
Copy link
Author

@javidjamae yes

@vbhartia
Copy link

I don't think this issue has been fixed yet?

Using the main package - passport-linkedin-oauth2 , I get an error - "failed to fetch user profile", referenced above.

Using this package - https://www.npmjs.com/package/@sokratis/passport-linkedin-oauth2 , with the fix, it seems to work?

@YuriScarbaci
Copy link

@javidjamae

@YuriScarbaci You said:

Since this repository is long abandoned it would be (in my opinion) a good idea to create a new package based on your last fork, to add to the npm registry...

If this repository is abandoned, it is not clear in the README at all. Out of curiosity, how do you know it's abandoned? It doesn't look like you're a committer / contributor.

Just an educated guess due to the library not being updated in the last 7 month even when major changes in the consumed linkedin api were about to break the whole library itself...

@siacomuzzi
Copy link
Member

First of all, apologies for the delay in answering.

  • Correct me if I'm wrong but the master branch should supports the latest LinkedIn API changes.
  • The only pending here is to make a new release and publish it to npm, right? cc @sebadoom

@MathRobin
Copy link

Any news ?

@RusinovAnton
Copy link

@siacomuzzi
seems like no :(
I've just installed the package from the master branch (e311f79) instead of npm and the problem persists - getting the same error for the "r_basicprofile" scope.

@siacomuzzi
Copy link
Member

@RusinovAnton I just tried with the example app provided in the repository and everything works as expected.

Please make sure you have added the following products in your Linkedin app:

image

@siacomuzzi
Copy link
Member

@MathRobin, latest version (passport-linkedin-oauth2@2.0.0) is available in npm

@YuriScarbaci
Copy link

latest version (passport-linkedin-oauth2@2.0.0) is available in npm

I did switch my production succesfully to this version and everything works as expected

the only thing that would be nice to clarify at this point is if we should use @SokratisVidros version or @siacomuzzi (both are working) based on if @siacomuzzi plans on keeping mantaining this package... or maybe merge both packages and make sokratis a project mantainer too? it's getting confusing to keep track of this module and most tutorial on internet points to this npm package too...

@RusinovAnton
Copy link

RusinovAnton commented Oct 4, 2019

@siacomuzzi

Please make sure you have added the following products in your Linkedin app

I do have those added, moreover, I have my acc connected to a company page. I just missed that set of permissions do not allow me to get "r_basicprofile", so thats error on my side.

image

Requests for available permissions works just fine.

@SokratisVidros
Copy link
Author

I'd be happy to merge both implementations into the original library.

@siacomuzzi
Copy link
Member

the only thing that would be nice to clarify at this point is if we should use @SokratisVidros version or @siacomuzzi (both are working) based on if @siacomuzzi plans on keeping mantaining this package... or maybe merge both packages and make sokratis a project mantainer too? it's getting confusing to keep track of this module and most tutorial on internet points to this npm package too...

@sebadoom @glena could you please answer these questions? I'm not the maintainer anymore.

@sebadoom
Copy link
Contributor

sebadoom commented Oct 23, 2019

latest version (passport-linkedin-oauth2@2.0.0) is available in npm

I did switch my production succesfully to this version and everything works as expected

the only thing that would be nice to clarify at this point is if we should use @SokratisVidros version or @siacomuzzi (both are working) based on if @siacomuzzi plans on keeping mantaining this package... or maybe merge both packages and make sokratis a project mantainer too? it's getting confusing to keep track of this module and most tutorial on internet points to this npm package too...

There are no plans to merge any forks into this repository. If the fork provides any functionality that is not provided by this repository, users are encouraged to use those forks, or, alternatively, provide a PR with the missing functionality with passing tests and based on master.

Closing, LinkedIn API v2 supported by latest release.

@sebadoom sebadoom closed this Oct 23, 2019
@javidjamae
Copy link

@sebadoom I appreciate that you guys updated this package to support LinkedIn API v2.

Seeing that you guys were only able to make this update many months after the v1 was no longer supported suggests that you don't have much time to do timely updates to this library.

Can you at least consider making @SokratisVidros a committer as it seems that he's interested and has the time / knowledge to make timely updates?

I think it would be greatly appreciated by the many consumers of this library.

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.

None yet