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.

Sign up for GitHub

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 option to create user if authenticated but not in local database #25

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Added option to create user if authenticated but not in local database #25

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
a4f9ae2
User.objects.get_by_natural_key with User.objects.get_or_create,
johnedstone Sep 7, 2017
afd1a27
Updated to pass tox test
johnedstone Sep 26, 2017
745e1a6
README update
johnedstone Sep 27, 2017
6202a30
README update
johnedstone Sep 27, 2017
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
3 changes: 3 additions & 0 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,9 @@ OIDC_AUTH = {

# (Optional) Token prefix in Bearer authorization header (default 'Bearer')
'BEARER_AUTH_HEADER_PREFIX': 'Bearer',

# (Optional) Create user if user authenticates but is not in the local database (default False)
'CREATE_USER': False,
}
```

Expand Down
26 changes: 21 additions & 5 deletions oidc_auth/authentication.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,11 +18,27 @@

def get_user_by_id(request, id_token):
User = get_user_model()
try:
user = User.objects.get_by_natural_key(id_token.get('sub'))
except User.DoesNotExist:
msg = _('Invalid Authorization header. User not found.')
raise AuthenticationFailed(msg)
if api_settings.CREATE_USER:
email = id_token.get('email', None)
first_name = id_token.get('given_name', '')
last_name = id_token.get('family_name', '')
if email:
user, created = User.objects.get_or_create(username=id_token.get('sub').lower(),
defaults={'first_name': first_name,
'last_name': last_name,
'email': email,
})
else:
user, created = User.objects.get_or_create(username=id_token.get('sub'.lower()),
defaults={'first_name': first_name,
'last_name': last_name,
})
else:
try:
user = User.objects.get_by_natural_key(id_token.get('sub'))
except User.DoesNotExist:
msg = _('Invalid Authorization header. User not found.')
raise AuthenticationFailed(msg)
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be nice if there were some tests for this, since this is quite some modification.

Copy link
Author

Choose a reason for hiding this comment

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

see my comments on testing below - thanks

return user


Expand Down
3 changes: 3 additions & 0 deletions oidc_auth/settings.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,9 @@

'JWT_AUTH_HEADER_PREFIX': 'JWT',
'BEARER_AUTH_HEADER_PREFIX': 'Bearer',

# Create user if user authenticates but is not in the local database
'CREATE_USER': False,
}

# List of settings that may be in string import notation.
Expand Down
3 changes: 3 additions & 0 deletions tests/settings.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
import os

SECRET_KEY='secret'
DATABASES = {
'default': {
Expand All @@ -13,4 +15,5 @@
OIDC_AUTH = {
'OIDC_ENDPOINT': 'http://example.com',
'OIDC_AUDIENCES': ('you',),
'CREATE_USER': os.getenv('CREATE_USER', 'yes') == 'yes',
Copy link
Contributor

Choose a reason for hiding this comment

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

Wouldn't this make more sense as a boolean?

Copy link
Author

Choose a reason for hiding this comment

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

This is a boolean, right? It evaluates to true or false. In my mind it's hard to pass in an env var as a boolean. So in python, it's a boolean. As an env var it's a string. If one wants to use the string 'True' or the string 'False', okay - maybe that would be clearer. Or maybe not using the string 'True', it makes one think, and see what's happening. I'm using the same approach as found in many Django books, here. Thanks for your comments here. It's good to discuss. Hope that makes sense.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hehe, whoops 😅 Must have been that Friday afternoon slip 🙈 Never mind the comment

}
6 changes: 5 additions & 1 deletion tests/test_authentication.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
from rest_framework.views import APIView
from requests import Response, HTTPError, ConnectionError
from oidc_auth.authentication import JSONWebTokenAuthentication, BearerTokenAuthentication
from oidc_auth.settings import api_settings
import sys
if sys.version_info > (3,):
long = int
Expand Down Expand Up @@ -208,7 +209,10 @@ def test_with_too_new_jwt(self):
def test_with_unknown_subject(self):
auth = 'JWT ' + make_id_token(self.user.username + 'x')
resp = self.client.get('/test/', HTTP_AUTHORIZATION=auth)
self.assertEqual(resp.status_code, 401)
if not api_settings.CREATE_USER:
self.assertEqual(resp.status_code, 401)
else:
self.assertEqual(resp.status_code, 200)
Copy link
Contributor

Choose a reason for hiding this comment

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

It might be nicer to split this up into two tests, one that has the variable set to True and one to False. Because one of the test paths won't be reached this way.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for your comments on testing. Unfortunately, I'm not in a environment where I can test, or do any more work on oidc. At the young age of 64, I was asked to retire, so I'm only doing a little coding now, e.g. aws lambda (zappa). And I don't have access to an environment where I can continue to test and improve this type of authentication. Perhaps you can continue to improve this. Thanks for your comments


def test_with_multiple_audiences(self):
auth = 'JWT ' + make_id_token(self.user.username, aud=['you', 'me'])
Expand Down
4 changes: 3 additions & 1 deletion tox.ini
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
[tox]
envlist =
{py27,py34}-django{18,19,110}-drf{32}
{py27,py34}-django{18,19,110}-drf{32}-createuser{yes,no}

[testenv]
commands =
Expand All @@ -9,6 +9,8 @@ setenv =
PYTHONDONTWRITEBYTECODE=1
DJANGO_SETTINGS_MODULE=tests.settings
PYTHONPATH={toxinidir}
createuseryes: CREATE_USER=yes
createuserno: CREATE_USER=no
deps =
django18: Django==1.8.4
django19: Django==1.9.12
Expand Down