Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

[Github] Pass access token in a header. #428

Merged
merged 4 commits into from
Mar 15, 2020

Conversation

asherf
Copy link
Contributor

@asherf asherf commented Feb 5, 2020

@ThomasWaldmann
Copy link

Would be good to have a release with that, otherwise github will spam us all 3 days for every app we use with social-auth against github.

@asherf asherf requested a review from omab February 6, 2020 19:22
@asherf
Copy link
Contributor Author

asherf commented Feb 6, 2020

Fixes: #430

Copy link

@PPingot PPingot left a comment

Choose a reason for hiding this comment

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

Good work.
Perhaps I would compact this into single line.
Using single quotation marks would be in align with rest of the file.
Thanks for this change.

@asherf
Copy link
Contributor Author

asherf commented Feb 10, 2020

@omab Any chance you get a few minutes to review & merge this change ?

@asherf
Copy link
Contributor Author

asherf commented Feb 11, 2020

I reached out to @omab over email. Hopefully this will get things going wrt to this PR.

@rafaelcapucho
Copy link

Hi there, Is there any chance to get it merged soon? Where I work the Infra department is very strict, they are pushing for this update in a daily basis @asherf @omab

Thank you.

Copy link

@rafaelcapucho rafaelcapucho left a comment

Choose a reason for hiding this comment

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

Hi @asherf ,
GithubMemberOAuth2.user_data (Line 81) also has a self.request with access_token as param, I think we should change it also to the new header style, no? Thank you

@onlyphantom
Copy link

Would also be nice once the merge has been completed, to provide a small update on what existing apps should do beyond a dependency upgrade, like a one-paragraph summary that says "place your token in the config file and then change this line of code here".

@vsoch
Copy link

vsoch commented Feb 18, 2020

I’m also worried about updating a production app with a version of social core from a couple of years ago, so I’d want to discuss how/if to go about that.

@rafaelcapucho
Copy link

I’m also worried about updating a production app with a version of social core from a couple of years ago, so I’d want to discuss how/if to go about that.

Here we forked the repository, inside our organization, in the same version that we're currently using, and I did the required changes (similar to this PR changes) myself, and I'm adding a .zip file address provided by Github to the requirements.txt file.

@vsoch
Copy link

vsoch commented Feb 18, 2020

That seems like a good idea @rafaelcapucho - the changes are minimal and if it's only the header (and the functions are the same) it should be relatively easy to map. I think I'll wait until this is finished / done (merged) before doing this for my application.

@asherf
Copy link
Contributor Author

asherf commented Feb 29, 2020

Tried reaching out via email and twitter...
I hope we can make progress on this...

@vsoch
Copy link

vsoch commented Feb 29, 2020

Huge +1 to what @asherf has contributed and said above. The deadline is approaching, and a lot of us are going to be forced to make change to our servers soon. It would be great to have a reviewed, and merged fix for this.

@asherf
Copy link
Contributor Author

asherf commented Feb 29, 2020

Link to twitter in case other people want to try and reach out.

https://twitter.com/linuxaddict

@vsoch
Copy link

vsoch commented Mar 3, 2020

This has been causing me quite some anxiety for a while, and (I have an older version of social auth) but I was able to make a custom backend with the needed changes that appears to be working. Here is the minimum example:

from social_core.backends.github import GithubOAuth2
from six.moves.urllib.parse import urljoin

# Update headers to use token
# https://github.com/python-social-auth/social-core/pull/428/files
class ShubGithubOAuth2(GithubOAuth2):

    def _user_data(self, access_token, path=None):
        url = urljoin(self.api_url(), 'user{0}'.format(path or ''))
        return self.get_json(url, headers={'Authorization': 'token {0}'.format(access_token)})

And I also did this for my private version of the login (basically the same but with different permissions, not shown). Then in the settings.py (or however you store that) instead of importing the module from social_core, I import it from my file:

# Python-social-auth

AUTHENTICATION_BACKENDS = (
    'django.contrib.auth.backends.ModelBackend',
#    'social_core.backends.github.GithubOAuth2',
    'shub.apps.users.views.auth.ShubGithubOAuth2',
    'shub.apps.users.views.auth.GithubPrivateOAuth2',
    'guardian.backends.ObjectPermissionBackend',
)

I suspect if you use the GithubMemberOAuth2 you would need to do the same, but for that class. I also did a careful check to compare the file for the version I have installed against the one here to ensure that nothing else was too different. There were some added class variables that I didn't see, but I suspect they are not represented elsewhere in the older code so it wouldn't be an issue.

And so far so good! The ultimate test will be if/when the date passes and to see if the thing stops working :P

@atodorov
Copy link
Contributor

atodorov commented Mar 7, 2020

Link to twitter in case other people want to try and reach out.
https://twitter.com/linuxaddict

Pinging OSS authors on Twitter, especially excessively is not the right way to go. That gets very frustrating very fast, regardless of the reasons authors don't reply. I am an author myself so I know (pylint-django among others).

OTOH @omab should take care to secure the future of the project by finding a few more co-maintainers. Clearly this project needs it. In the past I had reached out with proposal to help but didn't get a reply.

FTR I still volunteer to help b/c one of my projects is heavily dependent on social-core and social-django. I don't like to fork or carry around modules and classes copied from upstream. I will happily review pull requests, make sure they look good and have tests and build new versions for PyPI.

@asherf
Copy link
Contributor Author

asherf commented Mar 7, 2020

Link to twitter in case other people want to try and reach out.
https://twitter.com/linuxaddict

Pinging OSS authors on Twitter, especially excessively is not the right way to go. That gets very frustrating very fast, regardless of the reasons authors don't reply. I am an author myself so I know (pylint-django among others).

OTOH @omab should take care to secure the future of the project by finding a few more co-maintainers. Clearly this project needs it. In the past I had reached out with proposal to help but didn't get a reply.

FTR I still volunteer to help b/c one of my projects is heavily dependent on social-core and social-django. I don't like to fork or carry around modules and classes copied from upstream. I will happily review pull requests, make sure they look good and have tests and build new versions for PyPI.

Obviously I didn't intend for people to bombard him with messages.... but tbh it is somewhat frustrating that I can't get a reply for him via email (sent ~3 emails over the past month or so),
I only pinged him on twitter (dm) since I saw he was somewhat active there...
but still no answer.
in my emails, I volunteered to join this GH org and help maintain this project...
still no response...

so..... I would love it if you have any suggestion on how we can continue from here. since it seems this project as a lot of people active on it (creating PRs & issues) but there seem to be a single maintainer that doesn't seem to respond...

@vsoch
Copy link

vsoch commented Mar 7, 2020

I think if he is busy, or chooses not to respond, that's probably his right - being an open source maintainer doesn't mean that you owe the community anything. We can't demand a response based on a timeline set by a third party. We would hope that there would be support, but we also don't know the kind of workload or incentives that he currently has. So what to do? We each have to optimize for our own needs. That could mean any, some subset, or all of the following:

  • creating a custom backend that updates the functions accordingly
  • having good faith that the review will come eventually, possibly not in time for the deadline, but not yet deciding to choose another library
  • creating a proposal to fork the project and create a community around it (this is a huge burden to take on), probably I wouldn't do this so quickly, but maybe after a year.

I know it's frustrating, but I think we need to have compassion. We don't know the kind of things that @omab is dealing with, and I think as an open source maintainer for a very active and successful project, we should give him space. There is likely a reason he can't do this now. The fixes aren't so crazy that it's not possible to patch for the time being. When he is ready and engages, we can have discussion about how we can better help him in the future.

@atodorov
Copy link
Contributor

atodorov commented Mar 7, 2020

Both @asherf and @vsoch opinions have merit but we're hijacking the topic here. I have opened
#445 to collect proposals and opinions.

@omab omab merged commit bc79fd4 into python-social-auth:master Mar 15, 2020
@omab
Copy link
Contributor

omab commented Mar 15, 2020

Thanks for the change, as for the maintenance topic, I've posted a reply on the dedicated issue #445.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

8 participants