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

Email lockdown #19

Merged
merged 2 commits into from
Jan 17, 2017
Merged

Email lockdown #19

merged 2 commits into from
Jan 17, 2017

Conversation

Pomax
Copy link
Contributor

@Pomax Pomax commented Jan 15, 2017

fixes #16

@Pomax Pomax changed the title [wip] Email lockdown Email lockdown Jan 16, 2017
@Pomax
Copy link
Contributor Author

Pomax commented Jan 16, 2017

Tested this with my mozillafoundation.org account, which works, and my gmail.com and hotmail.com accounts, which get blocked. Admin login not being a moz login is essentially irrelevant as it uses its own login portal on /admin

@Pomax Pomax requested a review from alanmoo January 16, 2017 21:45
@alanmoo
Copy link
Contributor

alanmoo commented Jan 17, 2017

While this technically works, I'm concerned about the silent failure if non-Mozillians attempt to log in. I feel like we need some way to convey "you're not logged in but that's expected". Thoughts?

@Pomax
Copy link
Contributor Author

Pomax commented Jan 17, 2017

fair enough - rather than a redirect to / we can redirect to /login-failure so that we can serve up some content with a 403?

@alanmoo
Copy link
Contributor

alanmoo commented Jan 17, 2017

Or maybe a query param so that users can still get to the same page when merged with #25?

@Pomax
Copy link
Contributor Author

Pomax commented Jan 17, 2017

oh that's a good idea!

@Pomax
Copy link
Contributor Author

Pomax commented Jan 17, 2017

Updated the PR so that it now does a callback with a loggedin=... that is either True or False depending on whether authentication succeeded or not, and will either redirect the user to / with that argument if no original_url is provided, or redirects to the original_url with that query pair added (with a toggle to make sure we use ?loggedin=... or &loggedin=... depending on whether the original url already had query args)

@@ -21,8 +21,8 @@ def post_validate(request):
nonce = False

if request.data:
csrf_token = request.data['csrfmiddlewaretoken']
nonce = request.data['nonce']
csrf_token = request.data.get('csrfmiddlewaretoken', False)
Copy link
Contributor

Choose a reason for hiding this comment

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

Using .get allows a default? Is this just python behavior?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, it's a property of Python's dictionaries. You can either do direct access with array notation, or you can use get with a fallback, https://docs.python.org/2/library/stdtypes.html#dict.get

@Pomax Pomax merged commit f31b7a8 into master Jan 17, 2017
@Pomax Pomax deleted the email-lockdown branch January 17, 2017 20:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

restrict logins to moz-org users only
2 participants