-
-
Notifications
You must be signed in to change notification settings - Fork 13
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
Endpoint user/challenger/recover #636
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for submitting a PR 🎉 It's very appreciated!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi 👋 I'm still subscribed to this repo, so I'm getting notifications and saw this coming up (& I worked on the email challenge bits before), so I thought I'd give it a short review, even though it wasn't requested 😅
-- | ||
:> Capture "Username" Username | ||
-- | ||
:> Auth.HigherOrder |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line ensures that only fission-registered accounts (and UCANs that are delegated from fission-registered accounts) can make these requests 👍
recover username = do | ||
Entity userId User { } <- Web.Err.ensureMaybe couldntFindUser =<< getByUsername username | ||
now <- currentTime | ||
challenge <- RecoveryChallenge.create userId now |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
However, here we don't check any other property about the authentication, right?
So if I read this correctly, doesn't this allow any fission user to issue & get a challenge for any other fission user?
E.g. a malicious actor could create a new fission account, and then use their UCAN to run a request /user/challenge/recover/bob
, and then use the response to call /user/did/bob/{challenge}
with a DID they control as the body for that request, essentially taking away my control over bob's fission account and giving it to themselves?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahh thanks for catching that! I'll admit I have a fairly tenuous understanding of this codebase. This new endpoint was cobbled together from bits of various other ones I saw in the repo 😅 Looking at some other examples, I think I see how to add that validation here. I'll get this fixed today!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the goal, actually? How should the auth gating work for that endpoint?
Is the plan to require the user to be authed to access this endpoint? Although in that case I wonder why they'd need to recover in the first place.
Is the idea to allow this endpoint only for the email service? I.e. the email service has a special DID that this fission server respects and it opens up this endpoint for it?
Or is the idea that the fission service's DID would be the "root owner" in this case, and we'd check that the root DID of a UCAN coming in to use the recovery endpoint is the fission service's DID, allowing us to delegate a UCAN for accessing the recovery endpoint to other services such as the email service?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The current approach we're taking is:
- On registration, the client builds a UCAN that delegates root access to the
rs-email-service
's DID - That UCAN is sent to the
rs-email-service
and stored in a DB that maps usernames to UCANS - Then, when a user goes to recover their account, they enter their username and a request is sent from the client to the
rs-email-service
, which finds the associated UCAN in the DB and uses that to make a request to this new fission server endpoint to get a challenge - With the challenge in hand, the
rs-email-service
should be able to call/v2/api/user/did/{Username}/{Challenge}
to recover the user's account
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I should add that the rs-email-service
is just meant to be an example of how this email recovery flow could work. We expect devs to create their own services that maybe reference our rs-email-service
example
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That sound awesome and exactly like what I'd expect.
Let's go 🚀
Summary
This PR fixes/implements the following bugs/features
user/challenge/recover
endpoint to return achallenge
directly from the API rather than sending an email to the user that has thechallenge
appended to the dashboard linkWe need an endpoint that returns a
challenge
directly so thers-email-service
can call/v2/api/user/did/{Username}/{Challenge}
on behalf of user to recover their account.Test plan (required)
We will deploy this branch to staging to allow for easier testing before it's merged
Create a new user and call
/v2/api/user/challenge/recover/{Username}
with a valid UCAN as the bearer token, expect achallenge
to be returnedAfter Merge