Feature/warn when key is empty #261
Conversation
renatamarques97
changed the title
There was a problem hiding this comment.
Thank you for this @renatamarques97 !
I've just merged another PR which now conflicts with this one. Let me know if you can fix it and I will merge.
|
sure |
| 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 |
There was a problem hiding this comment.
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
| end | ||
| end | ||
|
|
||
| def self.token_secret_signature_key |
There was a problem hiding this comment.
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...
| @@ -15,6 +15,12 @@ def authenticate token: @token | |||
| assert_response :unauthorized | |||
| end | |||
|
|
|||
| test "responds with unauthorized with empty token in header" do | |||
There was a problem hiding this comment.
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...
resolves #212
resolves #257