Adding password reset email change fix #400

arroyoAle · 2023-08-30T13:42:07Z

What and why?

This fixes the case where a user changes email while having requested a forgot password link by deleting the password reset token in the database when the party email is changed

How to test?

deploy to environment
follow the forgot password path for an account
check the token is in the database
change the user email and verify the change through the link provided
check the token has been removed from the database and the counter is reset

Jira

https://jira.ons.gov.uk/browse/RAS-840

arroyoAle · 2023-09-04T13:53:22Z

/deploy arroya

ras-rm-pr-bot · 2023-09-04T13:54:02Z

Deploying to dev cluster with following parameters:

namespace: arroya
tag: pr-400
configBranch: main
paramKey: ``
paramValue: ``

LJBabbage

This needs a 2nd look. We should also try not to update the Pipfile.lock if not needed, just to limit the amount of things changing outside the scope of this PR. We also should be looking at line coverage when adding code, in this instance that would have helped identify some problems

LJBabbage · 2023-09-05T08:17:17Z

ras_party/controllers/account_controller.py

@@ -746,6 +746,15 @@ def put_email_verification(token, tran, session):
        # We set the user as verified on the OAuth2 server.
        set_user_verified(email_address)

+    if not respondent.pending_email_address and respondent.password_verification_token:


This feels in the wrong place and order. The issue materializes because the email in the token has the original email and due to the pending becoming the new email makes it makes it invalid. With that in mind it should be there and not here. Having it here puts in on the original verification path which isn't valid.

It should be done inside update_verified_email_address or alternatively after line 727

This has been moved up to line 729 right after the email update

LJBabbage · 2023-09-05T08:17:40Z

ras_party/controllers/account_controller.py

@@ -746,6 +746,15 @@ def put_email_verification(token, tran, session):
        # We set the user as verified on the OAuth2 server.
        set_user_verified(email_address)

+    if not respondent.pending_email_address and respondent.password_verification_token:
+        if not respondent.password_verification_token:


Is this reachable code? As the above means respondent.password_verification_token has to be True

The second if statement has been removed

LJBabbage · 2023-09-05T08:20:39Z

ras_party/controllers/account_controller.py

+            raise NotFound("Verification token not found")
+
+        # Reset password token fields in the database since they are linked to the old email
+        delete_respondent_password_verification_token(respondent.party_uuid, session)


I do wonder if these 2 are intrinsically linked and should have a parent function, maybe worth checking other usages in the code

They are linked but there's no parent function and adding it would require work to frontstage as well and I think is out of the scope of the card

LJBabbage · 2023-09-05T08:21:52Z

test/party_client.py

@@ -434,3 +434,31 @@ def get_pending_surveys_with_originator_party_id(self, party_id, expected_status
    def delete_pending_surveys_with_batch_no(self, batch_no, expected_status=202):
        response = self.client.delete(f"/party-api/v1/pending-surveys/{batch_no}", headers=self.auth_headers)
        self.assertStatus(response, expected_status)
+
+    def get_password_verification_token(self, party_id, expected_status=200):


Is this used? I can't see where

I had added these for completeness but they are now removed

LJBabbage · 2023-09-05T08:22:13Z

test/party_client.py

+        self.assertStatus(response, expected_status)
+        return response.json
+
+    def post_password_verification_token(self, party_id, payload, expected_status=200):


Same again, perhaps I am missing it

LJBabbage · 2023-09-05T08:32:44Z

test/test_respondent_controller.py

@@ -1215,6 +1215,30 @@ def test_put_email_verification_uses_case_insensitive_email_query(self):
            account_controller.put_email_verification(token)
            query.assert_called_once_with("[email protected]", db.session())

+    def test_email_verification_resets_password_reset(self):
+        self.populate_with_respondent()


This returns a respondent, so you don't need the line below

It looks like it might even do a token as well

This has been updated

LJBabbage · 2023-09-05T08:33:48Z

test/test_respondent_controller.py

+        db_respondent = respondents()[0]
+        token = self.generate_valid_token_from_email(db_respondent.email_address)
+        response = self.put_email_verification(token, 200)
+        db_respondent = respondents()[0]


Not sure why this has been re-assigned to the same thing as above

This updates the respondent variable which would otherwise fail the tests as it still contains the verification token and counter is 1

LJBabbage · 2023-09-05T08:34:39Z

test/test_respondent_controller.py

+        db_respondent = respondents()[0]
+        self.assertIsNone(db_respondent.password_verification_token)
+        self.assertEqual(db_respondent.password_reset_counter, 0)
+        self.assertEqual(response["status"], RespondentStatus.ACTIVE.name)


Checking status, is this needed, there should be another test that does that?

This has been removed

LJBabbage · 2023-09-05T08:34:58Z

test/test_respondent_controller.py

+        self.assertEqual(db_respondent.password_reset_counter, 0)
+        self.assertEqual(response["status"], RespondentStatus.ACTIVE.name)
+
+    def test_email_verification_no_password_reset_token(self):


Similar to the above with comments

LJBabbage · 2023-09-05T08:35:39Z

test/test_respondent_controller.py

-            self.get_respondent_by_id(respondent.party_uuid)
-        with self.assertRaises(Exception):
-            self.get_respondent_by_email(respondent_1.email_address)
+    # def test_batch_delete_user_data_marked_for_deletion(self):


Why has this been commented out? There should be reason in the code, on the PR. Or if not needed just removed

This was just to test locally since it was hanging for a long time locally, has been reverted

LJBabbage · 2023-09-06T12:37:55Z

ras_party/controllers/account_controller.py

@@ -726,6 +726,10 @@ def put_email_verification(token, tran, session):

        if respondent:
            update_verified_email_address(respondent, tran)
+            if not respondent.pending_email_address and respondent.password_verification_token:


This still needs some work, to get to this line we know this is a pending email update. The respondent has received an email which when they clicks updates the email with the pending and then removes the pending_email (line 798) (i.e update_verified_email_address)

Now because of that and the order this has been put in if not respondent.pending_email_address becomes redundant as the code underneath will always set respondent.pending_email_address to None, all we need to do is check the verification token existence

This has been updated

LJBabbage · 2023-09-06T12:41:22Z

test/party_client.py

+        response = self.client.delete(
+            f"/party-api/v1/respondents/{party_id}/password-verification-token/{token}", headers=self.auth_headers
+        )
+        self.assertStatus(response, expected_status)


This assert is still a bit light and password-verification-token returns return jsonify({"message": "Successfully removed token"}) which we should use

added functionality to check what message is returned by the api

LJBabbage · 2023-09-06T12:42:38Z

test/party_client.py

@@ -434,3 +434,15 @@ def get_pending_surveys_with_originator_party_id(self, party_id, expected_status
    def delete_pending_surveys_with_batch_no(self, batch_no, expected_status=202):
        response = self.client.delete(f"/party-api/v1/pending-surveys/{batch_no}", headers=self.auth_headers)
        self.assertStatus(response, expected_status)
+
+    def delete_password_verification_token(self, party_id, token, expected_status=200):


We wouldn't usually have a default var in a function when it is single use. It does make me wonder how we are testing 404 failures, but that is for another story I guess

For the password token parts the rest is getting tested in the test_account_controller.py

LJBabbage · 2023-09-06T12:44:16Z

test/party_client.py

+        )
+        self.assertStatus(response, expected_status)
+
+    def reset_password_reset_counter(self, party_id, expected_status=200):


Both comments above are valid here as well, although return jsonify({"message": "Successfully reset counter"}) this time

Added functionality to check what message is returned by the api

LJBabbage · 2023-09-06T12:52:26Z

test/test_respondent_controller.py

+        self.assertIsNone(respondent.password_verification_token)
+        self.assertEqual(respondent.password_reset_counter, 0)
+
+    def test_email_verification_no_password_reset_token(self):


Do we need this function? does it not have line coverage already?

It's more to check if the section doesn't fall over when there is no verification token as the rest of the tests have a token by default

Ok, it's not really needed as all you are doing is removing the token and then running the code and making sure it's not there. Not major, but I would removed

LJBabbage

Definitely looking better and party is there as far as I can see, but there is more to do on this story. If you run it into GCP and request a password change and then go in and change the email (by clicking the email etc) the token is removed as expected, however if you then complete the circuit and click the password change email you get a nasty 500 in the code, because of this https://github.com/ONSdigital/ras-frontstage/blob/main/frontstage/views/passwords/reset_password.py#L29. and that is caused as the 404 is just returning https://github.com/ONSdigital/ras-frontstage/blob/main/frontstage/controllers/party_controller.py#L151 The passwords/password-token-not-found.html is probably correct, the code has not been written correctly for the 404

adding password reset email change fix

4415214

arroyoAle changed the title ~~dding password reset email change fix~~ Adding password reset email change fix Aug 30, 2023

arroyoAle and others added 4 commits September 1, 2023 16:14

fixing code for tests

34a6af9

auto patch increment

1b70994

adding test to check token and counter have been reset

d465915

changing trello to jira in PR comment template

aa476ab

arroyoAle marked this pull request as ready for review September 4, 2023 11:29

adding test with no password token and adding to party_client.py

13f72de

LJBabbage requested changes Sep 5, 2023

View reviewed changes

arroyoAle and others added 6 commits September 5, 2023 14:05

start of addressing comments

fa19a20

auto patch increment

5af8835

Merge branch 'main' into fix-password-reset-with-email-change

572864f

addressing the rest of the comments

00f9074

addressing the rest of the comments

05278d6

reverting Pipfile.lock changes

d3cc704

LJBabbage requested changes Sep 6, 2023

View reviewed changes

review changes

e254d7d

LJBabbage requested changes Sep 6, 2023

View reviewed changes

removing no token test

8923d1b

LJBabbage approved these changes Sep 8, 2023

View reviewed changes

arroyoAle merged commit 95fa70b into main Sep 8, 2023
1 check passed

arroyoAle deleted the fix-password-reset-with-email-change branch September 8, 2023 12:59

Adding password reset email change fix #400

Adding password reset email change fix #400

Conversation

arroyoAle commented Aug 30, 2023 • edited Loading

What and why?

How to test?

Jira

arroyoAle commented Sep 4, 2023

ras-rm-pr-bot commented Sep 4, 2023

LJBabbage left a comment • 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

LJBabbage Sep 5, 2023 • 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

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LJBabbage left a comment • edited Loading

Choose a reason for hiding this comment

arroyoAle commented Aug 30, 2023 •

edited

Loading

LJBabbage left a comment •

edited

Loading

LJBabbage Sep 5, 2023 •

edited

Loading

LJBabbage left a comment •

edited

Loading