Upgrade devise, devise-two-factor, and attr_encrypted

Devise (3.5.4 => 4.1.1) Changelog: https://github.com/plataformatec/devise/blob/master/CHANGELOG.md

devise-two-factor (2.0.1 => 3.0.0) Changelog: https://github.com/tinfoil/devise-two-factor/blob/master/CHANGELOG.md

attr_encrypted (1.3.4 => 3.0.1) Changelog: https://github.com/attr-encrypted/attr_encrypted/blob/master/CHANGELOG.md

Devise 4 includes support for Rails 5, working towards #14286 (moved). devise-async doesn't support Devise 4.0 and in 4.1 the bug that was blocking using Devise's built-in ActiveJob integration was fixed. So devise-async is removed. devise-two-factor 3.0.0 is required for Devise 4 support.

attr_encrypted and encryptor are optional but recommended upgrades for devise-two-factor 3.0.0. The mode and algorithm will need to be changed in order to update to attr_encrypted 4.x in the future.

cc: @rspeicher

Added 1 commit:

• dd16cc48 - Fix a failing test

Added 57 commits:

• dd16cc48...fa7a682b - 54 commits from branch gitlab-org:master
• b3887bef - Upgrade devise and devise-two-factor, remove devise-async
• 0c77ed7e - Upgrade attr_encrypted and encryptor
• 24786b96 - Fix a failing test

Added 1 commit:

• c75abca9 - Upgrade attr_encrypted and encryptor

Added 1 commit:

• bf26c720 - Upgrade attr_encrypted and encryptor

mentioned in issue #14286 (moved)

Added 1 commit:

• 9f42af78 - Fix a broken spec

Reassigned to @rspeicher

Added 455 commits:

• 9f42af78...de20bd5b - 452 commits from branch gitlab-org:master
• d47b2b92 - Upgrade devise and devise-two-factor, remove devise-async
• d287315d - Upgrade attr_encrypted and encryptor
• 5647fb14 - Fix a broken spec

pokes @rspeicher

@connorshea What's the reasoning for using this algorithm over the default aes-256-gcm?

aes-256-cbc was the previous default and used by devise-two-factor to encrypt existing 2FA data. If we change to aes-256-gcm, the user would have to take the app offline, and decrypt then re-encrypt all encrypted info using the new cipher. Either that or all 2FA codes and any other encrypted data become non-decryptable.

Ok, good to know.

What about https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/4027?

That issue was fixed by Devise 4.1.0, see https://github.com/plataformatec/devise/issues/3550

Did this raise a different error when it was nil?

With 3.0, attr_encrypted needs an encryption key with sufficient length or it will cause an error. nil had insufficient length, so it caused a different error, yeah.

I'm a little concerned about this note in the changelog for 2.0.0:

Deprecated: :single_iv_and_salt and :per_attribute_iv_and_salt modes are deprecated and will be removed in the next major release.

At 3.0, we're now on the "next major release" but there's no further mention of this.

3.0 was released to fix the security vulnerability in 2.0, so my understanding is that that was delayed to 4.0.

I was surprised not to see any changes to config/locales/devise.en.yml from the update. Can we make sure that's updated and maybe we can override this error message there? This casing is weird.

I'm already on it :) Relevant issue:

https://github.com/plataformatec/devise/issues/4095

There's nothing we can do from our side that'll fix this since they humanize the keys using Ruby (https://github.com/plataformatec/devise/pull/4014/files#diff-c1be825bdb5f3160081e41432f83d0d7R106). We could monkeypatch it, but I'd prefer to fix it upstream instead.

@rspeicher I don't think the grammar issue warrants blocking this MR, do my comments address all your concerns?

Fixed

Ok, I tried this locally with an account that already had 2FA enabled and both the OTP and the backup codes worked without issue, so I think we're good.

Thanks @connorshea

Status changed to merged

mentioned in commit 7d33fba7