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 6 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all 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
  ...

# 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
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice.

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

Would this make hashed values of refresh_token and refresh_token_digest different? Since your using a completely new SecureRandom.hex(18). If so, this should not be the case.

It should hash the current refresh_token.

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
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍

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