Feature/6222 - Endpoint /api/v1/users/sign_out revokes access token and refresh token on request #6241
Conversation
|
👀 |
| let(:casa_org) { create(:casa_org) } | ||
| let(:volunteer) { create(:volunteer, casa_org: casa_org) } | ||
| let(:api_credential) { create(:api_credential, user: volunteer) } | ||
| let(:refresh_token) { api_credential.return_new_refresh_token![:refresh_token] } |
There was a problem hiding this comment.
Er, is this regenerating a refresh token a 2nd time?
There was a problem hiding this comment.
I think when you create the api credential table for the first time, the token digest fields are nil - at least in dev environment and so that is why I created the seed file to populate them.
I also need the plain text refresh token to be passed in the authorization header for the sign out.
There was a problem hiding this comment.
Looks good so far, as mentioned on the issues acceptance criteria and in our discussions. Still need to...
- Add the
sign_inroute toroutes.rb(session#destroy) - Add a destroy function to the
sessions_controller.rbusing therevocationfunction(s) helper you made - Address comments, test route with whatever you'd like to use though my preference is curl, then should lgtm 😎
xihai01
requested review from
compwron,
FireLemons and
elasticspoon
as code owners
| @@ -8,6 +8,22 @@ def create | |||
| end | |||
| end | |||
|
|
|||
| def destroy | |||
| # fetch refresh token from request header | |||
| refresh_token = request.headers["Authorization"]&.split(" ")&.last | |||
There was a problem hiding this comment.
Consider how we are structuring the response from the iOS app. If we're using the header, it's the access token or "api_token" that would go there, while the refresh token is in the body.
You'll need both in order to check whose tokens to revoke or set to nil on sign_out.
| # fetch refresh token from request header | ||
| refresh_token = request.headers["Authorization"]&.split(" ")&.last | ||
| # find user's api credentials by refresh token | ||
| api_credential = ApiCredential.find_by(refresh_token_digest: Digest::SHA256.hexdigest(refresh_token)) |
There was a problem hiding this comment.
api_credential = ApiCredential searches user based on only one token, instead do.
user_credentials = ApiCredential.find_by(
api_token_digest: Digest::SHA256.hexdigest(access_token),
refresh_token_digest: Digest::SHA256.hexdigest(refresh_token)
)
if user_credentials
user_credentials.revoke_api_token
user_credentials.revoke_refresh_token
render json: {message: "Signed out successfully."}, status: 200
else
...
There was a problem hiding this comment.
I'm not sure if we should use the api_token for checking because it expires every 7 hours. Using the refresh token is more reliable because it has a longer lifespan. If we want an additional check, we can pass along the user id in the request body as well.
So we would have something like .find_by(user_id, refresh_token_digest). Let me know what you think.
There was a problem hiding this comment.
I see, traditionally though, it's the access token/our api_token that goes in the header on every response. Having it expire frequently is the point.
This goes beyond just sign-out and will reflect the shape of our responses from the client past sign-in for every request so should scope this.
But if we're going to sign the user out based on just one token value it should be the access token because that value is valid for less time/more secure.
There was a problem hiding this comment.
Ah I see. That makes sense now.
Then let's put the access token in the header and use that for sign out.
No but can defiantly make one. This is the current AccountScreen with a Sign-out button. Current behaviour is just a console log |
ok I got it. I can work on this issue. Let me know if you havn't already and I will create an issue for it. |
Thanks, all good, here's the issue for end-to-end implementation. |
What github issue is this PR for, if any?
Resolves #6222
What changed, and why?
Added /api/v1/users/sign_out endpoint so both the access and refresh tokens for that user is cleared from the api_credentials table and set to nil.
Why?: for added security - tokens should be removed from api_credentials table because user is no longer signed in.
How is this tested? (please write tests!) 💖💪
Token Destroyer Helper Function tests (2 in total)→ spec/models/api_credential_spec.rbSign Out Request Test for 200 and 401 Response Cases→ spec/requests/api/v1/users/sessions_spec.rbScreenshots please :)
Testing sign out with postman on localhost
Steps:
First we sign in to fetch the refresh token
Lastly we sign out and pass in refresh token in the request authorization header
Feelings gif (optional)