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

Remove dependency on SCrypt #679

Merged
merged 2 commits into from
Sep 26, 2019
Merged

Remove dependency on SCrypt #679

merged 2 commits into from
Sep 26, 2019

Conversation

jaredbeck
Copy link
Collaborator

In Authlogic 5, we require everyone to install the scrypt gem, even if they only use eg. bcrypt. I propose removing the runtime dependency, and requiring everyone to explicitly specify their crypto_provider.

Besides fixing the obvious problem of forcing people to install a gem they don't use, I believe this change will also fix a rare RuntimeError.

Obviously, this is a breaking change, which would require us bumping the major version to 6.

Fixes #668, closes #669 (thanks for your great work on those @mihael, I hope you like this solution even better)

@jaredbeck jaredbeck requested a review from tiegz September 8, 2019 02:47
@jaredbeck

This comment has been minimized.

@jaredbeck

This comment has been minimized.

# Authlogic 6, the default was `CryptoProviders::SCrypt`. If you try
# to read this config option before setting it, it will raise a
# `NilCryptoProvider` error. See that error's message for further
# details, and rationale for this change.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we raise an error on boot instead of lazily? Seems more certain that people will see it.

@jaredbeck
Copy link
Collaborator Author

Should we raise an error on boot instead of lazily? Seems more certain that people will see it.

I don't think we can raise this error any earlier. We can't raise it until eg. user.rb has been loaded. In the dev. env., that doesn't necessarily happen at boot time.

Other than the error timing, are you OK with this breaking change?

Thanks again for the quick review.

1. Errors should share a parent class Authlogic::Error
2. Having them in one file is kind of convenient because they
  often have long messages, and it's nice to keep that copywriting
  out of the way. Also, it's sort of nice if a file like
  rails_adapter.rb only defines the adapter class and not any other
  classes?
@jaredbeck
Copy link
Collaborator Author

Should we raise an error on boot instead of lazily? Seems more certain that people will see it.

Oh, we could raise on write (during crypto_provider=) instead of on read (during crypto_provider). That's probably what you meant, I'll look into that.

[Fixes #668]

See changelog for description, rationale.
@jaredbeck jaredbeck merged commit 8e54152 into master Sep 26, 2019
@jaredbeck jaredbeck deleted the remove_dependency_on_scrypt branch September 26, 2019 19:56
@jaredbeck
Copy link
Collaborator Author

Released as 6.0.0

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.

2 participants