From 8d5974eb47e94902029673e11b09ef73f0a509db Mon Sep 17 00:00:00 2001 From: Ben Thorner Date: Mon, 7 Mar 2022 11:02:33 +0000 Subject: [PATCH] Simplify "activate_user" to clear session earlier A snippet of old code [^1] in "activate_user" was forcing us to keep "user_details" in the session until the very last moment with a "try / finally" combo. But "activate_user" already knows the ID of the user: it's the argument we pass to the function. None of the functions called by "activate_user" require the session to have that key either i.e. it's definitely redundant. It's unclear if we need to pop the key from the session in both "verify" methods - there are no tests covering this behaviour. For now, we can at least make the flow clearer by adjusting where we do the "pop" and the assignment. [^1]: https://github.com/alphagov/notifications-admin/commit/bbc7b173f05305fca3386138479ccdba257be302#diff-d12384ece5ad90e9b66063fd3ab170453788d36b7e0babf49ee016f0a880f251L71 --- app/main/views/verify.py | 17 ++++++----------- 1 file changed, 6 insertions(+), 11 deletions(-) diff --git a/app/main/views/verify.py b/app/main/views/verify.py index 9e434ff65..f72302a14 100644 --- a/app/main/views/verify.py +++ b/app/main/views/verify.py @@ -31,10 +31,8 @@ def verify(): form = TwoFactorForm(_check_code) if form.validate_on_submit(): - try: - return activate_user(user_id) - finally: - session.pop('user_details', None) + session.pop('user_details', None) + return activate_user(user_id) return render_template('views/two-factor-sms.html', form=form) @@ -62,15 +60,12 @@ def verify_email(token): flash("That verification link has expired.") return redirect(url_for('main.sign_in')) - session['user_details'] = {"email": user.email_address, "id": user.id} - if user.email_auth: - try: - return activate_user(user.id) - finally: - session.pop('user_details', None) + session.pop('user_details', None) + return activate_user(user.id) user.send_verify_code() + session['user_details'] = {"email": user.email_address, "id": user.id} return redirect(url_for('main.verify')) @@ -95,7 +90,7 @@ def activate_user(user_id): invited_org_user = InvitedOrgUser.from_session() if invited_org_user: - user_api_client.add_user_to_organisation(invited_org_user.organisation, session['user_details']['id']) + user_api_client.add_user_to_organisation(invited_org_user.organisation, user_id) if organisation_id: return redirect(url_for('main.organisation_dashboard', org_id=organisation_id))