Two factor authentication #1747

Open
wants to merge 3 commits into
from

Projects

None yet

5 participants

@umairwaheed
Collaborator
umairwaheed commented Sep 1, 2016 edited

The code is working. Tests are not working yet. The templates used by django-two-factor need an overhaul. Still need to clean commits as well.

@smarx
smarx commented Sep 1, 2016

Automated message from Dropbox CLA bot

@umairwaheed, it looks like you've already signed the Dropbox CLA. Thanks!

@umairwaheed umairwaheed changed the title from [WIP] Two factor authentication to Two factor authentication Sep 2, 2016
@umairwaheed
Collaborator

@timabbott done. 'django_templates/two_factor/_base.html' needs an improvement in my opinion.

@timabbott
Member

Looks like tests are still not working?

@umairwaheed
Collaborator

Checking.

@umairwaheed
Collaborator

@timabbott, the tests pass now. Please review.

@timabbott timabbott and 1 other commented on an outdated diff Sep 8, 2016
zerver/views/__init__.py
@@ -573,6 +573,11 @@ def get_context_data(self, **kwargs):
except KeyError:
pass
+ try:
+ context['next'] = self.request.GET['next']
+ except KeyError:
+ context['next'] = '/'
+
@timabbott
timabbott Sep 8, 2016 Member

I don't like this change, if we can avoid it, since it makes the login URL ugly unnecessarily. What we've done with Zulip in general is assume the next page is / if next is not specified, see for example the zulip_login_required code.

@umairwaheed
umairwaheed Sep 9, 2016 Collaborator

Yeah, I too am not a big fan of this. Let see what can be done.

@umairwaheed
umairwaheed Sep 9, 2016 Collaborator

Now the url no longer shows next but this code is still there because two_factor needs it internally. I have, actually, just used zulip_login_required to check login status before otp_required.

@umairwaheed
umairwaheed Sep 9, 2016 Collaborator

Actually, I have stopped assigning the default value to next.

@timabbott timabbott and 1 other commented on an outdated diff Sep 8, 2016
zerver/two_factor_migrations/0001_initial.py
@@ -0,0 +1,32 @@
+# -*- coding: utf-8 -*-
+from __future__ import unicode_literals
+
+from django.db import models, migrations
+from django.conf import settings
+import django.core.validators
@timabbott
timabbott Sep 8, 2016 Member

I don't understand why this is necessary -- is it a bug in the two_factor app that we're working around? It seems to me that django_two_factor should be creating the migrations that it needs automatically... I'd rather we investigate and figure out why that isn't happening so that we can use migrations from the two_factor app (since I worry that this setup will be hard to clean up later when two_factor does get fixed).

@umairwaheed
umairwaheed Sep 9, 2016 Collaborator

It seems that two_factor doesn't ship with a complete set of migrations. I can create fork.

@umairwaheed
umairwaheed Sep 9, 2016 Collaborator

This is fixed now. I just had to use the newer version.

@timabbott
Member
timabbott commented Sep 8, 2016 edited

Cool, thanks for working on this @umairwaheed! Overall it looks like you've cleaned up a bunch of issues in the original PR. I posted several comments on various oustanding issues.

@timabbott
Member

Also, we should add at least some backend tests for this feature.

@umairwaheed
Collaborator

Just need to add tests.

@umairwaheed
Collaborator

@timabbott, I have added tests for Two Factor Authentication. Please review, thanks.

@brainwane
Contributor

@umkay since you're interested in passwords & auth, figured you might want to take a look at this PR, and add any comments you might have :)

@timabbott
Member

@umairwaheed how does this work in terms of which users have to do 2FA in order to login?

It seems like we'd want users to be able to control whether 2FA is enabled for them; I don't see that in this PR though... am I missing something?

Also, is there configuration of API keys or whatnot required to actually use this?

(Also, you should probably rebase this past the merge conflicts).

@umairwaheed
Collaborator

It seems like we'd want users to be able to control whether 2FA is enabled for them; I don't see that in this PR though... am I missing something?

There is a button in user settings which allows the user to turn the 2FA on, https://github.com/zulip/zulip/pull/1747/files#diff-7481e1bc96ab6f0edf6bec70e87024a1R53. If you are saying that the whether the admin has the ability to enable 2FA for user then that is not implemented in this yet.

Also, you should probably rebase this past the merge conflicts

Sure.

@timabbott
Member

I don't think we need the admin to be able to toggle 2FA for a user; we'll eventually want a "require 2FA" admin option or something, but it's not pressing

@umairwaheed
Collaborator

OK. In that case the user can toggle it currently from the settings.

@umairwaheed
Collaborator

After rebasing, there is some problem with tests. Will try later tonight.

@umairwaheed
Collaborator

Few backend tests are still failing. Due to large number of conflicts I decided to squash commits to reduce the amount of work.

@timabbott
Member

sounds good re: squashing!

@umairwaheed
Collaborator

@timabbott, please review.

@umairwaheed
Collaborator

Also, is there configuration of API keys or whatnot required to actually use this?

Currently, I use fake backend provided by the plugin which doesn't require any configuration.

blablacio and others added some commits Jan 29, 2016
@blablacio @umairwaheed blablacio Integrate two-factor authentication. d31a22c
@umairwaheed umairwaheed static-analysis: Pass bytes to sha1(). fe9e2ec
@umairwaheed umairwaheed Fix 2FA integration.
The initial 2FA integration needed some updates to make
it work.
904ad82
@timabbott timabbott added this to the Zulip roadmap milestone Nov 18, 2016
@brainwane
Contributor

Just checked in with Tim who said this pull request is stuck waiting for some manual testing (on his part I believe) and the fact that there will be a hole on mobile (which probably just needs a disclaimer in the short term). I was curious because I am eager to use 2FA, so I figured I'd find out and share the news with anyone looking at this issue. :)

@umairwaheed
Collaborator

I'll rebase this. Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment