search

wellcomecollection / identity

Identity services for Wellcome Collection users
MIT License
0 stars 2 forks source link

Fix/login duplicate email #275

Closed melanierogan closed 2 years ago

melanierogan commented 2 years ago

Problem

As a user When I enter an email address that is duplicated I want a helpful error message so I'm clear why something has gone wrong

Ticket https://github.com/wellcomecollection/wellcomecollection.org/issues/7594

What has changed

I have tried to allow for the Error status 409/Duplicate patron in get_user, then add a case for it in login. We handle 409 Errors with ResponseStatus.MalformedRequest, I have adjusted our custom duplicateEmailCredentialMessage to throw that error at point of login if Sierra gives us a Malformed Request at login.

Further context

In auth0 logs, this error appears as: Unhandled API response [Request failed with status code 409] (cause: [{"code":133,"specificCode":0,"httpStatus":409,"name":"Internal server error","description":"Duplicate patrons found for the specified varFieldTag

jamieparkinson commented 2 years ago

Nice detective work on this - have you thought about how it'll affect other flows apart from login?

As per the docs, the "Get User" script runs in the following situations: create user, change email, change password, and password reset. We also know it runs at login.

I think there is still a problem with the Change Email script - what happens if a user tries to change their email to one that is present in multiple records? I think(?) the call to updatePatronEmail will succeed, leaving the user unable to log in. I guess a fix could be to check for the existence of the user manually in the Change Email script, but then we're duplicating functionality - and, maybe more importantly, Sierra API calls - that Auth0 does for us.

melanierogan commented 2 years ago

As per the docs, the "Get User" script runs in the following situations: create user, change email, change password, and password reset. We also know it runs at login.

Eesh, yes. I had not thought about the 'change' cases in email and password, or password reset. Let me have a look through and think about all cases.

I think there is still a problem with the Change Email script - what happens if a user tries to change their email to one that is present in multiple records? I think(?) the call to updatePatronEmail will succeed, leaving the user unable to log in. I guess a fix could be to check for the existence of the user manually in the Change Email script, but then we're duplicating functionality - and, maybe more importantly, Sierra API calls - that Auth0 does for us.

Yes this certainly complicates a seemingly straight forward use case. The upshot of this feels like, if we potentially allow for the use case of displaying the duplicate user message, we over complicate the auth0 flow elsewhere (in this case in changing email).

I wonder then, if looking at the 'problem' again helps. At the moment, the default error messaging from auth0 renders differently i.e. invalid email errors show in-line on the ui in the styling we want, but the current 'Something went wrong' error does not. Perhaps there is something I can do to implement default error styling.

I need to have a think through it

melanierogan commented 2 years ago

I'm going to close this PR as I have changed the approach to this particular ticket: ultimately I am going to focus on changing the default 'something went wrong, please try again later' wording. I want to refer to this closed PR in the ticket.