Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Clean up the 'magic' - Let Developer decide what to do. #90

Closed
g00fy- opened this Issue · 34 comments
@g00fy-

Current implementation tries to impose to many things on developer, and does to much "magic". Social-Auth assumes:

  1. social-auth should create a user account, and fetch some data from provider X to user model.
  2. social-auth should not create a user. The decision is based on SOCIAL_AUTH_CREATE_USERS.

IMHO, this approach is wrong. It suggests one of 2 mentioned ways, but there is little space for developer to define what he want's to do ( Developer can not control the way users are created. )

I think, creating user account etc, should be left to developer by default. Django-social-auth, could have built in ("generic") for creating account (with current funcionality), but usage of SOCIAL_AUTH_CREATE_USERS flag, should be deprecated (this looks like "magic")

Instead of using the flag, developer could do:

from social_auth.signals import auth_ok, create_user # or other name
auth_ok.connect(create_user) # or something else - custom way of creating users

Developer will be able to control the way users are created or use "generic user registration", if he doesn't need so much controll.

Example, why is this approach better:
Currently, when User uses multiple backends (providers), and has different first name and second name in those backends, everytime User shwitches backends, django-social-auth changes User model, according to extra_data from backend.

@phuihock

+1

One of the problem I am having now is that I can't ask user for his desired username. The default suggested username with long random numbers is likely not what user wants. Even if I assigned a callable to DEFAULT_USERNAME, it can't make use of the username user POSTed.

@cassus

I added a create_user signal in cassus/django-social-auth@c696f57

@cassus

Pull request: #109

@cassus

New pull request: #125

@Cruel

I'm in the same boat as phuihock, I can see no clean way to let the user select a username when creating an account. I like the create_user signal, it's what I'll use atm.

@omab
Owner

I think that a signaling system for such actions is not correct, instead I'm implementing a pipeline of actions with option to define custom actions and override the default pipeline.

@cassus

I like this idea. Any ETA on it?

@omab
Owner

It will be ready in a couple more days, not much time to hack these days :(

@jstlaurent

+1 from me as well.

I had the same kind of problem. I solved it by using my own view to replace complete, set the new user's is_active property to false and redirect them to a form where they can complete their registration and pick a username. It works well, but having more control over the flow would be great.

In any case, keep up the good work, the package is very useful.

@omab
Owner

My plans is to have this ready by the end of the weekend.

@cassus

Any progress?

@saidimu

Addressing this issue requires a clean API, something that appears to be non-trivial in the current code-base given the tight logic integration in the Django views.

Ideally, one should be able to go thro' the entire auth pipeline without requiring a Django view.

@omab omab referenced this issue from a commit
@omab Pipeline. Refs gh-90 6784213
@omab omab referenced this issue from a commit
@omab Remove debug code. Refs gh-90 7f74911
@omab
Owner

Please, take a look to the changes, feedback is well welcome.

@cassus

Looking at the code one thing is not clear to me. How can I interrupt and resume the pipeline? Eg. if I need the ability to ask the user for a username, or some confirmation. For this I need to stop the pipeline, return a HTTPResponse, and resume the pipeline when I get the next request with the information I need.
Currently I'm saving the auth data to a session, return HTTPResponse and call authenticate again with the data from the session when I get the confirmation.

@nemith

I agree with Cassus on this one. It sees like the pipleline is only good for backend processes and although it will be nice to extend/overwrite some of these built in processes I would like my users to choose a username post authentication and pre user creation.

So there needs to be some interaction with a view at some point to pull this off.

@omab
Owner

I'm working on a solution that allows to apply partial pipeline process, give me some days :)

@cassus

Great news! I can't wait to see it! :)

@Gromph

I was using django-social-auth 0.4.2 and had SOCIAL_AUTH_CREATE_USERS=False. I updated to 0.6.0 and started getting errors like "ValueError: Cannot assign None: "UserSocialAuth.user" does not allow null values." triggered by backends/pipeline/social.py:associate_user

It looks like the code is respecting the SOCIAL_AUTH_CREATE_USERS flag and not creating the user, but then going on to try to setup a new UserSocialAuth object with a null user.

So I found this bug and looked over the code, and a note about overriding the PIPELINE. So here is what I added to my settings.py:

SOCIAL_AUTH_PIPELINE = (
'social_auth.backends.pipeline.social.social_auth_user',
)

The only thing I want social-auth to do is authenticate the user. If the user doesn't exist yet I handle adding the entry to UserSocialAuth later.

So my question is, is this the right thing to do? It seems to work ok. And I wanted to put out the suggestion that it seems like it would be much simpler to continue to respect the SOCIAL_AUTH_CREATE_USERS flag and make it work as it did before. And too document how and why to override SOCIAL_AUTH_PIPELINE

thanks

@omab
Owner

The default pipeline should mimic the old behavior, I might have missing something in the process, will check it.

Overriding the pipeline is the desired idea now (also drop signal handlers in favor of custom pipeline processes, etc).

All this will be properly documented (when I get some time to finish all this stuff), sorry for the delays, meanwhile it's just the code :-(

@ygamretuta

hey, I don't know if this is the right place to post this but I wanted to customize the create_user action so that I can manually create a user given the extra details(access tokens, user ids, etc.) I obtain from authorizing another app. (my setup is an iPhone app is authorized, extra details are sent via Tastypie and creating the user via django-social-auth). As I browsed through the discussion, is it right to assume that I can't do what I want until this issue is resolved?

@kaminem64

I think the whole procedure "to authenticate user in backends's init.py and running a pipeline in it" is not reasonable.
SocialAuthBackend.authenticate should be replaced with another method which only runs get_user_details and returns kwargs, then developer can decide whether to use PIPIELINE or not in his/her own "view" (the django way).
The whole project is not in its pythonic way, exploring it's mechanism and mechanics is a time consuming project. I'll try my best to re-implement the procedure and send a pull request.

@kensaggy

Any word on updating the docs regarding the pipeline? Can someone please give an example of extending the create_user to create a UserProfile model for example... or obtain data from a backend after auth? or anything..

Thanks

@danxshap

Has the functionality to use a partial pipeline process been implemented? I'm trying to do the same thing as bennetb01 - let my users authenticate with the social provider and then enter data in a form before user creation. If someone could recommend a good way of doing this that would be awesome.

@kaminem64

@danxshap since the answer was too long I wrote it here:
http://solutioner.net/solutions/view/30/

@hassek

The idea is to set in settings this tuple:

SOCIAL_AUTH_PIPELINE = (
    'social_auth.backends.pipeline.social.social_auth_user',
    'social_auth.backends.pipeline.associate.associate_by_email',   
    'social_auth.backends.pipeline.user.get_username',  
    'MYAPP.views.MY_NEW_CREATE_USER',
    'social_auth.backends.pipeline.social.associate_user',
    'social_auth.backends.pipeline.social.load_extra_data',
    'social_auth.backends.pipeline.user.update_user_details',   
)

where MYAPP.views.MY_NEW_CREATE_USER will be a function with a different behavior returning the same data the original create_user function does.

This way you can modify any behaviour on the pipeline (even create your own pipeline if you desire, now that would be odd heh)

@mkai

I think the pipeline system is a huge improvement. I've already used it to override three of the methods to suit my app. However, in those cases I found myself copying/ pasting the whole method (not good) from django-social-auth because it's impossible to keep some default/ package behavior intact and customize only the pieces that I need.

So basically I'm not sure if a tuple of methods is a good design pattern when we have "the object" as a standard for that. Have you thought about implementing the pipeline in an object-oriented way?

class SocialAuthPipeline(object):
    def social_auth_user(...):
        ...

    def get_username(...):
        ...

Customizing would be easier then:

class CustomSocialAuthPipeline(SocialAuthPipeline):
    def get_username(...):
        username = super(CustomSocialAuthPipeline, self).get_username(...)
        return slugify(username)

What do you think?

@hassek

+1 to mkai

@omab
Owner

The tuple is needed to ensure the operation order, you will need it anyway to tell the object methods order in which they should be call. Having it inside an object or just a plain function won't make any difference IMO, you will have to override the method but copy the code and add your customization anyway since calling super() doesn't seems to be an option, if it is, then you could call the original django-social-auth method too.

The solution is here is better fragmentation IMO, that way we split the functions in smaller parts that can be really reusable by your overrides.

@nemith

@kaminem64 Modifying the base code shouldn't be required.

@gerardo

Great implementation! now it needs to be documented, there's nothing about it on the documentation.

@omab omab referenced this issue from a commit
@omab Partial pipeline. Refs #90 d53aef3
@omab
Owner

Partial pipeline work pushed, please check and comment back :)

Doc at http://django-social-auth.readthedocs.org/en/latest/pipeline.html#partial-pipeline

@Morpho

Partial pipeline is a BIG improvement! I already implemented it on a site. It seems very stable and easy to manage! Thumbs up!

@omab
Owner

I'm closing this one, feel free to reopen if needed.

@omab omab closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.