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

Added social auth... #405

Open
wants to merge 40 commits into
base: develop
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
40 commits
Select commit Hold shift + click to select a range
27b9ba6
Merge branch 'develop' of https://github.com/prjts/openbudgets into d…
Tudmotu Dec 23, 2013
6f3e3a5
Merge branch 'develop' of https://github.com/prjts/openbudgets into d…
Tudmotu Jan 13, 2014
877e8e6
BREAKING; Removed vendor dir.
Tudmotu Feb 3, 2014
975919e
Added bower files.
Tudmotu Feb 3, 2014
154a93f
STILL BREAKING; Added new bower deps.
Tudmotu Feb 3, 2014
7a62df1
idos tired commit
idoivri Feb 7, 2014
07e736b
fix
idoivri Feb 7, 2014
de224fd
Fixed jqScroll and setImmediate dependencies.
Tudmotu Feb 7, 2014
f60101e
Migrated all files [hopefully] to use the new dir structure of bower.
Tudmotu Feb 7, 2014
6a71cc2
Merge branch 'develop' of https://github.com/hasadna/omuni-budget int…
Tudmotu Feb 7, 2014
8dc9189
Merge branch 'develop' into feature/volo-to-bower
Tudmotu Feb 7, 2014
9cd4dd7
fix 355
dovp Feb 7, 2014
234cd85
fix 166 - Home page design implementation
dovp Feb 7, 2014
2358b76
Added social auth...
Feb 7, 2014
d83e4d8
Added requirements, removed unneeded imports from exception middlewar…
Feb 7, 2014
f941e4d
Added strict unique email constraint
Feb 10, 2014
69a839b
Removed break point
Feb 10, 2014
2ad75db
Added message param
Feb 10, 2014
7d4b614
Type fix, paramaterized unique email excpetion
Feb 10, 2014
eefeeaf
Reset requirements to project's default + django-social-auth 0.7.28
Feb 11, 2014
95000c8
Removed uneeded balnk spaces, and a markup error
Feb 11, 2014
61f7512
Some more aliasing and shim fixes.
Tudmotu Feb 16, 2014
a68a8d7
Added alias for spin.js.
Tudmotu Feb 16, 2014
4074506
Merge branch 'develop' of https://github.com/hasadna/omuni-budget int…
Tudmotu Feb 16, 2014
6fc8016
Merge pull request #403 from idoivri/develop
pwalsh Mar 7, 2014
abeb563
Merge pull request #404 from dovp/develop
pwalsh Mar 7, 2014
05ef643
Last enhancements towrds a merge
Mar 9, 2014
fb1ee77
added an explanation about social credentials
Mar 9, 2014
ac18294
Cleaned up and updated JS dependencies
ydaniv Mar 11, 2014
ee71430
Updated uijet to 0.0.34
ydaniv Mar 11, 2014
9a5c366
Fixed explorer app to use updated dependencies
ydaniv Mar 11, 2014
650f643
Updated JS dependencies
ydaniv Mar 11, 2014
9c3c79b
Merge branch 'develop' of https://github.com/hasadna/openmuni-budgets…
ydaniv Mar 11, 2014
66cd83e
Updated processed base css files
ydaniv Mar 11, 2014
1441faf
Update requirements.txt
pwalsh Mar 17, 2014
6749cd0
Making the pattern to loading local.py much easier so it is on by def…
pwalsh Mar 17, 2014
3faba06
Merge branch 'develop' of https://bitbucket.org/pwalsh/openbudgets in…
pwalsh Mar 17, 2014
f0b76ea
clean
pwalsh Mar 17, 2014
841760f
Merge branch 'develop' of github.com:hasadna/openmuni-budgets into de…
pwalsh Mar 17, 2014
44d074e
Merge branch 'social_auth' of github.com:BeOleg/openmuni-budgets into…
Mar 24, 2014
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions openbudgets/apps/accounts/middleware.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
from social_auth.middleware import SocialAuthExceptionMiddleware
from django.conf import settings


class OpenBudgetsSocialAuthExceptionMiddleware(SocialAuthExceptionMiddleware):
def get_redirect_uri(self, request, exception):
return settings.LOGIN_URL
8 changes: 8 additions & 0 deletions openbudgets/apps/accounts/social_exceptions.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
from social_auth.exceptions import SocialAuthBaseException
from django.utils.translation import ugettext
Copy link

Choose a reason for hiding this comment

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

I think that using ugettextlazy here will be better. and import as _ for make the code cleaner...

Copy link
Member

Choose a reason for hiding this comment

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

@yprez, I'm not sure we need a lazy gettext here. The lazy form is usually needed when you call _(...) in runtime when evaluating a class' attribute that's not wrapped in a callable, e.g. a model's field.
Here the text is wrapped in a function, so I don't see a reason for making it lazy.
Don't you agree?

Copy link

Choose a reason for hiding this comment

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

I suppose there is no functional difference in this case.
Personally I always use lazy gettext, so the function returns a lazy object, unless it's problematic. Seems cleaner to evaluate the gettext on rendering than on function call.

Maybe it's just my personal preference, I haven't given it much thought until now...



class NonIdenticalEmailAddress(SocialAuthBaseException):
def __unicode__(self):
return ugettext(u'Identical email error: %(message)s' % {'message':
self.message})
Copy link

Choose a reason for hiding this comment

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

This should prabably be:

return _(u'Identical email error: %s') % self.message

So that the string formatting is done after the translation.

Copy link
Member

Choose a reason for hiding this comment

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

@yprez is correct, however, it's still better to keep the named params form:

return _(u'Identical email error: %(message)s') % {'message': self.message}

this helps a lot later in the translation.

Copy link
Member

Choose a reason for hiding this comment

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

@ydaniv agreed, we should always use named params, and actually, in the codebase we mostly use the newer python string formatting, which I definitely prefer:

'{msg}'.format(msg=self.message)

At least - it needs to be lazy.

Copy link

Choose a reason for hiding this comment

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

agreed.

 _(u'Identical email error: {message}').format(self.message)

Is best imo.

Copy link
Member

Choose a reason for hiding this comment

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

Agreed. This form of formating is also best IMO
בתאריך 7 במרץ 2014 12:48, "Yuri Prezument" [email protected] כתב:

In openbudgets/apps/accounts/social_exceptions.py:

@@ -0,0 +1,8 @@
+from social_auth.exceptions import SocialAuthBaseException
+from django.utils.translation import ugettext
+
+
+class NonIdenticalEmailAddress(SocialAuthBaseException):

  • def unicode(self):
  •    return ugettext(u'Identical email error: %(message)s' % {'message':
    
  •                                                             self.message})
    

agreed.

_(u'Identical email error: {message}').format(self.message)

Is best imo.


Reply to this email directly or view it on GitHubhttps://github.com//pull/405/files#r10379261
.

Copy link
Author

Choose a reason for hiding this comment

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

Ok, will path this up and update the PR, as well as other improvements.

56 changes: 56 additions & 0 deletions openbudgets/apps/accounts/social_pipeline.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
from django.core.exceptions import MultipleObjectsReturned, ObjectDoesNotExist
from django.utils.translation import ugettext
from social_auth.models import UserSocialAuth
from .models import Account
from social_auth.exceptions import AuthException
from social_exceptions import NonIdenticalEmailAddress


def associate_by_email(details, user=None, *args, **kwargs):
"""Return user entry with same email address as one returned on details."""
email = details.get('email', None)

if user and user.email == email:
return None
elif not email or (user and user.email != email):
msg = ugettext('Social email address and account email ' +
'address must be idnetical')
raise NonIdenticalEmailAddress(msg)
if email:
# Try to associate accounts registered with the same email address,
# only if it's a single object. AuthException is raised if multiple
# objects are returned.
try:
return {'user': UserSocialAuth.get_user_by_email(email=email)}
except MultipleObjectsReturned:
raise AuthException(kwargs['backend'], 'Not unique email address.')
except ObjectDoesNotExist:
pass


def create_user(backend, details, response, uid, username, user=None, *args,
**kwargs):
"""Create user. Depends on get_username pipeline."""
if user:
return {'user': user}
if not username:
return None

# Avoid hitting field max length
email = details.get('email')
original_email = None
if email and UserSocialAuth.email_max_length() < len(email):
original_email = email
email = ''
first_name = details.get('first_name') or 'John'
last_name = details.get('last_name') or 'Doe'
password = Account.objects.make_random_password()

return {
'user': UserSocialAuth.create_user(username=username, email=email,
first_name=first_name,
last_name=last_name,
password=password),
'original_email': original_email,
'is_new': True
}
16 changes: 14 additions & 2 deletions openbudgets/apps/accounts/templates/accounts/account_detail.html
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,19 @@
</div>
</div>


<div class="social-accounts">
{% for provider,account in social_auth.items %}
{% if account and provider %}
<div class="account">
<form method="POST" action="{% url "socialauth_disconnect_individual" provider account.id %}"
class="social-revoke-form">
{% csrf_token %}
<input type="submit" class="social-revoke-control" value="-"> {{ provider }}
Copy link
Member

Choose a reason for hiding this comment

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

@BeOleg <input> is a non-content element. Perhaps it would be better to replace it with a <button> tag and put the {{ provider }} as its content?

Copy link
Author

Choose a reason for hiding this comment

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

The idea is to have a - sign side by side with the network's name, to symbolize the revoke of the credentials of this certain network.

Same thing can be achieved by placing {{provider}} in the 'value' attribute of the input element, a button element will work as well, as long as it's has a type='submit' attribute, since I only accept POST request to the credentials revoke resource to prevent XSS.

Wanted to enhance the design of this entire thing, but frankly could not figure out which less file applies to this section :(

Copy link
Member

Choose a reason for hiding this comment

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

Don't worry abut UI behaviour, until we do new UI work with Ran.

</form>
</div>
{% endif %}
{% endfor %}
</div>



Expand All @@ -42,7 +54,7 @@

{% block content %}
<div id="filter">
<select>
<select>socialauth_disconnect
<option>{% trans 'Show everything' %}</option>
<option>{% trans 'Item comments' %}</option>
<option>{% trans 'Vizualization states' %}</option>
Expand Down
1 change: 0 additions & 1 deletion openbudgets/apps/accounts/urls/ui.py
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@
AccountUpdateView, AccountRegistrationView, account_login, \
account_password_change, account_password_reset


urlpatterns = patterns('',

url(r'^activate/complete/$',
Expand Down
3 changes: 2 additions & 1 deletion openbudgets/apps/accounts/views/ui.py
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@
CustomRegistrationForm, CustomAuthenticationForm, CustomPasswordChangeForm,\
CustomPasswordResetForm
from openbudgets.commons.mixins.views import UserDataObjectMixin, JSONResponseMixin
from django.contrib.messages import get_messages


class AccountDetailView(LoginRequiredMixin, UserDataObjectMixin, DetailView):
Expand Down Expand Up @@ -153,7 +154,6 @@ def account_login(request, template_name='registration/login.html',
redirect_field_name=REDIRECT_FIELD_NAME,
authentication_form=CustomAuthenticationForm,
current_app=None, extra_context=None):

redirect_to = request.REQUEST.get(redirect_field_name, '')

if request.method == "POST":
Expand Down Expand Up @@ -206,6 +206,7 @@ def account_login(request, template_name='registration/login.html',
redirect_field_name: redirect_to,
'site': current_site,
'site_name': current_site.name,
'messages': get_messages(request),
Copy link

Choose a reason for hiding this comment

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

Why not use the messages context processor here? It's already enabled in the settings (

'django.contrib.messages',
)

}

if extra_context is not None:
Expand Down
16 changes: 15 additions & 1 deletion openbudgets/commons/templates/registration/login.html
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,16 @@
{% if form.errors %}
<p>{% trans 'Incorrect credentials' %}</p>
{% endif %}

{% if messages %}
<ul class="messages">
{% for message in messages %}
<li{% if message.tags %} class="{{ message.tags }}"{% endif %}>
{% if message.level == DEFAULT_MESSAGE_LEVELS.ERROR %}{% trans 'Important:' %} {% endif %}
{{ message }}
</li>
{% endfor %}
</ul>
{% endif %}
<section id="login">
<form class="box-3 _float-left" action="{% url 'auth_login' %}" method="post">
{% csrf_token %}
Expand All @@ -20,6 +29,11 @@
{% include 'partials/_form_action_login.html' %}
<input type="hidden" name="next" value="{{ next }}" />
</fieldset>
<div>
<a href="{% url "socialauth_begin" "facebook" %}" title="{% trans 'Login with Facebook' %}">FB</a>
<a href="{% url "socialauth_begin" "google" %}" title="{% trans 'Login with Google' %}">G+</a>
<a href="{% url "socialauth_begin" "twitter" %}" title="{% trans 'Login with Twitter' %}">Tw</a>
</div>
<div>
&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;
<a href="{% url 'auth_password_reset' %}" title="Forgot password? Reset it here">{% trans 'Forgot password' %}?</a>
Expand Down
40 changes: 39 additions & 1 deletion openbudgets/settings/__init__.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
import os


DEBUG = True

TEMPLATE_DEBUG = DEBUG
Expand Down Expand Up @@ -103,6 +102,7 @@
'django.middleware.clickjacking.XFrameOptionsMiddleware',
'corsheaders.middleware.CorsMiddleware',
'django.middleware.cache.FetchFromCacheMiddleware',
'openbudgets.apps.accounts.middleware.OpenBudgetsSocialAuthExceptionMiddleware',
)

INSTALLED_APPS = (
Expand Down Expand Up @@ -143,8 +143,45 @@
'openbudgets.apps.transport',
'openbudgets.apps.api',
'openbudgets.commons',
'social_auth',
)

AUTHENTICATION_BACKENDS = (
'social_auth.backends.facebook.FacebookBackend',
'social_auth.backends.twitter.TwitterBackend',
'social_auth.backends.google.GoogleBackend',
'social_auth.backends.google.GoogleOAuth2Backend',
'django.contrib.auth.backends.ModelBackend',
)

SOCIAL_AUTH_PIPELINE = (
'social_auth.backends.pipeline.social.social_auth_user',
'openbudgets.apps.accounts.social_pipeline.associate_by_email', # only difference from default
'social_auth.backends.pipeline.user.get_username',
# 'social_auth.backends.pipeline.user.create_user',
'openbudgets.apps.accounts.social_pipeline.create_user',#custom pipeline
'social_auth.backends.pipeline.social.associate_user',
'social_auth.backends.pipeline.social.load_extra_data',
'social_auth.backends.pipeline.user.update_user_details'

)

try:
Copy link
Member

Choose a reason for hiding this comment

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

I don't understand why you wrap all of these in a try/except.

Copy link
Author

Choose a reason for hiding this comment

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

@ydaniv asked me to catch an import error there, in case some developers
don't have the file with the social media tokens & secret keys.

On Fri, Mar 7, 2014 at 8:08 AM, Paul Walsh [email protected] wrote:

In openbudgets/settings/init.py:

)

+SOCIAL_AUTH_PIPELINE = (

  • 'social_auth.backends.pipeline.social.social_auth_user',
    
  • 'openbudgets.apps.accounts.social_pipeline.associate_by_email', # only difference from default
    
  • 'social_auth.backends.pipeline.user.get_username',
    
  • # 'social_auth.backends.pipeline.user.create_user',
    
  • 'openbudgets.apps.accounts.social_pipeline.create_user',#custom pipeline
    
  • 'social_auth.backends.pipeline.social.associate_user',
    
  • 'social_auth.backends.pipeline.social.load_extra_data',
    
  • 'social_auth.backends.pipeline.user.update_user_details'
    
  • )
    +
    +try:

I don't understand why you wrap all of these in a try/except.

Reply to this email directly or view it on GitHubhttps://github.com//pull/405/files#r10374253
.

Copy link
Member

Choose a reason for hiding this comment

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

So, shouldn't this stuff be in the said file then, and not here at all? Seeing as it all strictly depends on that file? Which file are you referring to?

Copy link
Member

Choose a reason for hiding this comment

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

if it these are project's defaults and it makes sense to have them here then they should be left here, if not then they can be removed and we only need import social_config in a try-catch clause.

from . import social_config
FACEBOOK_APP_ID=social_config.social_config_vars.get('FACEBOOK_APP_ID') or None
FACEBOOK_API_SECRET=social_config.social_config_vars.get('FACEBOOK_API_SECRET') or None
TWITTER_CONSUMER_KEY=social_config.social_config_vars.get('TWITTER_CONSUMER_KEY') or None
TWITTER_CONSUMER_SECRET=social_config.social_config_vars.get('GOOGLE_CONSUMER_SECRET') or None
FACEBOOK_EXTENDED_PERMISSIONS = ['email']
SOCIAL_AUTH_USERNAME_IS_FULL_EMAIL = True
SOCIAL_AUTH_FORCE_POST_DISCONNECT = True
SOCIAL_AUTH_REDIRECT_IS_HTTPS = False #?
SOCIAL_AUTH_CREATE_USERS= True
SOCIAL_AUTH_FORCE_RANDOM_USERNAME=False
SOCIAL_AUTH_PROTECTED_USER_FIELDS = ['email', 'first_name', 'last_name']
except ImportError:
pass

TEMPLATE_CONTEXT_PROCESSORS = (
'django.contrib.auth.context_processors.auth',
'django.core.context_processors.debug',
Expand All @@ -157,6 +194,7 @@
'openbudgets.commons.context_processors.site',
'openbudgets.commons.context_processors.forms',
'openbudgets.commons.context_processors.openbudgets',
'social_auth.context_processors.social_auth_by_name_backends',
)

LOGGING = {
Expand Down
25 changes: 25 additions & 0 deletions openbudgets/settings/social_config.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
social_config_vars = {}
social_config_vars['FACEBOOK_APP_ID']='393614634107619'#local
social_config_vars['FACEBOOK_API_SECRET']='67745d338966c0049543cd82b4601d49'
social_config_vars['TWITTER_CONSUMER_KEY']='v8v3kladozPuOmKdRF3aTw'
social_config_vars['TWITTER_CONSUMER_SECRET']='makIoMXdTmw6SI8DKsuqRH9ozESgKxXPuxEoMTyaZs'
social_config_vars['GOOGLE_CONSUMER_KEY']='318602050989.apps.googleusercontent.com'#redirects to openmuni.org.il
social_config_vars['GOOGLE_CONSUMER_SECRET']='F6enSvoTvkm9-4_NwsOCWYr7'

#staging
#social_config_vars['FACEBOOK_APP_ID']=''
#social_config_vars['FACEBOOK_API_SECRET']=''

# social_config_vars['TWITTER_CONSUMER_KEY']=''
# social_config_vars['TWITTER_CONSUMER_SECRET']=''
# social_config_vars['GOOGLE_CONSUMER_KEY']=''
# social_config_vars['GOOGLE_CONSUMER_SECRET']=''

#prod
#social_config_vars['FACEBOOK_APP_ID']=''
#social_config_vars['FACEBOOK_API_SECRET']=''

# social_config_vars['TWITTER_CONSUMER_KEY']=''
# social_config_vars['TWITTER_CONSUMER_SECRET']=''
# social_config_vars['GOOGLE_CONSUMER_KEY']=''
# social_config_vars['GOOGLE_CONSUMER_SECRET']=''
2 changes: 2 additions & 0 deletions openbudgets/urls.py
Original file line number Diff line number Diff line change
Expand Up @@ -57,5 +57,7 @@

url(r'^',
include('openbudgets.apps.pages.urls')),
url(r'',
include('social_auth.urls')),

)
103 changes: 62 additions & 41 deletions requirements.txt
Original file line number Diff line number Diff line change
@@ -1,43 +1,64 @@
django==1.5.5
Django==1.5.5
Fabric==1.8.1
Jinja2==2.7.2
MarkupSafe==0.18
Pillow==2.3.0
Pygments==1.6
Quilt==0.1-alpha
South==0.8.4
Sphinx==1.2.1
amqp==1.4.2
anyjson==0.3.3
argparse==1.2.1
billiard==3.3.0.14
celery==3.1.8
ckanclient==0.10
coverage==3.7.1
cuisine==0.6.5
django-autoslug==1.7.2
django-braces==1.3.1
django-celery==3.1.1
django-cors-headers==0.12
django-debug-toolbar==1.0.1
django-grappelli==2.4.8
django-gravatar2==1.1.3
django-modeltranslation==0.7.3
django-oauth-toolkit==0.6.0
django-pdb==0.3.2
django-redis-cache==0.10.2
django-registration==0.9b1
django-social-auth==0.7.28
django-subdomains==2.0.4
django-taggit==0.11.2
django-uuidfield==0.4.0
djangorestframework==2.3.9
gunicorn
psycopg2
south
dock==0.1.1
docutils==0.11
ecdsa==0.10
factory-boy==2.3.1
gdata==2.0.18
grappelli-modeltranslation==0.1.2
gunicorn==18.0
hiredis==0.1.2
httplib2==0.8
ipdb==0.8
ipython==1.1.0
jsonfield==0.9.20
kombu==3.0.11
mock==1.0.1
oauth2==1.5.211
oauthlib==0.6.1
paramiko==1.12.1
psycopg2==2.5.2
pycrypto==2.6.1
pystache==0.5.3
python-openid==2.2.5
python-quilt==0.2
pytz==2013b
tablib
celery
factory-boy
coverage
redis
hiredis
pillow
sphinx
raven
gdata
ckanclient
jsonfield
pystache
mock
django-celery
django-oauth-toolkit
django-redis-cache
django-autoslug
django-pdb
django-gravatar2
django-taggit
django-debug-toolbar
django-braces
django-modeltranslation
django-grappelli==2.4.8
django-cors-headers
django-subdomains
grappelli-modeltranslation
git+https://github.com/dcramer/django-uuidfield.git#egg=uuidfield
git+https://github.com/mozilla/unicode-slugify

# upstream still doesn't fully support custom user model
hg+https://bitbucket.org/prjts/django-registration

# temp, while working
git+https://github.com/pwalsh/quilt#egg=quilt
git+https://github.com/pwalsh/dock#egg=dock
raven==4.0.4
redis==2.9.1
six==1.3.0
sqlparse==0.1.10
tablib==0.9.11
unicode-slugify==0.1.1
wsgiref==0.1.2