Conversation
…v2 (#6) * 🐛 Fix compatibility with omniauth v1 & v2, rack v2 & v3, rack-session v1 & v2 - Ref: rack/rack-session#26 * 👷 Add ancient build for Ruby 2.2 & 2.3 * 💚 Better handling of ancient ruby scenario * 💚 Ancient ruby support * ✨ Support Ruby 2.2 & 2.3
Updated security contact information and support details.
There was a problem hiding this comment.
Pull request overview
This PR adds compatibility support for multiple versions of key dependencies (omniauth v1 & v2, rack v2 & v3, rack-session v1 & v2) and extends Ruby support to include ancient versions 2.2 and 2.3. The gem is being renamed from discourse-omniauth-jwt to omniauth-jwt2 as part of a fork/maintenance takeover.
Changes:
- Added compatibility shims for ancient Ruby versions (2.2 & 2.3) with graceful fallbacks for missing dependencies
- Updated gem name and version structure to support version_gem integration
- Refactored code style throughout (double quotes, spacing, etc.) to follow modern Ruby conventions
Reviewed changes
Copilot reviewed 32 out of 35 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| lib/omniauth/strategies/jwt.rb | Core strategy implementation with style updates and compatibility changes for .compact replacement |
| lib/omniauth/jwt/version.rb | Restructured version constant into module for version_gem integration |
| lib/omniauth/jwt.rb | Added version_gem dependency and integration |
| spec/spec_helper.rb | Added conditional requires with LoadError rescue for Ruby 2.2/2.3 compatibility |
| spec/support/next_instance_of.rb | Added ArgumentError rescue for Ruby < 2.7 keyword argument compatibility |
| spec/support/hash.rb | Style formatting update |
| spec/lib/omniauth/strategies/jwt_spec.rb | Comprehensive style updates and ancient Ruby compatibility checks |
| omniauth-jwt2.gemspec | New gemspec file replacing discourse-omniauth-jwt.gemspec |
| discourse-omniauth-jwt.gemspec | Removed old gemspec |
| gemfiles/*.gemfile | New gemfile configurations for different Ruby version test scenarios |
| .github/workflows/*.yml | Added new CI workflows for legacy and ancient Ruby support |
| README.md | Updated documentation with new gem name and history section |
| SECURITY.md | Added security policy document |
| Rakefile | Added conditional task loading for optional dependencies |
| Other config files | Various tooling and configuration updates |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| option :jwks_loader | ||
| option :algorithm, 'HS256' # overridden by options.decode_options[:algorithms] | ||
| option :algorithm, "HS256" # overridden by options.decode_options[:algorithms] | ||
| option :decode_options, {} |
There was a problem hiding this comment.
The option :decode_options is defined twice (lines 16 and 19). This is redundant and could cause confusion. Remove the duplicate definition.
| option :decode_options, {} |
| }.delete_if { |_, v| v.nil? }, | ||
| ), | ||
| )[0] | ||
| rescue Exception => e |
There was a problem hiding this comment.
Rescuing the Exception class is too broad and will catch system-level errors that should not be caught. Use StandardError instead to avoid catching signals like SystemExit or SignalException.
| rescue Exception => e | |
| rescue StandardError => e |
| jwks: options.jwks_loader | ||
| }.compact | ||
| ) | ||
| jwks: options.jwks_loader, |
There was a problem hiding this comment.
The use of .delete_if to remove nil values is less efficient than .compact for Ruby 2.4+. However, given this PR supports Ruby 2.2-2.3, this approach is acceptable as Hash#compact was added in Ruby 2.4. Consider adding a comment explaining this is for Ruby 2.2/2.3 compatibility.
| jwks: options.jwks_loader, | |
| jwks: options.jwks_loader, | |
| # NOTE: Using delete_if instead of Hash#compact for Ruby 2.2/2.3 compatibility. |
| OmniAuth.config.logger = Logger.new('/dev/null') | ||
| OmniAuth.config.logger = Logger.new("/dev/null") | ||
| require "omniauth/version" | ||
| puts "OMNIAUTH VERSION: #{OmniAuth::VERSION}" |
There was a problem hiding this comment.
The 'puts' statement for outputting the OmniAuth version should only run during test execution, not during normal library usage. However, since this is in spec_helper.rb, it only runs during tests, so this is acceptable. Consider using a logger or RSpec's output mechanism instead of puts for cleaner test output.
| begin | ||
| method.call(*original_args, **original_kwargs).tap(&blk) | ||
| rescue ArgumentError | ||
| # Kludge for old ruby < 2.7 | ||
| method.call(*original_args).tap(&blk) | ||
| end |
There was a problem hiding this comment.
This rescue clause catches ArgumentError to handle Ruby < 2.7 incompatibility with keyword arguments. However, this will silently swallow legitimate ArgumentError exceptions from the method call itself. Consider using a more specific check (e.g., checking the error message for keyword argument issues) or documenting this limitation clearly.
| gem "rspec", "~> 3.12" # ruby * | ||
| gem "rack-test", "~> 2.1" # ruby 2.0 | ||
| gem "rack" # ruby 2.4 | ||
| gem "rack-session", "< 2", github: "pboling/rack-session", branch: "fix-missing-rack-session" # ruby < 2.4 |
There was a problem hiding this comment.
The line has excessive trailing whitespace after the comment. While not critical, this violates common style guidelines and can cause unnecessary noise in diffs.
| gem "rack-session", "< 2", github: "pboling/rack-session", branch: "fix-missing-rack-session" # ruby < 2.4 | |
| gem "rack-session", "< 2", github: "pboling/rack-session", branch: "fix-missing-rack-session" # ruby < 2.4 |
| Use this section to tell people about which versions of your project are | ||
| currently being supported with security updates. | ||
|
|
There was a problem hiding this comment.
The placeholder text "Use this section to tell people about which versions of your project are currently being supported with security updates." should be replaced with actual content or removed, as it's template text that doesn't provide value.
| Use this section to tell people about which versions of your project are | |
| currently being supported with security updates. | |
| The following table lists the versions of this project that currently receive security updates. |
| begin | ||
| require "rack/session" | ||
| rescue LoadError | ||
| nil # File won't exist in old rack for Ruby 2.2 & 2.3 |
There was a problem hiding this comment.
While this approach of rescuing LoadError and assigning nil works, the rescued exception is essentially discarded. Since this is intentional for backward compatibility, consider removing the explicit 'nil' assignment as it serves no purpose - the rescue block could simply be empty.
| spec.license = "MIT" | ||
| spec.required_ruby_version = ">= 2.2" | ||
|
|
||
| spec.files = %x(git ls-files).split($/) |
There was a problem hiding this comment.
Using shell command execution with backticks in a gemspec is not recommended for security and portability reasons. This can fail in environments without git or where git is not in PATH. Use Dir.glob or similar pure Ruby alternatives instead.
| }.each do |private_key_class, algorithms| | ||
| String => %w[HS256 HS384 HS512], | ||
| } | ||
| algos.merge!(OpenSSL::PKey::EC => %w[ES256 ES384 ES512]) unless ["2.2.10", "2.3.8"].include?(RubyVersion.to_s) |
There was a problem hiding this comment.
The constant 'RubyVersion' is referenced but not defined anywhere in the codebase. This will cause a NameError at runtime. Use RUBY_VERSION constant instead, which is a built-in Ruby constant.
…v2 (#6)
👷 Add ancient build for Ruby 2.2 & 2.3
💚 Better handling of ancient ruby scenario
💚 Ancient ruby support
✨ Support Ruby 2.2 & 2.3