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

fix service invites #945

Merged
merged 3 commits into from
Apr 25, 2024
Merged

fix service invites #945

merged 3 commits into from
Apr 25, 2024

Conversation

terrazoon
Copy link
Contributor

@terrazoon terrazoon commented Apr 24, 2024

Description

The original attempt to implement service invites via login.gov bypassed most of the logic already contained in InvitedUser, which led to a state where things almost worked if you followed the happy path, but if you did anything unusual you would see weird anomalies.

This PR goes back and re-implements, removing the attempt to use redis to store the invite info. Instead the invite info is sent out in the login.gov as the STATE parameter, which mimics the previous implementation.

Security Considerations

This is not really a security consideration, but feedback from the login.gov team via zendesk support is that, while the state parameter is always returned back unaltered, it might be better to use a session or something to hold the information. I have two objections to this:

  1. Person A is sending an invite via the API server, and person B is receiving and accepting the invite via the UI, so as a practical matter I'm having a hard time visualizing how a session would work.

  2. Assuming a session could work, I believe we would run into the same situation that we ran into when we tried to use redis. I.e., assume a user has 3 pending invites, and they follow the link to login.gov, and then back to our app. Which invite to accept? There is know way to know, unless some kind of information is contained in the URL, which puts us right back to using the STATE param.

More Security Considerations

Note that the invitee's email address gets passed in the STATE param and that it's a form of PII. Right now we are just base64 encoding it, on the assumption that it is very transient. It gets sent by email to that email address, presumably used by the user, and then basically discarded. We could encrypt if need be, but maybe it's not necessary? I'm not really seeing an exploit here.

@terrazoon terrazoon self-assigned this Apr 24, 2024
@terrazoon terrazoon marked this pull request as draft April 24, 2024 18:01
@terrazoon terrazoon marked this pull request as ready for review April 25, 2024 14:16
@terrazoon terrazoon requested review from ccostino and a team April 25, 2024 14:28
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.

Thank you for the detailed write-up with the PR, @terrazoon! I think we're okay on the security concerns side, this isn't passing sensitive data back and forth, we're just looking for a way to serialize all of the values into a single thing we can pass with that state argument; the previous URL was generated with everything as well.

I only had one quick question around the generation of the permission list and if there was a way to simplify that some. Once that's ironed out if need be, this is good to go!

app/service_invite/rest.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!

@ccostino ccostino merged commit 87d0ae2 into main Apr 25, 2024
5 checks passed
@ccostino ccostino deleted the fix_service_invites branch April 25, 2024 20:58
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