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

Remove r_contactinfo. #307

Closed
wants to merge 1 commit into from
Closed

Remove r_contactinfo. #307

wants to merge 1 commit into from

Conversation

nexon
Copy link

@nexon nexon commented May 25, 2015

This is a fix because, it seems that linkedin now removes the r_contactinfo scope, so if the user add this scope, linked in says 'Your application has not been authorized for the scope "r_contactinfo"'

@ramsey
Copy link
Contributor

ramsey commented May 25, 2015

This breaks the build, so please change the test, so that this passes.

I'll merge this into the master branch once the test has been fixed, but the 1.0 branch no longer contains the LinkedIn provider. You might want to take a look at the LinkedIn Provider project to see if this same change needs to occur there.

Thanks!

@ramsey
Copy link
Contributor

ramsey commented May 25, 2015

Additionally, I can't find anywhere in the LinkedIn documentation where they say that they have removed support for the r_contactinfo scope. Can you please provide a link for reference?

In particular, this page says you must request the r_contactinfo scope in order to get contact information: https://developer.linkedin.com/docs/fields/contact

Maybe the application you're using hasn't been set up to use this scope?

@nexon
Copy link
Author

nexon commented May 25, 2015

Sure, the problem is that now you need to apply to a "Application with LinkedIn". By default isn't allow use it unless you apply for it.

from https://developer.linkedin.com/support/developer-program-transition#troubleshooting

The r_contactinfo member permission will now be associated exclusively with Apply with LinkedIn. As a result, you will only be able to request this member permission if your application has been approved by LinkedIn for this particular use.

so basically i think this r_contactinfo should be remove by default in the LinkedIn Provider.

@ramsey
Copy link
Contributor

ramsey commented May 25, 2015

I'm a little worried about how this "bug fix" might affect applications that are already using this. If I do a patch-level version increment (i.e. 0.11.1) and people are using a composer.json with a ~ or * for this package and they have already been approved by LinkedIn for this use, then when they update with composer, their applications will break.

It seems we have two choices:

  1. Merge this in and tag it as 0.12.0, but even that could cause problems for folks

  2. Since $scopes is a public property of the LinkedIn class, you may change the value after instantiating the provider, or you may pass in "scopes" as value in the options array when you instantiate the LinkedIn provider:

    $linkedin = new Provider\LinkedIn(['scopes' => 'r_basicprofile r_emailaddress']);
    

I'm inclined to go with option 2, since that's most likely how others implementing this are currently handling things, and it won't break those implementations that rely on r_contactinfo.

Version 1.0 of this library will be coming out soon, and this will no longer be a problem.

@stevenmaguire, since you maintain the oauth2-linkedin provider, would you like to weigh-in on this?

@stevenmaguire
Copy link
Member

Thanks for heads up @ramsey I will take a closer look and chime in.

@stevenmaguire
Copy link
Member

I share your concerns @ramsey. The current provider, within the 0.x version of this package and the decoupled package, both include the default scopes, including the deprecated one. The action that makes the most sense to me is a little of column A and a little of column B.

I am a bit nervous about providing default scopes at all, given this change and the way LinkedIn is handling it; "you will begin to see critical errors occurring now".

While I am not 100% convinced this is the "right way" to go, I would propose removing the default scopes entirely from this package and the decoupled package, with a minor version bump and provide specific documentation on how to explicitly set the scopes from consuming applications in every use of the provider.

I think it should be the sole responsibility of the consuming application to explicitly include the scopes it requires, and enforcing any API policy changes that the service adopts.

Other thoughts?

@ramsey
Copy link
Contributor

ramsey commented May 26, 2015

That sounds good to me. @nexon, can you amend your pull request according to @stevenmaguire's recommendation? Also, be sure to correct the tests, so that they pass.

As for documentation, let's create a sub-heading for "LinkedIn" under the "Built-In Providers" section on the README and include a brief example of how to set the scopes. When I cut the minor version tag, I'll also include information in the release description, and I'll be sure to get a CHANGELOG doc into the repository, as well.

@stevenmaguire
Copy link
Member

Should @nexon take care of the README update in his amended PR or would you like me to submit a separate PR for that?

I will update the decoupled package to reflect these changes.

@stevenmaguire
Copy link
Member

This is a snippet of documentation that I've made available in the decoupled provider README. Perhaps it will help here:

Managing Scopes

When creating your LinkedIn provider, you can specify the scopes your application may authorize.

$provider = new League\OAuth2\Client\Provider\LinkedIn([
    'clientId'          => '{linkedin-client-id}',
    'clientSecret'      => '{linkedin-client-secret}',
    'redirectUri'       => 'https://example.com/callback-url',
    'scopes'            => ['r_basicprofile r_emailaddress'],
]);

It is important to note, each scope must be space delimited and contained within one string.

At the time of authoring this documentation, the following scopes are available.

  • r_basicprofile
  • r_emailaddress
  • rw_company_admin
  • w_share

@stevenmaguire
Copy link
Member

The LinkedIn Provider has been updated to remove the default scopes and the README has been updated to document the known scopes since this change. A new minor release has been cut as a result of these changes.

@ramsey
Copy link
Contributor

ramsey commented May 26, 2015

Thanks, @stevenmaguire!

@nexon, thoughts?

@ramsey
Copy link
Contributor

ramsey commented May 31, 2015

@nexon, Just checking in with you about this. Are you able to make the changes discussed?

@ramsey
Copy link
Contributor

ramsey commented Jun 10, 2015

@stevenmaguire, would you like to submit a new PR with the changes we discussed, and I'll do a minor version bump of the 0.x series?

Thanks!

@stevenmaguire
Copy link
Member

@ramsey sounds good to me. I will take care of it now.

@stevenmaguire
Copy link
Member

@ramsey #327 is in

@ramsey
Copy link
Contributor

ramsey commented Jun 11, 2015

Thanks, @stevenmaguire. Closing out this PR.

@ramsey ramsey closed this Jun 11, 2015
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