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

Rescue specific exceptions in Authenticable #133

Merged
merged 4 commits into from
Mar 15, 2017
Merged

Rescue specific exceptions in Authenticable #133

merged 4 commits into from
Mar 15, 2017

Conversation

aldesantis
Copy link
Contributor

@aldesantis aldesantis commented Jan 2, 2017

Currently, Knock::Authenticable rescues all exceptions when trying to decode the JWT and find the user. This is an anti-pattern: if, for whatever reason, either .from_token_payload or any other piece of the code in the rescue block fails for an unexpected reason, authentication will silently fail.

In the best-case scenario, this means that the developer is left to debug a confusing issue without the required context.

In the worst-case scenario, authentication fails and the developer does not know about it (it should be caught by tests, but not all applications are tested properly, as we all know).

This PR changes Knock::Authenticable#define_current_entity_getter to only rescue ActiveRecord::RecordNotFound and JWT::DecodeError, the two errors which should be handled. All other errors will crash the application, as one would expect.

I have run the existing test suite against the PR and it passes.

@aldesantis
Copy link
Contributor Author

@nsarno have you had a chance to review this? :)

@nsarno
Copy link
Owner

nsarno commented Jan 17, 2017

@alessandro1997 I haven't. I'm way behind on reviewing PR's at the moment. Sorry about that.

@aldesantis
Copy link
Contributor Author

aldesantis commented Jan 17, 2017 via email

@nsarno
Copy link
Owner

nsarno commented Feb 15, 2017

Ideally we don't want to force the dependency on ActiveRecord, so you could replace:

ActiveRecord::RecordNotFound with Knock.not_found_exception_class

see: https://github.com/nsarno/knock/blob/master/lib/knock.rb#L20

@aldesantis
Copy link
Contributor Author

@nsarno fixed! :)

@nsarno
Copy link
Owner

nsarno commented Mar 15, 2017

Thank you @alessandro1997 👍

@nsarno nsarno merged commit 26c9090 into nsarno:master Mar 15, 2017
@aldesantis aldesantis deleted the specific-rescue branch March 28, 2017 21:00
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants