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

Accept Rails 7.0.0 #204

Merged
merged 1 commit into from
Feb 23, 2022
Merged

Accept Rails 7.0.0 #204

merged 1 commit into from
Feb 23, 2022

Conversation

kivanio
Copy link
Contributor

@kivanio kivanio commented Dec 16, 2021

No description provided.

@bsedat
Copy link
Member

bsedat commented Dec 16, 2021

Thank you!

@bsedat
Copy link
Member

bsedat commented Dec 16, 2021

Looks like Rails 7.0 requires Ruby 2.7 and above.

.github/workflows/ci.yml Show resolved Hide resolved
@andyjeffries
Copy link

I get an error when using devise-two-factor and upgrading an app to Rails 7.0. I tried with the version from this PR and the error persists.

$ rails db:migrate --trace
** Invoke db:migrate (first_time)
** Invoke db:load_config (first_time)
** Invoke environment (first_time)
** Execute environment
rails aborted!
NoMethodError: undefined method `has_key?' for nil:NilClass
/Users/andy/.rbenv/versions/2.7.1/lib/ruby/gems/2.7.0/gems/attr_encrypted-3.1.0/lib/attr_encrypted.rb:224:in `attr_encrypted?'
/Users/andy/.rbenv/versions/2.7.1/lib/ruby/gems/2.7.0/bundler/gems/devise-two-factor-f5e97fad4dc5/lib/devise_two_factor/models/two_factor_authenticatable.rb:17:in `block in <module:TwoFactorAuthenticatable>'

This eventually comes from app/models/user.rb:2 in my app which is:

devise :two_factor_authenticatable, :two_factor_backupable, otp_secret_encryption_key: ENV['OTP_KEY']

I don't know what the solution is from here, but the attr_encrypted latest version is 3.1.0 from 2018. I don't know if the use of that gem should be replaced with the default Rails 7.0 solution (or how we'd migrate data in existing encrypted columns to it).

@mitsuru
Copy link

mitsuru commented Dec 27, 2021

https://github.com/tinfoil/devise-two-factor/blob/ecd182e97e8e92436bd908af30165c18f17fe67e/lib/devise_two_factor/models/two_factor_authenticatable.rb#L10-L22

As long as opt_secret andotp_secret =are defined before loading the module,
so before trying Rails 7,I tried define encrypts: opt_secret provided by lockbox before the devise: two_factor_authenticatable.

encrypts :otp_secret
devise :two_factor_authenticatable, ...

After that, I upgraded to Rails 7 with this PR and rewrite encrypts to lockbox_encrypts for Rails 7.

lockbox_encrypts :otp_secret
devise :two_factor_authenticatable, ...

It worked correctly.

You need to take steps before upgrading to rails 7,
lockbox is well documented for migration from attr_encrypted.
https://github.com/ankane/lockbox#migrating-from-another-library

I propose decoupling otp_secret encryption from devise-two-factor and allowing Rails 7's default encrypts or lockbox to be selected.

@loust333
Copy link

Hello everyone,

Any update concerning this issue ?

@andyjeffries
Copy link

I haven't got back to upgrading that site yet due to a vacation over Xmas/NY. I'm hoping I'll have another chance this weekend and will let you know.

@loust333
Copy link

Thanks for the update @andyjeffries.

I will try out the changes of this issue and see if there is something else that comes up. :)

Have a nice week!

@jason-hobbs
Copy link

andyjeffries

I get this error as well after going to Rails 7. It is the only thing keeping me from upgrading fully. Is there any ETA for a fix or what can any of us do to help?

@jason-hobbs
Copy link

jason-hobbs commented Jan 25, 2022

I installed the lockbox gem and added the lockbox_encrypts :otp_secret
The server now starts but when I try to login I get:
NoMethodError - undefined method `otp_secret_ciphertext'

I can login as a user without 2fa enabled of course but I can't login with a user that has 2fa enabled and can't enable 2fa for a user.

@jason-hobbs
Copy link

I actually ended up using the branch from cybersecuricy here #192 and it works perfectly.

@andyjeffries
Copy link

Agree, that one works perfectly for me too. Thanks @jason-hobbs

@burkematthew
Copy link

Just checking in on what the next steps are here. This is one of the gems holding me back from upgrading to Rails 7.

@webrails
Copy link

webrails commented Feb 4, 2022

I also tested @cybersecuricy's approach. Seems to work just fine after migrating encrypted_otp_secret to otp_secret.

@DerekCrosson
Copy link

For now add the gem like this:

gem 'devise-two-factor', git: 'git://github.com/cybersecuricy/devise-two-factor.git', branch: 'securicy-fixes-rails-7'

And then bundle install, then carry on from where you got the error 🙂

@LesterKim
Copy link

@bsedat Does this look good to merge?

@bsedat bsedat merged commit 4a1468e into devise-two-factor:main Feb 23, 2022
@LesterKim
Copy link

@bsedat Thank you. Could we add a version tag as well?

@andyjeffries
Copy link

I don't know about anyone else, but seeing that this was merged, I switched back to the mainline version so that I can get 4.0.1 to fix CVE-2021-43177 on Rails 7.0.2.3, but it now breaks again for me.

If I run rspec in my codebase after switching, I get:

Failure/Error: devise :two_factor_authenticatable, :two_factor_backupable, otp_secret_encryption_key: ENV['OTP_KEY']

NoMethodError:
  undefined method `has_key?' for nil:NilClass

      encrypted_attributes.has_key?(attribute.to_sym)
                          ^^^^^^^^^
# ./app/models/user.rb:2:in `<class:User>'
# ./app/models/user.rb:1:in `<main>'

If I switch back to cybersecuricy's branch it works fine (but my CI run fails due to the CVE). So it looks like there's some fix on their branch that still isn't merged here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants