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

Non compliant param name update/fix for LinkedIn integration. #388

Closed
wants to merge 8 commits into from
Closed

Non compliant param name update/fix for LinkedIn integration. #388

wants to merge 8 commits into from

Conversation

jtroussard
Copy link
Contributor

Work around to remove duplicate access token value from url in the request() method.

Would be happy to collaborate on a more informed fix.

For more details please see issue:
access_token field added to request url when oauth2_access_token already present #377

Work around to remove duplicate access token value from url in the request() method.
@coveralls
Copy link

coveralls commented Sep 24, 2019

Coverage Status

Coverage decreased (-0.2%) to 90.335% when pulling 2ad25f7 on jtroussard:master into 36f24b4 on requests:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.5%) to 90.0% when pulling 62c8e76 on jtroussard:master into 36f24b4 on requests:master.

@JonathanHuot
Copy link
Contributor

Is it related to linkedin only ? also, isn't better to not add a duplicate instead of removing once it has been added ?
Cheers

@jtroussard
Copy link
Contributor Author

@JonathanHuot I have not attempted to establish authentication with any resource provider besides LinkedIn. I was hoping someone more familiar with this library would be able to provide a little more insight as to how the module determines how/when/where to add the access token string to the request URL. Any ideas/advice as how to go about finding this out?

As for the present code, I was just thinking about that this morning. Your suggestion is more direct. I'll make the changes now. Thanks!

More direct way of handling duplicate access token being added to request. Duplicate access token detection triggers log print.
@JonathanHuot
Copy link
Contributor

Also, if it is strictely related to Linkedin integration, I think it deserves to be located in https://github.com/requests/requests-oauthlib/blob/master/requests_oauthlib/compliance_fixes/linkedin.py

Is it possible ?

Thanks

@jtroussard
Copy link
Contributor Author

@JonathanHuot The access token appending was in front of me this whole time. It looks like the "oauth_access_token" value is being added to the URL as part of the LinkedIn compliance hook routine. If that is the case I would purpose that the compliance hook for loop be moved after the "access_token" value is added to the URL and then have the def _non_compliant_param_name() method handle the field name vernacular. I'll make the necessary changes now.

Moved compliance hook invocation until after the access token value is added to the request URL.
compliance update access_token -> oauth_access_token
@jtroussard jtroussard changed the title Update oauth2_session.py Non compliant param name update/fix for LinkedIn integration. Sep 25, 2019
@jtroussard
Copy link
Contributor Author

Any advice on why my commits are failing the file Travic CI check for Black? Is that a formatting thing? If it is I've run both modules through a Black formatting.

@andr1an
Copy link

andr1an commented Oct 1, 2019

@jtroussard Yes, you can call black requests_oauthlib/compliance_fixes/linkedin.py requests_oauthlib/oauth2_session.py to fix this.

@jtroussard
Copy link
Contributor Author

This has been moved to #397

@jtroussard jtroussard closed this Feb 17, 2020
@jtroussard
Copy link
Contributor Author

This has been moved to #397

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

4 participants