Refactor Argon2::Password

[joshbuker]

This is still a WIP, but wanted to open a draft so we can start discussing the potential changes @technion.

Edit: Refactor complete, test suite and rubocop passing, and code coverage/documentation are both at 100%.

[joshbuker]

I'll give it another look when I do my second pass and start building out the documentation/test suite, but it looks like the only breaking changes that will require user action are the following:

Migrate all instances of:

argon2 = Argon2::Password(m_cost: x, t_cost: y)
final = argon2.create(password)

To:

final = Argon2::Password.create(password, m_cost: x, t_cost: y)

And all instances of:

Argon2::ArgonHashFail

To:

Argon2::Error

Everything else should still work as-is.

[technion]

As per my statement on the bcrypt-ruby repo, I'm really hesitant to take on a breaking change out of some sort of "it looks better". I sure don't mind adding utility functions to help out in the ways we've previously discussed. But a new API has to really offer something more and to be honest I'm not seeing it. Argon2 users already went through a lot when the original spec split off into Argon2i and Argon2d and 2id. There's real value in stability at this point. It's also harder to reevaluate when the whole test suite appears to have been rewritten.

I don't understand why all these line breaks were added.

[joshbuker]

As per my statement on the bcrypt-ruby repo, I'm really hesitant to take on a breaking change out of some sort of "it looks better". I sure don't mind adding utility functions to help out in the ways we've previously discussed. But a new API has to really offer something more and to be honest I'm not seeing it.

The API changes to Argon2::Password are more than "it looks better." It's required to properly expose a Ruby object that has the metadata that downstream developers would otherwise have to write, and risk getting wrong. It also requires very little effort from the downstream dev to migrate, with no new functionality that they are required to grok if they just want to be on the latest and have it not break. For the password interface, the only changes they would need are:

# Take instances where we abstract creating the password by first exposing an Object instance
instance = Argon2::Password.new(m_cost: some_m_cost)
instance.create(input_password)

# And remove the abstraction step
Argon2::Password.create(input_password, m_cost: some_m_cost)

Renaming Argon2::ArgonHashFail to Argon2::Error allows us to nest more specific errors, which helps developers by allowing them to catch only a specific error if a use-case warrants it. It's also an incredibly simple replacement for downstream developers, a single find_all/replace.

# Find any instances of Argon2::ArgonHashFail, for example...
def login(username, password)
  [...]
rescue Argon2::ArgonHashFail
  [...]
end

# And do a straight 1:1 replacement
def login(username, password)
  [...]
rescue Argon2::Error
  [...]
end

It's also harder to reevaluate when the whole test suite appears to have been rewritten.

Honestly, this is a really good point. There isn't a need to bundle changing the test suite with this refactor as there's already concerns about breaking changes, and replacing the test suite doesn't exactly make that less scary. I'll revert the test suite changes that so that this is easier to grok and hopefully alleviate some of your concerns.

I'll also do this as two commits so you can view the second one and see the changes I need to make to the suite to get it passing. It should be representative of a downstream dev pulling 3.0.0.

don't understand why all these line breaks were added.

I assume this comment was intended for the README.md changes. The line breaks don't cause any functional differences (Github treats a single linebreak as being the same line when rendering) but keeps the file itself within the standard 80 characters line length limit. This helps readability when viewing the file locally, without any changes to the final formatting.

I can revert that change if it's bothersome, it's just a QOL change I do out of habit when reading a file that I committed alongside the other changes.

[joshbuker]

@technion I've tried to reduce the styling changes as much as possible. I've left some styling changes related to the yard docs being broken, but hopefully this should reduce your workload reviewing.

[joshbuker]

Just finished updating the documentation as well, yard is reporting 100% coverage locally.

With the code and documentation up-to-date, I'll mark the PR as officially ready for review.

[joshbuker]

I'm not totally sure how exposing this works. I made them all nil | Type to be safe, but technically every single one of these will be set in the initializer without exception. If it's okay with them being nil during the initialization, we could make these all hard return values (Type only / remove the ?)

[joshbuker]

@technion do you know if this is because my machine doesn't have enough memory, or is the MAX_M_COST currently above the Argon2 actual max memory cost? I crunched the numbers and I think it's only slightly above 2GB, so you'd think this should work as-is.

[coveralls]

Coverage decreased (-100.0%) to 0.0% when pulling ed99f4f on athix:feature/additional-helpers into 6f74bca on technion:master.

[joshbuker]

@technion Test suite and rubocop passing, documentation and code coverage at 100%.

Please let me know if there's any questions or concerns I can address. I'll add some additional tests later when I have time.

[joshbuker]

I have no idea why Coveralls is reporting 0% coverage at this point. Locally it still reports 100%, and even forcing a rebuild with a previously known-good commit causes it to fail. I'm guessing there's some issue on the coveralls side that's causing it to no longer see the coverage file, e.g. permissions were revoked.

Not really sure what I can do on my end to fix that. 🤷‍♂️

@technion It's been over a week without even a "I'm too busy to review this right now". I don't want to waste my time trying to push this upstream if there's no interest in it. Should I just use my fork and close this PR?

[technion]

I have indicated in several other places I'm opposed to breaking changes without a significant need, and generally opposed to "refactoring" a security library without a clear reason. Ruby's bcrypt gem has 296 commits across its lifetime. If you've entertained a fork to service your own needs I'd encourage you to go down that direction.

[joshbuker]

For those that wish to use the proposed changes, please see the sorcery-argon2 gem: Sorcery/argon2

It will be kept up-to-date with this upstream, with the addition of the new Argon2::Password interface.

[joshbuker]

@technion I assume you won't change your mind, but if you ever do I'm more than willing to help merge these changes back upstream.

I don't want to fragment implementations any more than is strictly necessary.