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

NoMethodError: undefined method `attr_encrypted' #21

Open
richardonrails opened this issue Nov 10, 2019 · 7 comments · May be fixed by #25
Open

NoMethodError: undefined method `attr_encrypted' #21

richardonrails opened this issue Nov 10, 2019 · 7 comments · May be fixed by #25
Labels

Comments

@richardonrails
Copy link

Just trying out your gem since from the documentation it seems a lot more straightforward than some of the existing Devise 2FA/OTP gems (mentioned in #20)

First issue I ran into is this:

$ rake db:migrate
rake aborted!
NoMethodError: undefined method `attr_encrypted' for User (call 'User.connection' to establish a connection):Class
Did you mean?  attr_readonly

I'm using Rails 6 with a pretty standard Devise setup.

Does attr_encrypted need to be added as a dependency? Even when I include that in my gemfile and try to re-run the migration then I get a different error (`ArgumentError: Unknown validator: 'SymmetricEncryptionValidator')

@aspencer8111
Copy link

aspencer8111 commented Nov 22, 2019

Getting the exact same behaviour on our Rails 5.2 app. Adding attr_encrypted gem got us past that error, but now battling the SymmetricEncryptionValidator error. Will post here if I find anything (and submit a patch of course)

Update: Adding the symmetric-encryption gem to my Gemfile and bundling fixed the SymmetricEncryptionValidator. Seems like a PR to this gem's .gemspec file is in order. Will do when I find some free time.

@williamatodd
Copy link
Owner

@aspencer8111 what did you have in mind for the .gemspec? I've added a generic 'SymmetricEncryption must be configured prior to using this gem.' requirement in #22, but please let me know if you have other suggestions.

@aspencer8111
Copy link

@williamatodd - PR #22 looks like a great way of dealing with it. My only other idea would be to add the symmetric-encryption gem to the dependencies. But that is your call to make 😄

@williamatodd
Copy link
Owner

@aspencer8111 I haven't looked at the docs for a long time, but I think runtime-dependency is just an alias for dependency.

@cgunther
Copy link

I think the issue is that even though the gemspec has a dependency on symmetric-encryption, I don't believe Rails/Bundler will require/load transitive dependencies like that by default. When you add devise-2fa to your Gemfile, Rails/Bundler will require that specific gem, but then it's up to that gem to require it's dependencies.

For example, rotp is required here:

rqrcode is required here:

However, symmetric-encryption is never required. It is, however, required by the dummy app in the spec directory, which is probably why the spec's don't have this issue:

require 'symmetric_encryption'

If someone interprets "Setup the symmetric-encryption gem" in the readme as adding that gem directly to your own Gemfile, Rails/Bundler will require it, circumventing the problem, but I think it's best this gem be the one that requires symmetric-encryption.

I don't think #22 would actually fix this issue since it's not addressing symmetric-encryption not being required anywhere. I'd imagine you'd just see a different error about Rails not recognizing the :encrypted type of attribute.

@williamatodd
Copy link
Owner

@cgunther You're welcome to propose a fix either in the docs or otherwise. My personal inclination is to come up with a mechanism to enable the use of different encryption adapters, or even none at all should a user desire. For example, I'm using Lockbox on a new project and I'd like the option of using that instead of Symmetric Encryption but I cannot break functionality to accomplish that goal.

@cgunther
Copy link

Gotcha.

My thinking then would be the docs should treat encryption as an optional/suggested upgrade as opposed to a prerequisite/requirement, then the generator should likely not assume any encryption, using out-of-the-box Rails instead. I'll work up a PR to better codify what I'm thinking.

Given the gem is pre-1.0, I think there's more flexibility to introduce a breaking change with proper documentation in the changelog/readme as necessary. Plus, my hunch is that for anyone who's already set up symmetric-encryption to work with this gem (or another encryption strategy), very little would change as this gem itself doesn't seem to rely directly on encryption, it just tries to set you up with encryption on a new install as a good default.

cgunther added a commit to cgunther/devise-2fa that referenced this issue Feb 16, 2021
Previously it was assumed you were using symmetric-encryption to encrypt
the secrets in the database for extra security, however the gem itself
wasn't actually required, leading to trouble setting things up for the
first time.

Now encryption is treated as an optional added layer of security with
instructions on how to accomplish it. This should also open the door to
using alternate encryption strategies.

This should fix williamatodd#21.
cgunther added a commit to cgunther/devise-2fa that referenced this issue Feb 16, 2021
Previously it was assumed you were using symmetric-encryption to encrypt
the secrets in the database for extra security, however the gem itself
wasn't actually required, leading to trouble setting things up for the
first time.

Now encryption is treated as an optional added layer of security with
instructions on how to accomplish it. This should also open the door to
using alternate encryption strategies.

This should fix williamatodd#21.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants