Allow multiple project invites for one user #1043
Conversation
There was a problem hiding this comment.
help me understand the workflow here. User clicks an invite link, they land at the register page and register. They click another invite and then what? they already registered, so they should just be added automatically. But it looks like they would be required to 'register' again which doesn't really make sense. They should just be able to click the link and the invite is accepted on the account they already created (assuming details match etc).
|
After looking at the code review, I think I got the flow wrong. I thought the accept-invite link went to the login controller and from there redirected to the |
rmunn
force-pushed
the
feat/allow-multiple-project-invites
branch
from
1ebb43e to
632e94f
Compare
|
I now have the frontend forbidding email changes. Next up is figuring out how to allow the register page to be skipped if the user already has an account. We could put this logic in LoginRedirect, but that seems like not quite the right solution: it's a special case, and I don't want to load up LoginRedirect with special cases. I'll think about it. |
rmunn
force-pushed
the
feat/allow-multiple-project-invites
branch
from
a900c17 to
05eda53
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
Commit 05eda53 didn't work, as I couldn't figure out how to tell HotChocolate to allow the Register audience access to the GQL query I needed. So I tried d253132 instead, which adds logic into LoginRedirect to use an alternate URL if the email already exists. I'm not super happy with putting special-case logic into LoginRedirect, so if you have a better suggestion I'd love to try it. But this is at least working. Still to do: display errors, if any, in the alternate acceptInvitation page, and probably center the turnstile component instead of sticking it off on the side of the page, since its spinner and green checkmark works nicely as a "Please wait... Hey, you're approved" indicator when things succeed. (Which is why we need to hide it away and display errors if things didn't work). |
rmunn
force-pushed
the
feat/allow-multiple-project-invites
branch
from
8848847 to
d253132
Compare
|
Nope, bb04079 doesn't work because the Projects and Orgs lists haven't been populated in our query. https://www.github.com/dotnet/efcore/issues/4526 would fix this if it had ever been implemented, but it doesn't look like that will ever make it into EF Core from the tenor of the discussion so far. (AFAICT, SQL Server doesn't implement the necessary feature, and EF Core doesn't want to add features that start failing if you switch databases). Simplest fix is probably to just do |
|
Not 100% sure if I should even have an error message for "unknown error" or not, but other than that minor quibble, this is working. I'd welcome input from @hahn-kev on my approach with LoginController; I was worried that it could be used to expose whether a given email address belongs to a user on our system, but in order to do that the attacker would have to forge or capture a JWT from us containing that email address. If they can forge a JWT, we have much bigger problems. And if they have captured a JWT, they probably got it from the person's email, in which case they already know that person is receiving emails from Lexbox. However, security concerns aside, I'm still wondering if there was a better way to do this. I would have preferred to have the acceptInvitation page do a GraphQL query to say "Hey, does this account already exist?" and skip showing the form if that's the case. But I couldn't figure out how to make that work, so I went with the LoginController approach instead. |
|
Marking as draft since 5b820df is NOT finished, or even tested. It's just a rough sketch of what I'll be implementing, so no code review needed yet. |
|
@hahn-kev - Self-review completed; this is ready for final review now. |
Now that we use invitation links for both projects and orgs, it will start to become rather common for someone to receive an org invitation and a project invitation within the same few minutes. So we need to allow users to click on invitation links even if they already have a Lexbox account.
Without this check, it would be possible for someone who manages to intercept someone else's email (by social engineering or other means) to forge a project invitation request, reusing the still-valid JWT from the genuine user.
No backend checks yet, this is just frontend for now.
This fails: despite the AllowAnonymous, the GraphQL query is being rejected because the RegisterAccount audience is not allowed to use GQL queries. I'll need to find some other way to do this.
Instead of GQL queries to check if the user already exists, we put the logic into LoginController. This works: users can now receive multiple invitation links, and only see the registration page if they have not registered yet. If they have registered before, they're taken to an alternate URL that logs them in immediately. Still to do: ensure that the acceptInvitation backend doesn't throw an error if the user is already a member of the project, but just silently returns success if we're already in the desired state.
If a user clicks the same invitation link twice, he should not get an error message, he should just be taken to his home page. He's already a member of the project, so trying to join the project should not error but just succeed silently.
This allows the turnstile component to double as a visual indicator to the user that things are progressing.
Ensure that LoginRedirect can never be used to redirect to a different site. In the future we may whitelist certain domains such as lexbox.org, but for now we just strip off the hostname and make sure it has to be a relative URL.
WIP. Not yet tested, probably not yet functional.
Rather than redirect to the frontend and then query the backend to find out if the user already exists, let's redirect to a backend endpoint, which will then decide which frontend page to redirect to. A double redirect may not be ideal, but it's better than special-case code in the LoginRedirect method.
Back to just a single frontend page for accepting invites — great.
This can only happen if an admin creates the account while the user was filling out the registration page, so this probably will never happen.
These services are already injected in the constructor
rmunn
force-pushed
the
feat/allow-multiple-project-invites
branch
from
22dddf0 to
8b939a1
Compare
Fixes #584.
Now that we have both org and project invites, it's going to be pretty common for a user to receive at least two invites in a short time: one to invite him to an org, and the other to a project (likely owned by the org).