From 9a9a9e8212b75dc7580921e90fba2ae96345cd84 Mon Sep 17 00:00:00 2001 From: Wes Appler <145372368+wes-otf@users.noreply.github.com> Date: Fri, 22 Mar 2024 03:50:17 -0400 Subject: [PATCH] Removed registration form, cleaned up login flow (#3821) Fixes #3813. Main changes: - [x] Got rid of the registration form in favor of using the `/auth/` view. Having two places where the user can potentially register is confusing. - [x] Cleaned up different aspects of the login flow that were mentioned in #3813, like inconsistency in terminology (`Login` vs `Log in`), removal of buttons where they don't need to be, etc. - [x] Added a button on the password-ed login to bring the user back to the passwordless log in/register view. - [x] Have buttons in column rather than row on *both* mobile & desktop view and made them the same width (`18 rem`). - Should we center the login page for desktop? This was done as it felt awkward to have everything on the left side while the buttons crept to the center - [x] Added icons to `Log in with <ORG> email` & `Log in or register via email` for consistency - [x] Moved login buttons to their own templates as some were being reused - [x] Added missing translation blocks --- hypha/apply/users/forms.py | 2 +- hypha/apply/users/services.py | 4 +- hypha/apply/users/templates/users/login.html | 37 +++++----- .../templates/users/password_reset/form.html | 8 +-- .../users/passwordless_login_signup.html | 29 +++----- .../apply/users/templates/users/register.html | 45 ------------ hypha/apply/users/tests/test_registration.py | 71 ------------------- hypha/apply/users/urls.py | 6 -- hypha/apply/users/views.py | 64 +---------------- hypha/static_src/sass/components/_link.scss | 15 ++++ hypha/templates/base-apply.html | 11 +-- hypha/templates/includes/login_button.html | 4 +- .../templates/includes/org_login_button.html | 8 +++ .../includes/password_login_button.html | 8 +++ .../includes/passwordless_login_button.html | 8 +++ hypha/templates/includes/register_button.html | 4 -- 16 files changed, 76 insertions(+), 248 deletions(-) delete mode 100644 hypha/apply/users/templates/users/register.html delete mode 100644 hypha/apply/users/tests/test_registration.py create mode 100644 hypha/templates/includes/org_login_button.html create mode 100644 hypha/templates/includes/password_login_button.html create mode 100644 hypha/templates/includes/passwordless_login_button.html delete mode 100644 hypha/templates/includes/register_button.html diff --git a/hypha/apply/users/forms.py b/hypha/apply/users/forms.py index ef8005b26..bbe5356a4 100644 --- a/hypha/apply/users/forms.py +++ b/hypha/apply/users/forms.py @@ -32,7 +32,7 @@ class PasswordlessAuthForm(forms.Form): """ email = forms.EmailField( - label=_("Email Address"), + label=_("Email address"), required=True, max_length=254, widget=forms.EmailInput(attrs={"autofocus": True, "autocomplete": "email"}), diff --git a/hypha/apply/users/services.py b/hypha/apply/users/services.py index 8762aa00c..70d33ea32 100644 --- a/hypha/apply/users/services.py +++ b/hypha/apply/users/services.py @@ -63,7 +63,7 @@ class PasswordlessAuthService: def send_email_no_account_found(self, to): context = self.get_email_context() - subject = "Login attempt at {org_long_name}".format(**context) + subject = "Log in attempt at {org_long_name}".format(**context) # Force subject to a single line to avoid header-injection issues. subject = "".join(subject.splitlines()) @@ -91,7 +91,7 @@ class PasswordlessAuthService: } ) - subject = "Login to {username} at {org_long_name}".format(**context) + subject = "Log in to {username} at {org_long_name}".format(**context) # Force subject to a single line to avoid header-injection issues. subject = "".join(subject.splitlines()) diff --git a/hypha/apply/users/templates/users/login.html b/hypha/apply/users/templates/users/login.html index a9f8eed60..66e473638 100644 --- a/hypha/apply/users/templates/users/login.html +++ b/hypha/apply/users/templates/users/login.html @@ -1,11 +1,11 @@ {% extends "base-apply.html" %} -{% load i18n wagtailcore_tags %} +{% load i18n wagtailcore_tags heroicons %} -{% block title %}{% trans "Login" %}{% endblock %} +{% block title %}{% trans "Log in" %}{% endblock %} {% block body_class %}bg-white{% endblock %} {% block content %} - <div class="w-full grid bg-white mt-5 md:py-4"> + <div class="max-w-2xl grid bg-white mt-5 md:py-4"> <section> <div class="px-4 pt-4"> @@ -23,10 +23,10 @@ {% elif wizard.steps.current == 'backup' %} <h2 class="text-2xl">{% trans "Two factor verification" %}</h2> <p> - {% blocktrans trimmed %}Please enter one of the backup codes to login to your account.{% endblocktrans %} + {% blocktrans trimmed %}Please enter one of the backup codes to log in to your account.{% endblocktrans %} </p> <p class="text-sm mb-4 text-fg-muted"> - Those codes were generated for you during 2FA setup to print or keep safe in a password manager. + {% blocktrans trimmed %}Those codes were generated for you during 2FA setup to print or keep safe in a password manager.{% endblocktrans %} </p> {% endif %} @@ -61,24 +61,21 @@ {% endif %} <div class="form__group max-w-sm flex items-center justify-between gap-4"> - <button class="link link--button link--button-secondary" type="submit">{% trans "Log in" %}</button> - - {% if ENABLE_PUBLIC_SIGNUP %} - <a class="hover:opacity-75" href="{% url 'users:register' %}{% if redirect_url %}?next={{ redirect_url }}{% endif %}" hx-boost="true"> {% trans "Create account" %}</a> - {% endif %} + <button class="link link--button link--button--login" type="submit">{% trans "Log in" %}</button> </div> - {% if GOOGLE_OAUTH2 %} - <div class="flex items-center justify-start relative"> - <hr class="inline w-32 h-px my-6 bg-gray-300 border-0"> - <span class="px-3 text-gray-400 font-medium">{% trans "or" %}</span> - <hr class="inline w-32 h-px my-6 bg-gray-300 border-0"> - </div> + <div class="flex items-center justify-center min-[465px]:justify-start relative mb-4"> + <hr class="inline w-32 h-px my-6 bg-gray-300 border-0"> + <span class="px-3 text-gray-400 font-medium">{% trans "or" %}</span> + <hr class="inline w-32 h-px my-6 bg-gray-300 border-0"> + </div> - <div class=""> - <a class="link link--button link--button-tertiary" href="{% url "social:begin" "google-oauth2" %}{% if next %}?next={{ next }}{% endif %}">{% blocktrans %}Log in with your {{ ORG_SHORT_NAME }} email{% endblocktrans %}</a> - </div> - {% endif %} + <section class="space-y-0.5"> + {% if GOOGLE_OAUTH2 %} + {% include "includes/org_login_button.html" %} + {% endif %} + {% include "includes/passwordless_login_button.html" %} + </section {% else %} <div class="form__group"> diff --git a/hypha/apply/users/templates/users/password_reset/form.html b/hypha/apply/users/templates/users/password_reset/form.html index 23fdbb1fc..82ef552a6 100644 --- a/hypha/apply/users/templates/users/password_reset/form.html +++ b/hypha/apply/users/templates/users/password_reset/form.html @@ -28,14 +28,8 @@ <p>{{ form.email.label_tag }}</p> <p>{{ form.email }}</p> - <button class="btn btn-primary" type="submit">{% trans "Send reset email" %}</button> + <button class="link link--button link--button--login" type="submit">{% trans "Send reset email" %}</button> </form> - - <div class="mt-4"> - <a href="{% url 'users:login' %}{% if redirect_url %}?next={{ redirect_url }}{% endif %}" hx-boost="true"> - {% trans "Log in" %} - </a> - </div> </div> </section> {% endblock %} diff --git a/hypha/apply/users/templates/users/passwordless_login_signup.html b/hypha/apply/users/templates/users/passwordless_login_signup.html index a8af8c41a..40c99a60a 100644 --- a/hypha/apply/users/templates/users/passwordless_login_signup.html +++ b/hypha/apply/users/templates/users/passwordless_login_signup.html @@ -1,7 +1,9 @@ {% extends base_template %} {% load i18n wagtailcore_tags heroicons %} -{% block title %}{% trans "Login or Signup" %}{% endblock %} +{% block title %} + {% trans "Log in" %}{% if ENABLE_PUBLIC_SIGNUP %} {% trans "or" %} {% trans "Sign up" %}{% endif %} +{% endblock %} {% block content %} <div class="max-w-2xl bg-white mt-5 md:py-4"> @@ -16,9 +18,9 @@ <h2 class="text-2xl"> {% if ENABLE_PUBLIC_SIGNUP %} - {% blocktrans %}Login or signup to {{ ORG_SHORT_NAME }}{% endblocktrans %} + {% blocktrans %}Log in or signup to {{ ORG_SHORT_NAME }}{% endblocktrans %} {% else %} - {% blocktrans %}Login to {{ ORG_SHORT_NAME }}{% endblocktrans %} + {% blocktrans %}Log in to {{ ORG_SHORT_NAME }}{% endblocktrans %} {% endif %} </h2> @@ -42,32 +44,21 @@ {% endif %} <div class="form__group"> - <button class="link link--button link--button-secondary" type="submit">{% trans "Next" %}</button> + <button class="link link--button link--button--login" type="submit">{% trans "Next" %}</button> </div> - <div class="flex items-center justify-start relative mb-4"> + <div class="flex items-center justify-center min-[465px]:justify-start relative mb-4"> <hr class="inline w-32 h-px my-6 bg-gray-300 border-0"> <span class="px-3 text-gray-400 font-medium">{% trans "or" %}</span> <hr class="inline w-32 h-px my-6 bg-gray-300 border-0"> </div> - <section> + <section class="space-y-0.5"> {% if GOOGLE_OAUTH2 %} - <a - class="link link--button link--button-tertiary" - href="{% url "social:begin" "google-oauth2" %}{% if next %}?next={{ next }}{% endif %}" - > - {% blocktrans %}Log in with your {{ ORG_SHORT_NAME }} email{% endblocktrans %} - </a> + {% include "includes/org_login_button.html" %} {% endif %} - <a - class="link link--button link--button-tertiary" - href="{% url 'users:login' %}{% if next %}?next={{next}}{% endif %}" - > - {% heroicon_mini "key" size=18 class="inline align-text-bottom me-1" aria_hidden=true %} - {% trans "Login with Password" %} - </a> + {% include "includes/password_login_button.html" %} </section> </form> </section> diff --git a/hypha/apply/users/templates/users/register.html b/hypha/apply/users/templates/users/register.html deleted file mode 100644 index 1b9b08de0..000000000 --- a/hypha/apply/users/templates/users/register.html +++ /dev/null @@ -1,45 +0,0 @@ -{% extends "base-apply.html" %} -{% load i18n wagtailcore_tags %} - -{% block title %}{% trans "Register" %}{% endblock %} -{% block body_class %}bg-white{% endblock %} - -{% block content %} - <div class="w-full grid md:mt-5 md:py-4"> - <section> - <div class="px-4 pt-4"> - <form class="form" method="post" action="{% url 'users:register' %}"> - {% csrf_token %} - {% if redirect_url %} - <input type="hidden" value="{{ redirect_url }}" name="next"> - {% endif %} - <h2 class="text-2xl">{% trans "Create your account" %}</h2> - - {% if form.non_field_errors %} - <div class="wrapper wrapper--error">{{ form.non_field_errors.as_text }}</div> - {% endif %} - - {% for field in form %} - {%if field.id_for_label != 'id_is_superuser' and field.label != 'Groups' %} - {% include "forms/includes/field.html" %} - {%endif%} - {% endfor %} - <div class="form__group"> - <button class="button button--primary" type="submit">{% trans "Submit" %}</button> - </div> - </form> - <p class="mt-4"> - {% trans "Already have an account?" %}<a href="{% url 'users:login' %}{% if redirect_url %}?next={{ redirect_url }}{% endif %}" hx-boost="true"> {% trans "Log in" %}</a> - </p> - </div> - </section> - - {% if settings.users.AuthSettings.register_extra_text %} - <section> - <div class="px-4 my-4"> - {{ settings.users.AuthSettings.register_extra_text|richtext}} - </div> - </section> - {% endif %} - </div> -{% endblock %} diff --git a/hypha/apply/users/tests/test_registration.py b/hypha/apply/users/tests/test_registration.py deleted file mode 100644 index 510a2708e..000000000 --- a/hypha/apply/users/tests/test_registration.py +++ /dev/null @@ -1,71 +0,0 @@ -from django.conf import settings -from django.core import mail -from django.test import TestCase, override_settings -from django.urls import reverse - -from hypha.apply.funds.tests.factories import FundTypeFactory -from hypha.apply.utils.testing import make_request - - -class TestRegistration(TestCase): - @override_settings(ENABLE_PUBLIC_SIGNUP=False) - def test_registration_enabled_has_no_link(self): - response = self.client.get("/", follow=True) - self.assertNotContains(response, reverse("users:register")) - - @override_settings(ENABLE_PUBLIC_SIGNUP=True) - def test_registration_enabled_has_link(self): - response = self.client.get("/", follow=True) - self.assertContains(response, reverse("users:register")) - - @override_settings(ENABLE_PUBLIC_SIGNUP=True) - def test_registration(self): - response = self.client.post( - reverse("users:register"), - data={ - "email": "test@test.com", - "first_name": "Not used - see full_name", - "last_name": "Not used - see full_name", - }, - secure=True, - ) - assert len(mail.outbox) == 1 - assert "Activate your account on the" in mail.outbox[0].body - - assert response.status_code == 302 - assert reverse("users:register-success") in response.url - - @override_settings(ENABLE_PUBLIC_SIGNUP=True) - def test_duplicate_registration_fails(self): - response = self.client.post( - reverse("users:register"), - data={ - "email": "test@test.com", - "first_name": "Not used - see full_name", - "last_name": "Not used - see full_name", - }, - secure=True, - ) - mail.outbox.clear() - - response = self.client.post( - reverse("users:register"), - data={ - "email": "test@test.com", - "first_name": "Not used - see full_name", - "last_name": "Not used - see full_name", - }, - secure=True, - ) - - assert len(mail.outbox) == 0 - self.assertContains(response, "A user with that email already exists") - - @override_settings(FORCE_LOGIN_FOR_APPLICATION=True, ENABLE_PUBLIC_SIGNUP=False) - def test_force_login(self): - fund = FundTypeFactory() - response = fund.serve( - make_request(None, {}, method="get", site=fund.get_site()) - ) - assert response.status_code == 302 - assert response.url == reverse(settings.LOGIN_URL) + "?next=/" diff --git a/hypha/apply/users/urls.py b/hypha/apply/users/urls.py index 3759fb820..32afa399a 100644 --- a/hypha/apply/users/urls.py +++ b/hypha/apply/users/urls.py @@ -17,8 +17,6 @@ from .views import ( PasswordlessSignupView, PasswordResetConfirmView, PasswordResetView, - RegisterView, - RegistrationSuccessView, TWOFAAdminDisableView, TWOFADisableView, TWOFASetupView, @@ -39,10 +37,6 @@ public_urlpatterns = [ ), path("login/", LoginView.as_view(), name="login"), path("logout/", auth_views.LogoutView.as_view(next_page="/"), name="logout"), - path("register/", RegisterView.as_view(), name="register"), - path( - "register-success/", RegistrationSuccessView.as_view(), name="register-success" - ), ] account_urls = [ diff --git a/hypha/apply/users/views.py b/hypha/apply/users/views.py index 041fcfc99..430643ced 100644 --- a/hypha/apply/users/views.py +++ b/hypha/apply/users/views.py @@ -18,7 +18,7 @@ from django.contrib.sites.shortcuts import get_current_site from django.core.exceptions import PermissionDenied from django.core.signing import TimestampSigner, dumps, loads from django.http import HttpResponse, HttpResponseRedirect -from django.shortcuts import Http404, get_object_or_404, redirect, render, resolve_url +from django.shortcuts import Http404, get_object_or_404, redirect, render from django.template.loader import render_to_string from django.template.response import TemplateResponse from django.urls import reverse, reverse_lazy @@ -30,7 +30,7 @@ from django.utils.translation import gettext_lazy as _ from django.views.decorators.cache import never_cache from django.views.decorators.debug import sensitive_post_parameters from django.views.generic import UpdateView -from django.views.generic.base import TemplateView, View +from django.views.generic.base import TemplateView from django.views.generic.edit import FormView from django_htmx.http import HttpResponseClientRedirect from django_otp import devices_for_user @@ -56,7 +56,6 @@ from .decorators import require_oauth_whitelist from .forms import ( BecomeUserForm, CustomAuthenticationForm, - CustomUserCreationForm, Disable2FAConfirmationForm, PasswordlessAuthForm, ProfileForm, @@ -74,65 +73,6 @@ from .utils import ( User = get_user_model() -@method_decorator( - ratelimit(key="ip", rate=settings.DEFAULT_RATE_LIMIT, method="POST"), - name="dispatch", -) -class RegisterView(View): - redirect_field_name = "next" - form = CustomUserCreationForm - - def get(self, request): - # We keep /register in the urls in order to test (where we turn on/off - # the setting per test), but when disabled, we want to pretend it doesn't - # exist va 404 - if not settings.ENABLE_PUBLIC_SIGNUP: - raise Http404 - - if request.user.is_authenticated: - return redirect(settings.LOGIN_REDIRECT_URL) - - ctx = { - "form": self.form(register_view=True), - "redirect_url": get_redirect_url(request, self.redirect_field_name), - } - return render(request, "users/register.html", ctx) - - def post(self, request): - # See comment in get() above about doing this here rather than in urls - if not settings.ENABLE_PUBLIC_SIGNUP: - raise Http404 - - form = self.form(register_view=True, data=request.POST) - context = {} - if form.is_valid(): - # If using wagtail password management - if "password1" in form.cleaned_data: - context["password"] = form.cleaned_data["password1"] - - site = Site.find_for_request(self.request) - user, created = User.objects.get_or_create_and_notify( - email=form.cleaned_data["email"], - site=site, - redirect_url=get_redirect_url(request, self.redirect_field_name), - defaults={ - "full_name": form.cleaned_data["full_name"], - }, - **context, - ) - if created: - params = {"name": user.full_name, "email": user.email} - # redirect to success page with params as query params - return HttpResponseRedirect( - resolve_url("users:register-success") + "?" + urlencode(params) - ) - return render(request, "users/register.html", {"form": form}) - - -class RegistrationSuccessView(TemplateView): - template_name = "users/register-success.html" - - @method_decorator( ratelimit(key="ip", rate=settings.DEFAULT_RATE_LIMIT, method="POST"), name="dispatch", diff --git a/hypha/static_src/sass/components/_link.scss b/hypha/static_src/sass/components/_link.scss index 9624fab83..591eb1124 100644 --- a/hypha/static_src/sass/components/_link.scss +++ b/hypha/static_src/sass/components/_link.scss @@ -9,9 +9,24 @@ @include button($color--light-blue, $color--dark-blue); display: inline-block; + // The custom width of 465px allow the buttons to scale with the input field + $input-box-max-width: 465px; + &--narrow { @include button--narrow; } + + &--login { + /* stylelint-disable-next-line media-query-no-invalid */ + @media (min-width: $input-box-max-width) { + width: 20rem; + } + } + + /* stylelint-disable-next-line media-query-no-invalid */ + @media (max-width: $input-box-max-width) { + width: 100%; + } } &--underlined { diff --git a/hypha/templates/base-apply.html b/hypha/templates/base-apply.html index 89c4cc789..0c393448e 100644 --- a/hypha/templates/base-apply.html +++ b/hypha/templates/base-apply.html @@ -48,12 +48,9 @@ {% heroicon_outline "bars-3" size=32 stroke_width=2 class="inline" aria_hidden="true" %} </button> {% else %} - {% if request.path != '/auth/' %} + {% if request.path != '/auth/' and request.path != '/login/' %} {% include "includes/login_button.html" %} {% endif %} - {% if ENABLE_PUBLIC_SIGNUP and request.path == '/auth/' %} - {% include "includes/register_button.html" %} - {% endif %} {% endif %} </div> @@ -121,7 +118,7 @@ </div> {% endif %} - {% if request.path != '/auth/' %} + {% if request.path != '/auth/' and request.path != '/login/' %} {% include "includes/login_button.html" %} {% endif %} @@ -129,10 +126,6 @@ <a href="{% url 'users:logout' %}" class="button button--transparent"> {% trans "Log out" %} </a> - {% else %} - {% if ENABLE_PUBLIC_SIGNUP and request.path != '/register/' %} - {% include "includes/register_button.html" %} - {% endif %} {% endif %} </div> </div> diff --git a/hypha/templates/includes/login_button.html b/hypha/templates/includes/login_button.html index d5157d8dc..b440bef38 100644 --- a/hypha/templates/includes/login_button.html +++ b/hypha/templates/includes/login_button.html @@ -8,7 +8,7 @@ > {% heroicon_micro "user" class="inline align-text-bottom w-4 h-4 me-1" aria_hidden=true %} - My {{ ORG_SHORT_NAME }} + {% blocktrans %}My {{ ORG_SHORT_NAME }}{% endblocktrans %} </a> {% else %} <a @@ -26,6 +26,6 @@ href="{{ APPLY_SITE.root_url }}{% url 'users:passwordless_login_signup' %}{% if redirect_url %}?next={{ redirect_url }}{% endif %}" > {% heroicon_micro "user" class="inline align-text-bottom w-4 h-4 me-1" aria_hidden=true %} - {% trans "Login" %} + {% trans "Log in" %} {% if ENABLE_PUBLIC_SIGNUP %} {% trans " or Sign up" %} {% endif %} </a> {% endif %} diff --git a/hypha/templates/includes/org_login_button.html b/hypha/templates/includes/org_login_button.html new file mode 100644 index 000000000..e40ff3b36 --- /dev/null +++ b/hypha/templates/includes/org_login_button.html @@ -0,0 +1,8 @@ +{% load i18n heroicons %} +<a + class="link link--button link--button--login" + href="{% url "social:begin" "google-oauth2" %}{% if next %}?next={{ next }}{% endif %}" +> + {% heroicon_mini "building-office" size=18 class="inline align-text-bottom me-1" aria_hidden=true %} + {% blocktrans %}Log in with your {{ ORG_SHORT_NAME }} email{% endblocktrans %} +</a> \ No newline at end of file diff --git a/hypha/templates/includes/password_login_button.html b/hypha/templates/includes/password_login_button.html new file mode 100644 index 000000000..276766921 --- /dev/null +++ b/hypha/templates/includes/password_login_button.html @@ -0,0 +1,8 @@ +{% load i18n heroicons %} +<a + class="link link--button link--button--login" + href="{% url 'users:login' %}{% if next %}?next={{next}}{% endif %}" +> + {% heroicon_mini "key" size=18 class="inline align-text-bottom me-1" aria_hidden=true %} + {% trans "Log in with password" %} +</a> \ No newline at end of file diff --git a/hypha/templates/includes/passwordless_login_button.html b/hypha/templates/includes/passwordless_login_button.html new file mode 100644 index 000000000..d5d2ba78c --- /dev/null +++ b/hypha/templates/includes/passwordless_login_button.html @@ -0,0 +1,8 @@ +{% load i18n heroicons %} +<a + class="link link--button link--button--login" + href="{% url 'users:passwordless_login_signup' %}{% if next %}?next={{next}}{% endif %}" +> + {% heroicon_mini "envelope" size=18 class="inline align-text-bottom me-1" aria_hidden=true %} + {% trans "Log in without password" %} +</a> diff --git a/hypha/templates/includes/register_button.html b/hypha/templates/includes/register_button.html deleted file mode 100644 index 9134346bc..000000000 --- a/hypha/templates/includes/register_button.html +++ /dev/null @@ -1,4 +0,0 @@ -{% load i18n %} -<a href="{{ APPLY_SITE.root_url }}{% url 'users:register' %}{% if redirect_url %}?next={{ redirect_url }}{% endif %}" class="button button--transparent {{ class }}"> - {% trans "Sign up" %} -</a> -- GitLab