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

Feature/6222 - Endpoint /api/v1/users/sign_out revokes access token and refresh token on request #6241

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion app/controllers/api/v1/base_controller.rb
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
class Api::V1::BaseController < ActionController::API
rescue_from ActiveRecord::RecordNotFound, with: :not_found
before_action :authenticate_user!, except: [:create]
before_action :authenticate_user!, except: [:create, :destroy]

def authenticate_user!
api_token, options = ActionController::HttpAuthentication::Token.token_and_options(request)
Expand Down
16 changes: 16 additions & 0 deletions app/controllers/api/v1/users/sessions_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,22 @@ def create
end
end

def destroy
# fetch refresh token from request header
refresh_token = request.headers["Authorization"]&.split(" ")&.last
Copy link
Collaborator

@7riumph 7riumph Mar 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

# find user's api credentials by refresh token
api_credential = ApiCredential.find_by(refresh_token_digest: Digest::SHA256.hexdigest(refresh_token))
Copy link
Collaborator

@7riumph 7riumph Mar 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
  ...

Copy link
Collaborator Author

@xihai01 xihai01 Mar 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

@7riumph 7riumph Mar 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah I see. That makes sense now.
Then let's put the access token in the header and use that for sign out.

# set api and refresh tokens to nil; otherwise render 401
if api_credential
api_credential.revoke_api_token
api_credential.revoke_refresh_token
render json: {message: "Signed out successfully."}, status: 200
else
render json: {message: "An error occured when signing out."}, status: 401
nil
end
end

private

def user_params
Expand Down
8 changes: 8 additions & 0 deletions app/models/api_credential.rb
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,14 @@ def is_refresh_token_expired?
refresh_token_expires_at < Time.current
end

def revoke_api_token
update_columns(api_token_digest: nil)
end

def revoke_refresh_token
update_columns(refresh_token_digest: nil)
end

private

# Generate unique tokens and hashes them for secure db storage
Expand Down
2 changes: 1 addition & 1 deletion config/routes.rb
Original file line number Diff line number Diff line change
Expand Up @@ -250,7 +250,7 @@
namespace :v1 do
namespace :users do
post "sign_in", to: "sessions#create"
# get 'sign_out', to: 'sessions#destroy'
delete "sign_out", to: "sessions#destroy"
end
end
end
Expand Down
1 change: 1 addition & 0 deletions db/seeds.rb
Original file line number Diff line number Diff line change
Expand Up @@ -147,3 +147,4 @@ def create_org_related_data(db_populator, casa_org, options)
Rails.logger.error { "Caught error during db seed emancipation_options_prune, continuing. Message: #{e}" }
end
load(Rails.root.join("db/seeds/placement_data.rb"))
load(Rails.root.join("db/seeds/api_credential_data.rb"))
6 changes: 6 additions & 0 deletions db/seeds/api_credential_data.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
ApiCredential.destroy_all
users = User.all

users.each do |user|
ApiCredential.create!(user: user, api_token_digest: Digest::SHA256.hexdigest(SecureRandom.hex(18)), refresh_token_digest: Digest::SHA256.hexdigest(SecureRandom.hex(18)))
end
18 changes: 18 additions & 0 deletions spec/models/api_credential_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -100,4 +100,22 @@
expect(api_credential.refresh_token_digest).to eq(Digest::SHA256.hexdigest(refresh_token))
end
end

describe "#revoke_api_token" do
it "sets api token to nil" do
api_credential.return_new_api_token![:api_token]
api_credential.revoke_api_token

expect(api_credential.api_token_digest).to be_nil
end
end

describe "#revoke_refresh_token" do
it "sets refresh token to nil" do
api_credential.return_new_refresh_token![:refresh_token]
api_credential.revoke_refresh_token

expect(api_credential.refresh_token_digest).to be_nil
end
end
end
36 changes: 33 additions & 3 deletions spec/requests/api/v1/users/sessions_spec.rb
Original file line number Diff line number Diff line change
@@ -1,6 +1,9 @@
require "swagger_helper"

RSpec.describe "sessions API", type: :request do
let(:casa_org) { create(:casa_org) }
let(:volunteer) { create(:volunteer, casa_org: casa_org) }

path "/api/v1/users/sign_in" do
post "Signs in a user" do
tags "Sessions"
Expand All @@ -15,9 +18,6 @@
required: %w[email password]
}

let(:casa_org) { create(:casa_org) }
let(:volunteer) { create(:volunteer, casa_org: casa_org) }

response "201", "user signed in" do
let(:user) { {email: volunteer.email, password: volunteer.password} }
schema "$ref" => "#/components/schemas/login_success"
Expand All @@ -41,4 +41,34 @@
end
end
end

path "/api/v1/users/sign_out" do
delete "Signs out a user" do
tags "Sessions"
produces "application/json"
parameter name: :authorization, in: :header, type: :string, required: true

let(:refresh_token) { create(:api_credential, user: volunteer).return_new_refresh_token![:refresh_token] }

response "200", "user signed out" do
let(:authorization) { "Bearer #{refresh_token}" }
schema "$ref" => "#/components/schemas/sign_out"
run_test! do |response|
expect(response.content_type).to eq("application/json; charset=utf-8")
expect(response.body).to eq({message: "Signed out successfully."}.to_json)
expect(response.status).to eq(200)
end
end

response "401", "unauthorized" do
let(:authorization) { "Bearer foo" }
schema "$ref" => "#/components/schemas/sign_out"
run_test! do |response|
expect(response.content_type).to eq("application/json; charset=utf-8")
expect(response.body).to eq({message: "An error occured when signing out."}.to_json)
expect(response.status).to eq(401)
end
end
end
end
end
6 changes: 6 additions & 0 deletions spec/swagger_helper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,12 @@
properties: {
message: {type: :string}
}
},
sign_out: {
type: :object,
properties: {
message: {type: :string}
}
}
}
},
Expand Down
31 changes: 30 additions & 1 deletion swagger/v1/swagger.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ components:
type: string
email:
type: string
api_token_expires_at:
token_expires_at:
type: datetime
refresh_token_expires_at:
type: datetime
Expand All @@ -28,6 +28,11 @@ components:
properties:
message:
type: string
sign_out:
type: object
properties:
message:
type: string
paths:
"/api/v1/users/sign_in":
post:
Expand Down Expand Up @@ -61,6 +66,30 @@ paths:
required:
- email
- password
"/api/v1/users/sign_out":
delete:
summary: Signs out a user
tags:
- Sessions
parameters:
- name: Authorization
in: header
required: true
schema:
type: string
responses:
'200':
description: user signed out
content:
application/json:
schema:
"$ref": "#/components/schemas/sign_out"
'401':
description: unauthorized
content:
application/json:
schema:
"$ref": "#/components/schemas/sign_out"
servers:
- url: https://{defaultHost}
variables:
Expand Down