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.

Sign up for GitHub

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

Jump to bottom

Add new Lint/DeprecatedOpenSSLConstant cop #7950

Merged
merged 1 commit into from
May 14, 2020
Merged

Add new Lint/DeprecatedOpenSSLConstant cop #7950

merged 1 commit into from
May 14, 2020

Conversation

bdewater
Copy link
Contributor

@bdewater bdewater commented May 10, 2020 •
edited
Loading

With ruby/openssl#366 the algorithm constants for OpenSSL::Cipher and OpenSSL::Digest are planned to be deprecated, see ruby/openssl#304 for reasoning. This PR aims to introduce two cops to autocorrect this syntax, similar to e.g. Lint/BigDecimalNew.

Before submitting the PR make sure the following are checked:

  • Wrote good commit messages.
  • Commit message starts with [Fix #issue-number] (if the related issue exists).
  • Feature branch is up-to-date with master (if not - rebase it).
  • Squashed related commits together.
  • Added tests.
  • Added an entry to the Changelog if the new code introduces user-observable changes. See changelog entry format.
  • The PR relates to only one subject with a clear title and description in grammatically correct, complete sentences.
  • Run bundle exec rake default. It executes all tests and RuboCop for itself, and generates the documentation.

@bdewater bdewater force-pushed the openssl-constants branch from b30794d to 0531c1c Compare May 10, 2020 22:00
koic
koic reviewed May 10, 2020
View reviewed changes
config/default.yml Outdated Show resolved Hide resolved
koic
koic reviewed May 10, 2020
View reviewed changes
config/default.yml Outdated Show resolved Hide resolved
koic
koic reviewed May 10, 2020
View reviewed changes
config/default.yml Outdated Show resolved Hide resolved
@bdewater bdewater force-pushed the openssl-constants branch from 0531c1c to 1c95752 Compare May 11, 2020 01:00
@bdewater
Copy link
Contributor Author

Thanks for the review @koic!

@bdewater bdewater changed the title Add new Style/OpenSSLDigestConstant cop Add new Lint/OpenSSLDigestConstant cop May 11, 2020
@bdewater bdewater changed the title Add new Lint/OpenSSLDigestConstant cop Add new Lint/OpenSSLDigestConstant and Lint/OpenSSLCipherConstant cop May 11, 2020
@bdewater bdewater marked this pull request as ready for review May 11, 2020 03:30
@koic
Copy link
Member

koic commented May 11, 2020

I think Lint/OpenSSLCipherConstant and Lint/OpenSSLDigestConstantcops have the same check purpose, and they are enabled or disabled together. Also, the implementation is almost the same.
So I think it can be unified as Lint/DeprecatedOpenSSLConstant cop.

@bbatsov
Copy link
Collaborator

bbatsov commented May 11, 2020

We can even go one step further and have a Lint/DeprecatedConstants cop. I don't think we need a different cop for each deprecated constant. I think we already have something similar for methods.

@bdewater bdewater force-pushed the openssl-constants branch from af6e2e6 to 1a43961 Compare May 13, 2020 02:08
@bdewater bdewater changed the title Add new Lint/OpenSSLDigestConstant and Lint/OpenSSLCipherConstant cop Add new Lint/OpenSSLDeprecatedConstant cop May 13, 2020
@bdewater bdewater force-pushed the openssl-constants branch 2 times, most recently from 585a5d3 to 8575679 Compare May 13, 2020 02:28
@bdewater
Copy link
Contributor Author

Merged into one cop. A Lint/DeprecatedConstants cop like Lint/DeprecatedClassMethods is not going to work for the rewrite logic here I think 🤔

@rhenium
Copy link

rhenium commented May 13, 2020

OpenSSL::Cipher::AES{128,192,256} have another initialization form - The cipher mode could be omitted for just these three constants.

OpenSSL::Cipher::AES128.new

needs to be auto-corrected into:

OpenSSL::Cipher.new("aes-128-cbc")

@bdewater bdewater force-pushed the openssl-constants branch from 8575679 to 94b8331 Compare May 14, 2020 02:00
@bdewater
Copy link
Contributor Author

Thanks @rhenium! I pushed a fix.

@bdewater bdewater changed the title Add new Lint/OpenSSLDeprecatedConstant cop Add new Lint/DeprecatedOpenSSLConstant cop May 14, 2020
@bdewater bdewater force-pushed the openssl-constants branch from 94b8331 to 537e620 Compare May 14, 2020 02:11
@bdewater bdewater force-pushed the openssl-constants branch from 537e620 to d182466 Compare May 14, 2020 02:28
@bbatsov bbatsov merged commit 0052dae into rubocop:master May 14, 2020
@bbatsov
Copy link
Collaborator

bbatsov commented May 14, 2020

Thanks!

@bdewater bdewater deleted the openssl-constants branch May 14, 2020 14:06
This was referenced May 15, 2020
Closed
Closed
Open
Open
Closed
Open
@tas50
Copy link
Contributor

tas50 commented May 15, 2020

Thanks for cutting a cop for this deprecation @bdewater. It makes scoping Ruby upgrades a lot more predictable.

robbkidd pushed a commit to chef/supermarket that referenced this pull request Jun 1, 2020
@tas50 @robbkidd
resolve openssl deprecation, Rubocop: Lint/DeprecatedOpenSSLConstant
b021488 
> Lint/DeprecatedOpenSSLConstant:
>   Use OpenSSL::Digest.hexdigest('MD5', key_in_der_format) instead of
>   OpenSSL::Digest::MD5.hexdigest(key_in_der_format).

Rubocop has a new cop[1] that detects an upcoming deprecation to the
OpenSSL gem[2] that's built into Ruby.

[1] rubocop/rubocop#7950
[2] ruby/openssl#366

The previous constant will cause failures in Ruby 3.

Signed-off-by: Tim Smith <[email protected]>
Signed-off-by: Robb Kidd <[email protected]>
@tas50 tas50 mentioned this pull request Jun 1, 2020
Merged
This was referenced Jun 28, 2020
Merged
Merged
@vipulnsward vipulnsward mentioned this pull request Jun 28, 2020
Closed
@bdewater bdewater mentioned this pull request Jun 29, 2020
Open
@bdewater bdewater mentioned this pull request Jul 7, 2020
Merged
8 tasks
@bdewater bdewater mentioned this pull request Oct 13, 2020
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@koic koic koic left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@bdewater @koic @bbatsov @rhenium @tas50