Rest auth #79

rubengrill · 2018-07-22T09:16:54Z

This PR adds a rest api (#71) and support for Django 2.0.
I also dropped support for Django < 1.11 to not waste time implementing the rest api.
In #52 it is briefly discussed to follow Django's recommendation here, to support what Django supports.

I did some backward incompatible changes, I will document those changes later in this PR to discuss if those are acceptable and if they are, what the strategy should be to handle those changes (new major version, deprecation warnings...).

In "Refactor forms" some changes are made to allow easier customization of the sent emails, following the design of Django's auth views/forms.

nopassword/backends/base.py

rubengrill · 2018-07-22T09:52:56Z

nopassword/backends/email.py


-    def send_login_code(self, code, secure=False, host=None, **kwargs):
-        subject = getattr(settings, 'NOPASSWORD_LOGIN_EMAIL_SUBJECT', _('Login code'))


Backward incompatible: Instead of a custom setting, we use a template instead (like django auth forms/views)

Why do we want to use templates for this? Subjects can only be plain text in one line, so this feels a bit like we introduce a breaking change in configuration that is not really giving us anything?

It would be consistent with django default auth forms: https://github.com/django/django/blob/2.1/django/contrib/auth/forms.py#L239
Also it would be consisent to also use a template for the subject, not only for the message itself.
A use case I could think of where it might be desired is to have the site name in the subject, e.g. [example.com] Login with one-time password, where the current site is dependent on the request (sites framework).

Sounds good to me

nopassword/backends/email.py

rubengrill · 2018-07-22T10:14:34Z

nopassword/urls.py

@@ -5,23 +5,10 @@

 urlpatterns = [
    url(
-        r'^login/$',


Backward incompatible: Renamed urls to reflect view name changes.

nopassword/views.py

tests/urls.py

rubengrill · 2018-07-23T14:14:54Z

@relekang Are the breaking changes acceptable in your opinion? If so, how should the they be handled?
I see three possible strategies:

Just document them and create a new major version
Change code to be compatible and add warnings
Don't do backward incompatible changes

relekang · 2018-07-23T20:29:22Z

Thanks @rubengrill, api endpoints is a good addition. I am leaving for vacation today so will review this in about two weeks, the pr is quite large so I won't manage it before I leave. Thank you for your patience ✌️

sebslomski · 2018-07-26T07:32:41Z

@rubengrill Thanks, I'm already using your PR for local development.
@relekang Really looking forward to having this on PyPi!

relekang

@rubengrill I finally got some time to look this over. Over all it looks great 👏 I left some questions and comments. Before we can merge this all the changes that have done, on configuration and usage needs to be reflected in the documentation.

Could you also reword the commits to use conventional commits?

nopassword/backends/base.py

relekang · 2018-08-11T18:21:13Z

nopassword/backends/email.py


-    def send_login_code(self, code, secure=False, host=None, **kwargs):
-        subject = getattr(settings, 'NOPASSWORD_LOGIN_EMAIL_SUBJECT', _('Login code'))


Why do we want to use templates for this? Subjects can only be plain text in one line, so this feels a bit like we introduce a breaking change in configuration that is not really giving us anything?

relekang · 2018-08-11T18:26:52Z

nopassword/backends/email.py

-        msg = EmailMultiAlternatives(subject, text_content, from_email, to_email)
-        msg.attach_alternative(html_content, 'text/html')
-        msg.send()
+        if self.html_template_name is not None:


What is the benefit of this change? In my opinion we should strive for an easier solution to adopt for the user than needing to extend the class. It would be perfect if the template existed then we would send the html version.

relekang · 2018-08-11T18:31:46Z

nopassword/templates/registration/login_code_request_email.html

@@ -0,0 +1,14 @@
+{% load i18n %}{% autoescape off %}


This seems like the text based email? Why does it have an html file ending?

yes that's nonsense, I will change to be txt

nopassword/utils.py

relekang · 2018-08-11T18:34:14Z

requirements.txt

@@ -1,4 +1,4 @@
 twilio==5.4.0
 mock>=1.0
+djangorestframework<=4.0


Shouldn't this be the same as in setup.py?

Yes, that's right

tests/settings.py

rubengrill

Hi @relekang , Thanks for your comments!

Just to be sure:

Before we can merge this all the changes that have done, on configuration and usage needs to be reflected in the documentation.

This means documenting the breaking changes and different usage is enough, it will be a new major version and there doesn't need to be code for backward compatibility?

Please check my comments to discuss the breaking changes. Afterwards I will start with integrating the feedback, changing the documentation and rewording the commits.

Thanks again for your time and feedback! :)

nopassword/backends/base.py

rubengrill · 2018-08-19T21:13:54Z

nopassword/backends/email.py


-    def send_login_code(self, code, secure=False, host=None, **kwargs):
-        subject = getattr(settings, 'NOPASSWORD_LOGIN_EMAIL_SUBJECT', _('Login code'))


It would be consistent with django default auth forms: https://github.com/django/django/blob/2.1/django/contrib/auth/forms.py#L239
Also it would be consisent to also use a template for the subject, not only for the message itself.
A use case I could think of where it might be desired is to have the site name in the subject, e.g. [example.com] Login with one-time password, where the current site is dependent on the request (sites framework).

rubengrill · 2018-08-19T21:33:23Z

nopassword/backends/email.py

-        msg = EmailMultiAlternatives(subject, text_content, from_email, to_email)
-        msg.attach_alternative(html_content, 'text/html')
-        msg.send()
+        if self.html_template_name is not None:


The idea was to be consistent with how django default auth form/view handles that:
https://github.com/django/django/blob/2.1/django/contrib/auth/views.py#L211
https://github.com/django/django/blob/2.1/django/contrib/auth/forms.py#L268

I don't exactly know why django does it this way but I can imagine that providing a default html email template would just be too opinonated, as you want to use html templates to include headers, logos, tables, containers, ... so any default template will have to be changed by the vendor either way.

But you are absolutely right, providing a custom backend just to use a html template seems to be too much work (I guess same applies to the django views/forms here...).

I just saw that allauth has a neat approach:
It also doesn't provide a default html template, but at least it tries to render the html template.
When there is a TemplateDoesNotExist exception, it ignores this template.

Should we use this approach, to set html_template_name with a default, but we don't provide that template and wrap rendering in a try/catch?

rubengrill · 2018-08-19T21:36:32Z

nopassword/templates/registration/login_code_request_email.html

@@ -0,0 +1,14 @@
+{% load i18n %}{% autoescape off %}


yes that's nonsense, I will change to be txt

nopassword/views.py

rubengrill · 2018-08-19T22:20:14Z

requirements.txt

@@ -1,4 +1,4 @@
 twilio==5.4.0
 mock>=1.0
+djangorestframework<=4.0


Yes, that's right

tests/settings.py

relekang · 2018-08-22T12:09:26Z

@rubengrill I have a pretty busy week, but will look into all your comments on the end of the week.

This means documenting the breaking changes and different usage is enough, it will be a new major version and there doesn't need to be code for backward compatibility?

Yes, and all the breaking changes that I did not comment on in the first review I agree with fully 👍

rubengrill · 2018-08-28T18:11:49Z

Hi @relekang, great, then I'll wait for your final comments to start with the fixes.
Looking forward to the feedback.

relekang · 2018-08-29T07:13:30Z

I think I commented on all your questions now 😊

rubengrill · 2018-09-01T21:52:19Z

All fixes have been addressed, except this:

Update documentation (will follow soon)
Reword commits (will follow soon)
Tests rest api: I have trouble here building a separate environment, as then we would have 2n environments. Also the tests should somehow be separated as the rest tests shouldn't run on the regular environments, and running the regular tests again in the rest environments doesn't make much sense.

I also renamed the views/forms, I'm very sorry for this confusion.
When testing in an example project I realized that the old naming didn't play well with the default of LOGIN_URL = '/accounts/login/', where the user was asked for the login code right after being redirected from a view that requires login...

relekang · 2018-09-03T07:33:21Z

The updated changes looks good to me, great work 😊

rubengrill · 2018-09-06T21:20:18Z

Docs are updated now, last step is rewording commits to confirm to conventional commits, I'll do this after the docs are approved as well, as I don't know how rebasing will affect the comments in this PR and might make review harder.

I will also add some new issues we should address before releasing a new version.
We wanted to integrate this PR already into a project and at some points integration was more difficult than expected.

relekang · 2018-09-07T08:44:56Z

The changes looks good 👍

Missed this earlier why did the test setup change to sqlite?

panosangelopoulos · 2018-09-10T15:04:03Z

nopassword/backends/base.py

-    def verify_user(self, user):
-        return user.is_active
+            timeout = getattr(settings, 'NOPASSWORD_LOGIN_CODE_TIMEOUT', 900)
+            timestamp = datetime.now() - timedelta(seconds=timeout)


Please use timezone provided by django instead of datetime.

rubengrill · 2018-09-12T09:02:10Z

I had some problems to get the tests running with the default configuration, some local problems with my postgresql database and some warnings of an outdated pip dependency.
The environment variable for using sqlite somehow didn't work, I had to hardcode using the sqlite database to get everything running.
That's why I thought it might be good for contributors to be able to just start working without having connections problems in the beginning.
Should this be reverted?

An authentication backend should return a user or None. Returning a LoginCode does not comply with that spec. Also, for some authentication backends it is a problem if no password is given (e.g. allauth backend). Checking if a user exists and creating a login code in the form should be more stable this way.

Makes more sense from outside of nopassword, as requesting a login code is the entry point of the login process. This way the default value of LOGIN_URL (`/accounts/login/`) is correctly opening the login code request page.

rubengrill · 2018-09-12T10:06:31Z

Renamed the first commits to confirm to conventional commits.

relekang · 2018-09-12T21:36:56Z

I think we can keep the sqlite for now. It seems CI is not working, so I will run all tests locally tomorrow and merge if it is all green. Great work @rubengrill 👏

rubengrill · 2018-09-13T12:22:51Z

Awesome, looking forward to see it merged :)

relekang · 2018-09-14T08:19:15Z

Thanks @rubengrill 👍

rubengrill commented Jul 22, 2018

View reviewed changes

relekang suggested changes Aug 11, 2018

View reviewed changes

rubengrill commented Aug 19, 2018

View reviewed changes

rubengrill mentioned this pull request Sep 6, 2018

REST login_url for external frontend #81

Open

panosangelopoulos reviewed Sep 10, 2018

View reviewed changes

rubengrill added 14 commits September 12, 2018 12:01

fix: Fix isort error

a44a83c

chore: Support Django 2.0

8069f60

feat: Add rest views

74d2c06

chore: Remove support for Django < 1.11

57dd6c0

feat: Use username field metadata for form/serializer fields

3ff83a4

feat: Raise error when sending code without backend

79485df

feat: Add admin

579d66e

chore: Refactor forms

c235e37

chore: Use sqlite3 in tests

74a7982

fix: Change login_code_request_email template extension

ac994dd

feat: Set default html_template_name in EmailBackend

0c3b736

fix: Rename sms template

e6bc59e

feat: Add NOPASSWORD_LOGIN_ON_GET setting

1eb1798

rubengrill added 7 commits September 12, 2018 12:04

chore: Change djangorestframework version in requirements

1ae9545

chore: Add django 2.1 as tox env

aa277a8

fix: Rename LoginCodeRequestView to LoginView

38ea223

Makes more sense from outside of nopassword, as requesting a login code is the entry point of the login process. This way the default value of LOGIN_URL (`/accounts/login/`) is correctly opening the login code request page.

feat: Support next in rest

3168d2f

docs: Update README

c378fff

docs: Update docs

88f44b9

docs: Add changelog

3ebc7a2

rubengrill force-pushed the rest-auth branch from 09deab8 to 3ebc7a2 Compare September 12, 2018 10:04

fix: Use timezone.now() instead of datetime.now()

07693c0

relekang approved these changes Sep 12, 2018

View reviewed changes

relekang merged commit cf22def into relekang:master Sep 14, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rest auth #79

Rest auth #79

rubengrill commented Jul 22, 2018 •

edited

Loading

rubengrill Jul 22, 2018

relekang Aug 11, 2018

rubengrill Aug 19, 2018

relekang Aug 29, 2018

rubengrill Jul 22, 2018

rubengrill commented Jul 23, 2018 •

edited

Loading

relekang commented Jul 23, 2018

sebslomski commented Jul 26, 2018

relekang left a comment

relekang Aug 11, 2018

relekang Aug 11, 2018

relekang Aug 11, 2018

rubengrill Aug 19, 2018

relekang Aug 11, 2018

rubengrill Aug 19, 2018

rubengrill left a comment

rubengrill Aug 19, 2018

rubengrill Aug 19, 2018

rubengrill Aug 19, 2018

rubengrill Aug 19, 2018

relekang commented Aug 22, 2018

rubengrill commented Aug 28, 2018

relekang commented Aug 29, 2018

rubengrill commented Sep 1, 2018

relekang commented Sep 3, 2018

rubengrill commented Sep 6, 2018

relekang commented Sep 7, 2018

panosangelopoulos Sep 10, 2018

rubengrill commented Sep 12, 2018

rubengrill commented Sep 12, 2018

relekang commented Sep 12, 2018

rubengrill commented Sep 13, 2018

relekang commented Sep 14, 2018


		def send_login_code(self, code, secure=False, host=None, **kwargs):
		subject = getattr(settings, 'NOPASSWORD_LOGIN_EMAIL_SUBJECT', _('Login code'))

Rest auth #79

Rest auth #79

Conversation

rubengrill commented Jul 22, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rubengrill commented Jul 23, 2018 • edited Loading

relekang commented Jul 23, 2018

sebslomski commented Jul 26, 2018

relekang left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rubengrill left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

relekang commented Aug 22, 2018

rubengrill commented Aug 28, 2018

relekang commented Aug 29, 2018

rubengrill commented Sep 1, 2018

relekang commented Sep 3, 2018

rubengrill commented Sep 6, 2018

relekang commented Sep 7, 2018

Choose a reason for hiding this comment

rubengrill commented Sep 12, 2018

rubengrill commented Sep 12, 2018

relekang commented Sep 12, 2018

rubengrill commented Sep 13, 2018

relekang commented Sep 14, 2018

rubengrill commented Jul 22, 2018 •

edited

Loading

rubengrill commented Jul 23, 2018 •

edited

Loading