omab / django-social-auth

Django social authentication made simple
https://groups.google.com/group/django-social-auth
BSD 3-Clause "New" or "Revised" License
2.65k stars 759 forks source link

'SOCIAL_AUTH_ERROR_KEY' documented but not implemented #175

Closed evdb closed 12 years ago

evdb commented 12 years ago

It is mentioned in the README and in some config templates, but not in the actual code.

Using SOCIAL_AUTH_ERROR_KEY = 'social_errors' in settings.py I'm not seeing a value being set in the session on auth error (cancelling a twitter login) even though the user is redirected to the LOGIN_ERROR url.

omab commented 12 years ago

SOCIAL_AUTH_ERROR_KEY was deprecated in favor of django.contrib.messages (if enabled) or plain logger. I need to update the docs.

tijs commented 12 years ago

Is there any way to 'catch' the error messages and rewrite them into something a bit more user friendly (and translated)?

tijs commented 12 years ago

i.e. doing this in the view is hardly ideal:

storage = messages.get_messages(request)
for message in storage:
    regex = re.compile("(?<=\(\')[\w\s\.]+")
    message.message = regex.findall(message.message)[0]

Instead it would be nice if you could configure a dictionary of messages for each error message a backend could return for instance. Sorry this is turning into a seperate feature request i guess..

omab commented 12 years ago

@tijs, there's wat to "catch" the error messages beside the messages django structure, let me think about it and I'll try to come with something.

tijs commented 12 years ago

@omab great! let me know if you need some more input

thomasjenkins commented 12 years ago

Just stumbled across this too. I'd like to avoid showing strings like "access_denied" to the end user for things like FB user cancel. It would also be nice to know DSA was the source of the messages so they could be displayed differently when using the same template for non-social authn. Perhaps, I just missed that last feature.

I would have just subscribed to the bug instead of adding a useless "me too" comment, but I'm not sure how to do that on GitHub without commenting.

On a related-ish side note, the SOCIAL_AUTH_RAISE_EXCEPTIONS = False requirement to debug the actual end-user flow for provider error responses could possibly be made more prominent in the docs. The behavior of raising ValueError for things like user cancel was quite unexpected and I assumed I had mucked something up.

tijs commented 12 years ago

@thomasjenkins did you find something in the docs though? last i looked i did not find anything on how to handle this better...

thomasjenkins commented 12 years ago

@tijs the docs thing I mentioned was just about setting SOCIAL_AUTH_RAISE_EXCEPTIONS to disable DSA from raising a ValueError for any provider response other that successful auth. Sorry, confusing on my part mentioning it here. I wonder if user-caused responses should raise something else, or not raise at all, as they aren't exceptional but rather an expected user flow, but I should put that in another issue.

I don't have a better solution for the error messages. I resort to just displaying a generic couldn't-sign-in-with-provider message if there is are any messages with level "error" returning from social-auth.

tijs commented 12 years ago

@thomasjenkins yeah thats what i do now too, trouble is that local issues such as a social account already being linked to a different user also end up in that flow and are hard to differentiate from the rest. Although i'm now thinking they may not have the same error level...

omab commented 12 years ago

@thomasjenkins, @tijs, please check the last change linked on this thread.

tijs commented 12 years ago

@omab ah very cool! i'll try that out and let you know if i run into any issues but it looks like that would resolve most of my headaches.

omab commented 12 years ago

@tijs, I know it might not be quite straightforward, it's just that I'm always looking for the open solutions that might fit best to the whole django projects jungle.

thomasjenkins commented 12 years ago

@omab you beat me to proposing a pull request with more or less the exact exception changes in 0db8f14. I guess I'll have to find another thing to contribute. :)

I'll try those changes later today, but at a glance those should work well.

omab commented 12 years ago

Seems that the changeset solved the issue, I'm closing the ticket, reopen if needed.