Skip to content
This repository has been archived by the owner on Mar 22, 2021. It is now read-only.

Feature/warn when key is empty #261

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Feature/warn when key is empty #261

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2f7dc67
add warning when key is missing
renatamarques97 May 24, 2020
f265719
test for empty key
renatamarques97 May 24, 2020
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
14 changes: 12 additions & 2 deletions lib/knock.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,18 @@ module Knock
self.token_signature_algorithm = "HS256"

# Configure the key used to sign tokens.
mattr_accessor :token_secret_signature_key
self.token_secret_signature_key = -> { Rails.application.secrets.secret_key_base }
mattr_writer :token_secret_signature_key, default: -> { Rails.application.secrets.secret_key_base }

class EmptySecretKey < StandardError
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this class can be moved to a separated file. Perhaps it would be nice to have all project exception classes inside a folder named exceptions or errors

def initialize(msg="Knock secret signature key can't be empty")
super
end
end

def self.token_secret_signature_key
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it would be better raising the error in the assign moment (setter) instead of the reading moment (getter). It's better to prevent setter from accepting a wrong value than accepting it and causing errors when you try to use that value... So, I'd suggest to invert it, creating a method for the setter, and using mattr_reader for the getter...

raise EmptySecretKey unless @@token_secret_signature_key
@@token_secret_signature_key
end

# Configure the public key used to decode tokens, when required.
mattr_accessor :token_public_key
Expand Down
6 changes: 6 additions & 0 deletions test/dummy/test/controllers/protected_resources_controller_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,12 @@ def authenticate token: @token
assert_response :unauthorized
end

test "responds with unauthorized with empty token in header" do
Copy link
Collaborator

Choose a reason for hiding this comment

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

If you follow my tip in the setter/getter thing, this test won't be necessary anymore. Look, we won't simply fail the request with unauthorized response. Instead, it will prevent the host app from loading until a valid token be properly set. I think this way makes the issue explicit and help the devs to fix it faster...

authenticate token: ""
get :index
assert_response :unauthorized
end

test "responds with success with valid token in header" do
authenticate
get :index
Expand Down