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

Login API's for DevAuthBackEnd for Android project #851

Closed
wants to merge 4 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@kunall17
Copy link
Contributor

kunall17 commented Jun 1, 2016

More details at #65

@smarx

This comment has been minimized.

Copy link

smarx commented Jun 1, 2016

Automated message from Dropbox CLA bot

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

return json_success(extra_context)



This comment has been minimized.

@timabbott

timabbott Jun 3, 2016

Member

stylistically, we use just 1 newline between functions

return json_error("This user is not registered; do so from a browser.", data={"reason": "unregistered"}, status=403)
return json_error("Your username or password is incorrect.", data={"reason": "incorrect_creds"}, status=403)
if not user_profile.is_active:
return json_error("Your account has been disabled.", data={"reason": "disabled"}, status=403)

This comment has been minimized.

@timabbott

timabbott Jun 3, 2016

Member

This should also check for user_profile.realm.deactivated, similar to our other authorization code paths

def api_dev_android_direct_login(request, username=REQ):
# This function allows logging in without a password on Android app and should only be called in development
# environments. It may be called if the DevAuthBackend is included in settings.AUTHENTICATION_BACKENDS
if ( not dev_auth_enabled()) or settings.PRODUCTION:

This comment has been minimized.

@timabbott

timabbott Jun 3, 2016

Member

I would write this as:
if not dev_auth_enabled() or settings.PRODUCTION:

if return_data.get("valid_attestation") == True:
# We can leak that the user is unregistered iff they present a valid authentication string for the user.
return json_error("This user is not registered; do so from a browser.", data={"reason": "unregistered"}, status=403)
return json_error("Your username or password is incorrect.", data={"reason": "incorrect_creds"}, status=403)

This comment has been minimized.

@timabbott

timabbott Jun 3, 2016

Member

It seems like several of the lines above are unnecessary, since I don't think valid_attestation is set by anything other than the Google auth code paths. Can you clean this up?

@timabbott

This comment has been minimized.

Copy link
Member

timabbott commented Jun 3, 2016

Cool, thanks for working on this @kunall17! This generally looks pretty good structurally, though I posted a few comments on improvements you should make. Also, this branch has some lint failures detected by the Travis CI checks (you can run tools/lint-all to get them locally), can you investigate and debug those?

Finally, I'd love to see some tests for this new authentication code path; you probably want to add something similar to FetchAPIKeyTest in zerver/tests/test_auth_backends.py.

@neerajwahi as FYI since you may want this for zulip-mobile (and @niftynei FYI as well).

@kunall17 kunall17 force-pushed the kunall17:devauthloginandroid branch 4 times, most recently from cb74615 to ac7f7be Jun 3, 2016

@timabbott

This comment has been minimized.

Copy link
Member

timabbott commented Jun 5, 2016

@kunall17 it looks like you posted a new version, which is great! I'll take a look. But next time you should post a comment when you do so, since GitHub doesn't send an automated notification when you push a new version of the code (so I just happened to notice this had new code).

user_profile = authenticate(username=username)
login(request, user_profile)
if user_profile.realm.deactivated:
raise json_error(_("Realm for account has been deactivated"))

This comment has been minimized.

@timabbott

timabbott Jun 5, 2016

Member

This is indented too far

# environments. It may be called if the DevAuthBackend is included in settings.AUTHENTICATION_BACKENDS
if not dev_auth_enabled() or settings.PRODUCTION:
# This check is probably not required, since authenticate would fail without an enabled DevAuthBackend.
return json_error(_("Dev enviornment not enabled.", data={"reason": "dev_disabled"}, status=403))

This comment has been minimized.

@timabbott

timabbott Jun 5, 2016

Member

"environment" is mispelled

@csrf_exempt
@require_post
@has_request_variables
def api_dev_android_direct_login(request, username=REQ):

This comment has been minimized.

@timabbott

timabbott Jun 5, 2016

Member

I think REQ now only supports being used with parens (e.g. username=REQ()) -- we eliminated the old bare REQ syntax recently.

This comment has been minimized.

@kunall17

kunall17 Jun 5, 2016

Author Contributor

Well honestly I don't know much about python, but the code here works.

This comment has been minimized.

@timabbott

timabbott Jun 5, 2016

Member

What I was saying is when you rebase onto master (which we'll need to do in order to merge this), I think this will stop working. Can you switch it?

This comment has been minimized.

@kunall17

kunall17 Jun 5, 2016

Author Contributor

It would work If I replace the REQ with REQ() as I see these changes have been implemented in this commit 960144a ,
So I should just replace those REQ right?

This comment has been minimized.

@timabbott

timabbott Jun 5, 2016

Member

Yep!

-Tim Abbott (mobile)
On Jun 5, 2016 8:24 AM, "Kunal Gupta" notifications@github.com wrote:

In zerver/views/init.py
#851 (comment):

@@ -572,6 +572,36 @@ def dev_direct_login(request, **kwargs):
return HttpResponseRedirect("%s%s" % (settings.EXTERNAL_URI_SCHEME,
request.get_host()))

+@csrf_exempt
+@require_post
+@has_request_variables
+def api_dev_android_direct_login(request, username=REQ):

It would work If I replace the REQ with REQ() as I see these changes have
been implemented in this commit 960144a
960144a
,
So I should just replace those REQ right?


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/zulip/zulip/pull/851/files/ac7f7be901f77d111751ea635a5f0d2d2dce9b2d#r65819225,
or mute the thread
https://github.com/notifications/unsubscribe/ACnm2lHUhVsq0MkQM-_jGCbV4LDojKb1ks5qIuomgaJpZM4IrGW1
.

@csrf_exempt
def api_android_get_dev_email(request):
if not dev_auth_enabled() or settings.PRODUCTION:
# This check is probably not required, since authenticate would fail without an enabled DevAuthBackend.

This comment has been minimized.

@timabbott

timabbott Jun 5, 2016

Member

I think this is actually required, since otherwise there would be a security vulnerability where anyone on the Internet could get access to the list of users on a production Zulip server.

So probably you should remove the comment.

This comment has been minimized.

@kunall17

kunall17 Jun 5, 2016

Author Contributor

Removed this comment.

def api_android_get_dev_email(request):
if not dev_auth_enabled() or settings.PRODUCTION:
# This check is probably not required, since authenticate would fail without an enabled DevAuthBackend.
return json_error(_("Dev enviornment not enabled.", data={"reason": "dev_disabled"}, status=403))

This comment has been minimized.

@timabbott

timabbott Jun 5, 2016

Member

same spelling issue as above

This comment has been minimized.

@kunall17

kunall17 Jun 5, 2016

Author Contributor

fixed.

users = users_query.order_by('email')[0:MAX_DEV_BACKEND_USERS]
extra_context['direct_admins'] = [u.email for u in users if u.is_realm_admin]
extra_context['direct_users'] = [u.email for u in users if not u.is_realm_admin]
return json_success(extra_context)

This comment has been minimized.

@timabbott

timabbott Jun 5, 2016

Member

I think you can eliminate the extra_context variable and write this more briefly as

return json_success(dict(direct_admins=[u.email for u in users if u.is_realm_admin],
                                        direct_users=[u.email for u in users if not u.is_realm_admin])

This comment has been minimized.

@kunall17

kunall17 Jun 5, 2016

Author Contributor

Updated the code with this line, works pretty well!

url(r'^api/v1/dev_android_get_email$', 'api_android_get_dev_email'),



This comment has been minimized.

@timabbott

timabbott Jun 5, 2016

Member

Can you remove 2 of the blank lines here?

@@ -142,6 +142,14 @@
# password/pair and returns an API key.
url(r'^api/v1/fetch_api_key$', 'api_fetch_api_key'),

# This is for the signing in through the devAuthBackEnd.
url(r'^api/v1/dev_android_direct_login$', 'api_dev_android_direct_login'),

This comment has been minimized.

@timabbott

timabbott Jun 5, 2016

Member

Can you remove this blank line? It'd be great to group these two blocks together.

@timabbott

This comment has been minimized.

Copy link
Member

timabbott commented Jun 5, 2016

Cool, this is looking better! I posted a few more comments on ways to improve the code.

@kunall17 kunall17 force-pushed the kunall17:devauthloginandroid branch from ac7f7be to b6e18ec Jun 5, 2016

@kunall17

This comment has been minimized.

Copy link
Contributor Author

kunall17 commented Jun 5, 2016

@timabbott Updated the code!
Rebased with the new commits posted here as well!

MAX_DEV_BACKEND_USERS = 100
users_query = UserProfile.objects.select_related().filter(is_bot=False, is_active=True)
users = users_query.order_by('email')[0:MAX_DEV_BACKEND_USERS]
return json_success(dict(direct_admins=[u.email for u in users if u.is_realm_admin], direct_users=[u.email for u in users if not u.is_realm_admin]))

This comment has been minimized.

@timabbott

timabbott Jun 5, 2016

Member

you should line wrap direct_users to be on the next line, and aligned (using spaces, not tabs) with direct_admins

@timabbott

This comment has been minimized.

Copy link
Member

timabbott commented Jun 5, 2016

Looks a lot better! Other than a few small style things, I think the only thing that's open is this:

Finally, I'd love to see some tests for this new authentication code path; you probably want to add something similar to FetchAPIKeyTest in zerver/tests/test_auth_backends.py.

https://docs.djangoproject.com/en/1.9/topics/testing/ is helpful documentation on writing Python backend tests, and you should also be able to pattern-match off FetchAPIKeyTest pretty well for this...

@kunall17 kunall17 force-pushed the kunall17:devauthloginandroid branch from b6e18ec to bf5e66d Jun 5, 2016

@kunall17

This comment has been minimized.

Copy link
Contributor Author

kunall17 commented Jun 5, 2016

@timabbott Updated with those last two comments as well!
Now I will update the test's!

@kunall17

This comment has been minimized.

Copy link
Contributor Author

kunall17 commented Jun 13, 2016

@timabbott Sorry so much for the delay!

Well I have been facing some issues in building the tests

    def test_dev_auth_disabled(self):
        with mock.patch('zproject.backends.dev_auth_enabled', return_value=False):  
            result = self.client.post("/api/v1/dev_android_direct_login",
                                      dict(username=self.email))
            self.assert_json_error_contains(result, "Dev enviornment not enabled.", 403)

To test if the DevAuth is enabled or not, this should work I guess, but it does not even if i change the return_value=False to True nothing changes and it log's in both the cases. I tried the same thing(changing return values) for the FetchApiKeyTest.test_password_auth_disabled and it worked.

Is their something I'm doing wrong?

EDIT: If I remove the backend from settings.py it works!

@timabbott

This comment has been minimized.

Copy link
Member

timabbott commented Jun 14, 2016

Cool, does that mean you've figured out the testing issue?

@kunall17

This comment has been minimized.

Copy link
Contributor Author

kunall17 commented Jun 14, 2016

Yeah, if this line worked correctly then I could go ahead further.
with mock.patch('zproject.backends.dev_auth_enabled', return_value=False):

Whats wrong in this?

@kunall17 kunall17 force-pushed the kunall17:devauthloginandroid branch 2 times, most recently from 7441bea to 089eb59 Jun 15, 2016

@kunall17

This comment has been minimized.

Copy link
Contributor Author

kunall17 commented Jun 15, 2016

@timabbott Updated the code with Tests as well!

def test_dev_auth_disabled(self):
with mock.patch('zerver.views.dev_auth_enabled', return_value=False):
result = self.client.post("/api/v1/dev_android_direct_login",
dict(username=self.email))

This comment has been minimized.

@timabbott

timabbott Jun 15, 2016

Member

this line should be indented more

@@ -195,3 +195,31 @@ def test_deactivated_realm(self):
dict(username=self.email,
password=initial_password(self.email)))
self.assert_json_error_contains(result, "Your realm has been deactivated", 403)

class FetchAPIKeyDevAuthBackEndTest(AuthedTestCase):

This comment has been minimized.

@timabbott

timabbott Jun 15, 2016

Member

You can probably just call this AndroidDevDirectLoginTest :)

@timabbott

This comment has been minimized.

Copy link
Member

timabbott commented Jun 15, 2016

The tests look great! I posted a few small comments.

You should also add a similar test for api_android_get_dev_email, since that's also security-sensitive (it would be bad to leak the list of users on a server to the public!). Since there are fewer checks in that function, the test will be a bit smaller.

@timabbott

This comment has been minimized.

Copy link
Member

timabbott commented Jun 15, 2016

Also, can you add type annotations (the # type: comments you see in other view files) for your new functions? See http://zulip.readthedocs.io/en/latest/mypy.html for details on how to do this.

@kunall17 kunall17 force-pushed the kunall17:devauthloginandroid branch from 089eb59 to a90cb97 Jun 16, 2016

@kunall17

This comment has been minimized.

Copy link
Contributor Author

kunall17 commented Jun 16, 2016

@timabbott Updated with those comments and a new test for getDev Email's.!

# type: () -> None
with mock.patch('zerver.views.dev_auth_enabled', return_value=False):
result = self.client.get("/api/v1/dev_android_get_email")
self.assert_json_error_contains(result, "Dev environment not enabled.", 400)

This comment has been minimized.

@timabbott

timabbott Jun 16, 2016

Member

this test class should probably actually go in test_auth_backends next to the other tests.

def authenticate(self, username):
return common_get_active_user_by_email(username)
def authenticate(self, username, return_data=None):
return common_get_active_user_by_email(username, return_data=return_data)

This comment has been minimized.

@timabbott

timabbott Jun 16, 2016

Member

Why did you need to add the return_data argument here?

This comment has been minimized.

@kunall17

kunall17 Jun 17, 2016

Author Contributor

To get the inactive_user and inactive_realm data as here

@timabbott

This comment has been minimized.

Copy link
Member

timabbott commented Jun 16, 2016

Awesome, thanks @kunall17! This is looking great. I posted one question and a request to move the latest tests to another file, but otherwise this looks ready to merge.

def test_success(self):
# type: () -> None
result = self.client.get("/api/v1/dev_android_get_email")
self.assert_json_success(result)

This comment has been minimized.

@timabbott

timabbott Jun 16, 2016

Member

This should probably inspect the result to make sure it has the right format, not just check it returns success. The idea is that you want the test to detect if the API were to accidentally change, without having to discover that we broke things via the feature no longer working.

# type: () -> None
result = self.client.post("/api/v1/dev_android_direct_login",
dict(username=self.email))
self.assert_json_success(result)

This comment has been minimized.

@timabbott

timabbott Jun 16, 2016

Member

Same comment as below; this test should confirm that it actually worked (in this case, returned the correct email address and API key for self.user_profile)

@kunall17 kunall17 force-pushed the kunall17:devauthloginandroid branch from a90cb97 to fbfbcaa Jun 17, 2016

@timabbott

This comment has been minimized.

Copy link
Member

timabbott commented Jun 17, 2016

Cool, I did a few tweaks to this:

  • Squashed the test commits into the commits that introduced the features being tested
  • Renamed the endpoints to be more similar to the non-development version and not explicitly mention Android (since I expect we'll also be using these for the iOS apps in not too long!)
  • Some other small style nits
    and then merged it. Thanks @kunall17 for building this!!!

@timabbott timabbott closed this Jun 17, 2016

@kunall17

This comment has been minimized.

Copy link
Contributor Author

kunall17 commented Jun 17, 2016

@timabbott thankyou for merging this.. :)

@kunall17

This comment has been minimized.

Copy link
Contributor Author

kunall17 commented Jun 17, 2016

BTW @timabbott can you help me with this, not related to this PR,
I was trying to get my local vagrant zulip server to run on my physical android device for testing purposes, it worked when I did port forwarding via USB but I couldn't debug apps by that so I was trying if I could connect these both via connecting through same router can you suggest a way to achieve this?
The server works on the emulator on http://10.0.3.2:9991/ And localhost:9991 on the host linux machine.

@timabbott

This comment has been minimized.

Copy link
Member

timabbott commented Jun 17, 2016

Hmm, I haven't tried doing this before; @niftynei may have done this before though...

@niftynei

This comment has been minimized.

Copy link

niftynei commented Jun 22, 2016

@kunall17 I got this working on OSX by changing the forwarded_port host_ip to 0.0.0.0. I'm not sure exactly why this works, but it makes the Zulip dev instance available on the local network at my laptop's IP address, which I can then access from a free-standing Android device.

This is the line in the Vagrantfile that you'll need to modify: https://github.com/zulip/zulip/blob/master/Vagrantfile#L16

@timabbott

This comment has been minimized.

Copy link
Member

timabbott commented Jun 22, 2016

It'd be awesome to put instructions for this into the README.md for the Android app.

Also, I think it'd be great to add to the /.zulip-vagrant-config code in Vagrantfile support for an option that does this, so that one doesn't need to patch the backend to make it work.

@kunall17

This comment has been minimized.

Copy link
Contributor Author

kunall17 commented Jun 22, 2016

@niftynei thankyou so much, works flawlessly! 👍 :)

@niftynei

This comment has been minimized.

Copy link

niftynei commented Jun 23, 2016

@timabbott I'm not sure what you mean by adding it to /.zulip-vagrant-config. Can you point me to the right place?

@timabbott

This comment has been minimized.

Copy link
Member

timabbott commented Jun 23, 2016

We parse ~/.zulip-vagrant-config in Vagrantfile here: https://github.com/zulip/zulip/blob/master/Vagrantfile#L21

The port forwarding config is here, just a few lines above: https://github.com/zulip/zulip/blob/master/Vagrantfile#L16

So the idea was to add support for having this new option set in that config file as well.

(Vagrantfile is a ruby script, which may be relevant for figuring out how to modify it)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.