Feature/6222 - Endpoint /api/v1/users/sign_out revokes access token and refresh token on request #6241
Conversation
|
👀 |
app/models/api_credential.rb
Outdated
| @@ -37,6 +37,19 @@ def is_refresh_token_expired? | |||
| refresh_token_expires_at < Time.current | |||
| end | |||
|
|
|||
| # clear tokens | |||
| # token argument takes in two strings: api_token and refresh_token | |||
| def revoke_token(token) | |||
There was a problem hiding this comment.
Mind separating these out like a revoke_api_token and revoke_refresh_token function, stylistic to go with the other functions.
There was a problem hiding this comment.
Initially this is what I wanted to do but then I thought of avoiding duplication
Let me know if you still want to keep them separate.
There was a problem hiding this comment.
I have separated it into two functions.
spec/models/api_credential_spec.rb
Outdated
| @@ -100,4 +100,24 @@ | |||
| expect(api_credential.refresh_token_digest).to eq(Digest::SHA256.hexdigest(refresh_token)) | |||
| end | |||
| end | |||
|
|
|||
| describe "#revoke_token" do | |||
There was a problem hiding this comment.
Same thing as above here. So would make 2 tests or one for each function.
| @@ -42,6 +42,12 @@ | |||
| properties: { | |||
| message: {type: :string} | |||
| } | |||
| }, | |||
| sign_out: { | |||
| @@ -19,7 +19,7 @@ components: | |||
| type: string | |||
| email: | |||
| type: string | |||
| api_token_expires_at: | |||
| token_expires_at: | |||
There was a problem hiding this comment.
no problem. Just in case you weren't aware, we shouldn't edit this yaml file. It is automatically generated by the bin/rails rswag:swaggerize command.
So in the future whenever we add swagger request specs or make edits to the swagger_helper.rb
then make sure to run the above swaggerize command to update the yaml. 😎
| @@ -28,6 +28,11 @@ components: | |||
| properties: | |||
| message: | |||
| type: string | |||
| sign_out: | |||
| @@ -61,6 +66,30 @@ paths: | |||
| required: | |||
| - password | |||
| "/api/v1/users/sign_out": | |||
| 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 😎
WIP WIP WIP WIP WIP
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)