Password#password_changed? returns true when transitioning crypto providers

[pduey]

• [x] This is not a usage question.
• [x] This is not a security issue.
• [x] I am committed to implementing this feature in a reasonable amount of time, and responding promptly to feedback.

Current Behavior

Upgraded from authlogic 3 (to 5.1.0), added:

c.transition_from_crypto_providers = [Authlogic::CryptoProviders::Sha512]
c.crypto_provider = Authlogic::CryptoProviders::SCrypt

Now, whenever a user signs in with their existing password it gets rehashed using SCrypt, however the password_changed? method returns true, even though the password (what the user enters) has not really changed, only the crypted password and salt.

In my app, certain events are triggered if the password has changed. Specifically, a password changed notification. Now my users are confused.

I added this as a feature, because I guess it could be argued it's not a bug, per se.

Proposed Behavior

If the only change is rehashing the password, password_changed? should return false.

Proposed Solution

          def transition_password(attempted_password)
            self.password = attempted_password
+           @password_changed = false
            save(validate: false)
          end

Password#password= sets @password_changed = true, so this would override that.

Alternatively, a new accessor called password_transitioned could be added so the current behavior is unchanged, then I can include that in my conditions.

Personally, I can't think of a case where I'd expect password_changed? to return true unless the user actually changed their original password. If I want to know if the encrypted password changed, I could use the active record generated attribute method, so to me just plain password means the value that the user enters.

[jaredbeck]

Hi Paul, I'm glad to hear you're moving from SHA-512 to SCrypt. Good choice.

If the only change is rehashing the password, password_changed? should return false.

I agree. password_changed? is (should be!) defined by ActiveModel::Dirty, not by Authlogic, right? Or am I thinking of an older version of rails?

Upgraded from authlogic 3 (to 5.1.0) ..

Just for the record: I'd strongly recommend upgrading one major version at a time, and carefully reading the changelog for each. I'd also strongly discourage upgrading and changing one's hashing algorithm at the same time.

Proposed Solution .. [in transition_password, set] .. @password_changed = false

I wonder if @password_changed is (should be) an instance variable "owned" by ActiveModel::Dirty, and maybe we should not be using it? Could be wrong, just thinking out loud ..

Can you please start a draft PR with a test that reproduces the issue?

[pduey]

Hmmm... To ActiveModel, password is a "virtual" or "transient" attribute, i.e., not persisted, so it's dirty by definition. It does have it's encrypted representation that is persisted, but ActiveModel doesn't know about that. I suspect it would be a lot of shoehorning into ActiveModel for the one attribute.

I also use the attr_encrypted gem, which does do exactly that, but in that case it makes sense since it's generalized and the goal of the gem is to provider transparent encryption on any attribute.

Good point about incremental changes. I could/should have left out the part about upgrading, since this is really about transitioning crypto providers.

Yes I will submit a failing test.

[pduey]

I've submitted the draft PR.

[pduey]

Bump. Can this go forward?