Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor the remaining pieces of the sign-in process #1512

Merged
merged 10 commits into from
May 9, 2024
Merged

Conversation

terrazoon
Copy link
Contributor

@terrazoon terrazoon commented May 6, 2024

Description

  1. The check for invited user email vs expected email was already moved to register.py in a previous PR, so it is in place and working.
  2. Added many debug statements so in the future we can just temporarily set the log level on production to debug.
  3. Added tests for accepting invite and for failing due to email address mismatch
  4. Added tests for encoding/decoding invite data
  5. removed old code and tests pointing to things (/register) that don't exist anymore.

Security Considerations

  1. the debug statements dump a lot of stuff including user's email addresses. The thinking is we typically need this info to debug and we would only enable it on production during an urgent debug session, for a short period of time. I think there is already a ticket to make some way to automatically scrub logs and partially mask email addresses etc, but that doesn't exist yet.
  2. We are passing the invite data in the URL and it is not particularly hard to decode it. Amongst this info is the invitee's email address. Do we have to actually encrypt this information? I feel like we are only sending it to the invitee's email inbox, and from there they are clicking on the link and going to login.gov, etc. It's not clear to me that it's such a big risk to leave it as is with just base64 encoding.

@terrazoon terrazoon marked this pull request as draft May 6, 2024 17:43
@terrazoon terrazoon changed the title notify-admin-1459 Refactor the remaining pieces of the sign-in process May 7, 2024
@terrazoon terrazoon marked this pull request as ready for review May 7, 2024 21:37
Copy link
Contributor

@ccostino ccostino left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome, thanks @terrazoon! On the whole this looks good to me and it appears like it'll at least address the issue, though I still need to test locally. I had a few comments and questions that I've left in the mean time as I continue going through things and others get a chance to take a look.

By all accounts it looks like this takes care of everything in #1459 too, except possibly some E2E test things with the Login.gov integration, but that's fine to save for another issue (which we should have, I'll check the board).

Nicely done with the test cleanup and updates as well, thank you! 🎉

app/main/views/register.py Outdated Show resolved Hide resolved
app/main/views/sign_in.py Show resolved Hide resolved
app/models/user.py Outdated Show resolved Hide resolved
tests/app/main/views/test_register.py Outdated Show resolved Hide resolved
Copy link
Contributor

@ccostino ccostino left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One final change and I think we're good to merge this to staging for final testing - all looked good to me locally, but I haven't been able to replicate the issue locally it seems. Only in staging or production.

app/models/user.py Outdated Show resolved Hide resolved
Copy link
Contributor

@ccostino ccostino left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, @terrazoon! Let's get this into staging for some more testing!

@ccostino ccostino merged commit 37c57cb into main May 9, 2024
8 checks passed
@ccostino ccostino deleted the notify-admin-1459 branch May 9, 2024 00:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants