-
Notifications
You must be signed in to change notification settings - Fork 46
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 option to create user if authenticated but not in local database #25
base: master
Are you sure you want to change the base?
Conversation
and normalized username to lower case to be consistent with django-auth-ldap
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi, thanks for the PR! Could you perhaps write some tests for the code you added?
@@ -13,4 +15,5 @@ | |||
OIDC_AUTH = { | |||
'OIDC_ENDPOINT': 'http://example.com', | |||
'OIDC_AUDIENCES': ('you',), | |||
'CREATE_USER': os.getenv('CREATE_USER', 'yes') == 'yes', |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
user = User.objects.get_by_natural_key(id_token.get('sub')) | ||
except User.DoesNotExist: | ||
msg = _('Invalid Authorization header. User not found.') | ||
raise AuthenticationFailed(msg) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
if not api_settings.CREATE_USER: | ||
self.assertEqual(resp.status_code, 401) | ||
else: | ||
self.assertEqual(resp.status_code, 200) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
Updated to pass testing, and added a test for OIDC_AUTH['CREATE_USER'] = True