[frjo]

The actual bug got fixed some time ago. During the review of that fix it was found that the implementation had some other issues as well.

The email change confirmation link applied the change as soon as it was fetched with a GET. That ment that link-preview bots etc. could trigger it. This PR implements a confirmation screen just like we do in other places in Hypha. The user clicks the link in the e-mail and then comes to the confirmation screen.

Other things also fixed in this PR:

• Inconsistencies in user templates.
• Autoescape issues in mail templates.
• Make profile form normalise the email the same way registration does.
• Add ratelimit to e-mail change view.
• Add a Log out other sessions button to the account page.
• Tie token to user, not only email.

Test Steps

• Test that the e-mail change flow works well for users.
• Test that a user can only change to an address they own/control.
• Test for information leaks, can you check if a specific address exist in the system?

[wes-otf]

Testing this today!

[frjo]

There are more things I want to do here, like using MarkdownMail and EMAIL_SUBJECT_PREFIX for all user e-mail, but need to hold off. The PR is big enough as it is.

The important changes are in:

• hypha/apply/users/views.py
• hypha/apply/users/utils.py
• hypha/apply/users/forms.py

The rest is docs, templates and tests.

[wes-otf]

this is a great catch

[wes-otf]

this worked great and includes some really fantastic improvements! I had some more nitpicky comments that are more UI based but all core functionality is perfect

[wes-otf]

Suggested change

	messages.success(self.request, self.success_message)
	if len(form.changed_data) > 1:
	messages.success(self.request, self.success_message)

maybe we could do something like this? the profile hasn't been updated yet if it was only the email that was changed.

[wes-otf]

Suggested change

	<h3 class="mt-6 mb-2 text-sm font-semibold">{% trans "Sessions" %}</h3>
	<h3 class="mt-6 mb-2 text-sm font-semibold">{% trans "Sessions" %}</h3>
	{% if other_sessions_count %}
	<p class="mb-2 text-sm text-fg-muted">
	{% blocktrans count sessions=other_sessions_count %}
	Log out of {{ other_sessions_count }} other session, for example on another computer or a lost device. You stay logged in here.
	{% plural %}
	Log out of {{ other_sessions_count }} other sessions, for example on another computer or a lost device. You stay logged in here.
	{% endblocktrans %}
	</p>
	<form method="post" action="{% url 'users:logout_other_sessions' %}">
	{% csrf_token %}
	<button class="btn btn-warning btn-outline" type="submit">
	{% trans "Log out other sessions" %}
	</button>
	</form>
	{% else %}
	<p class="mb-2 text-sm text-fg-muted">
	{% trans "You aren't actively logged in on any other devices." %}
	</p>
	{% endif %}

Thoughts on something like this? Might be nice to inform the user if they're actually logged in anywhere/if that button will actually do anything. Code I used to make it work locally:

added convenience func to utils.py

def get_active_sessions(user):
    """Gets all active sessions for a user. Manually decodes each session to extract the user ID"""
    return [session for session in Session.objects.filter(expire_date__gt=timezone.now()) if session.get_decoded().get(SESSION_KEY) == str(user.pk)]

updated AccountView's get_context_data

    def get_context_data(self, **kwargs):
        show_change_password = (
            password_management_enabled() and self.request.user.has_usable_password(),
        )
        
        # Don't include the user's current session in the count
        other_sessions_count = len(get_active_sessions(self.request.user)) - 1

        return super().get_context_data(
            default_device=default_device(self.request.user),
            show_change_password=show_change_password,
            other_sessions_count=other_sessions_count,
            **kwargs,
        )

updated logout_other_sessions_view

@login_required
@require_POST
def logout_other_sessions_view(request):
    """Log out all sessions of the user except the current one."""
    for session in [s for s in get_active_sessions(request.user) if s.session_key != request.session.session_key]:
        session.delete()
    messages.success(request, _("You have been logged out of all other sessions."))
    return redirect("users:account")

[wes-otf]

oops I meant to comment on the entire session logout template block

[frjo]

Excellent suggestion!

[frjo]

Implemented your suggestions now. Made a few changes, added a "get_other_sessions" helper function that always excludes current session instead of "get_active_sessions". Also made it yield as soo as it found another user session, we only need to know if it is more than one.

The check might becomes a tad slow on large and active sites so please see how it behaves on OTF live. But it only runs when user access their account page so not often.