Handle authentic check when secret is nil

[FinnLawrence]

Issue

Calling google_authentic?(code) on an ActiveRecord model with a nil google_secret throws an error:

NoMethodError: undefined method 'tr' for nil:NilClass

Solution

For now I've written a spec to illustrate the failure - added next to existing tests for secrets being nil.

@jaredonline let me know if you have a preference on how to handle this - it does look like the root cause of the exception is in the rotp gem but I think it's worth adding a check/solution here. Couple of options:

1. Add return false unless secret.present? to line 57 of lib/google-authenticator-rails.rb. If this is preferred, does this check need to be added anywhere else?
2. Add return false unless google_secret_value.present? to line 31 of lib/google-authenticator-rails/active_record/helpers.rb
3. Instead of returning false, raise an exception in one of these places - maybe ArgumentError or similar? Right now the error isn't helpful in diagnosing why the call to google_authentic? fails.

Let me know if you have a preference and I can add it in, cheers.

[jaredonline]

Oh wow; I'm surprised its taken this long for this to come up. Thanks for catching this @FinnLawrence

I can't really reason about what it means to try and validate a secret when there is no secret which, to me, suggests that we should raise in this case.

I imagine GoogleAuthenticatorRails.valid? should raise when secret is nil (or any other invalid value, if there are some).

ActiveRecord::Helpers.google_authentic? could also raise when google_secret_value is nil, if we felt like being extra caeful.

[jaredonline]

What do you think?

[FinnLawrence]

Thanks @jaredonline agree on the exception vs returning false.

For me, I just had trouble when calling google_authentic?(code) and getting an error message that didn't make sense. Initially I thought it was to do with the code I supplied. So, I've added it to the google_authentic? method directly since it's also the only place (?) that calls GoogleAuthenticatorRails.valid? by the looks of it. It feels more natural to me having the exception in the code that's actually included in my Active Record model.

I've also added a spec/support for passing in the code as an integer (since this is how the readme illustrates it).

Let me know if you think I should make any other tweaks/changes here 😎

[jaredonline]

@FinnLawrence just released 3.2.1 with your changes. Thank you!