Sync fork with upstream wmlele/devise-otp + Castle deltas (2.0.0.castle.0)#8
Draft
bartes wants to merge 160 commits into
Draft
Sync fork with upstream wmlele/devise-otp + Castle deltas (2.0.0.castle.0)#8bartes wants to merge 160 commits into
bartes wants to merge 160 commits into
Conversation
- Unify the term "trusted browser" vs "trusted device" - Allow browser to be set as trusted when providing token - Update yml to reflect browser change - Remove the hard coded timeframe from `trusted_browser.explain`
Fix typo in en.yml
Support Rails 5 and Devise 4.2.x
…sted-browser-changes Trusted browser updates
Rails 7 support
Fix links and render for Turbo
Simplify the flow by not requiring the OTP code on refresh
Remove placeholder
- Update OTP Credentials controller to execute the valid_for_authenitcation? method - - Checks if account is locked - - Increments failed attempts and/or locks account as needed - Remove resource.valid_otp_challenge? check from OTP Credentials controller - - Already covered by find_valid_otp_challenge - Add tests to confirm updated functionality
- Update sign_in form to pass "Remember me" selection through to OTP Credentials form via otp_challenge_path redirect; - Remove second, persistence related "Remember me" checkbox from OTP Credentials form; - Add tests to confirm functionality;
…issue in Gemfile;
…ele#140) - Move browser persistence actions to dedicated controller - Standardize browser persistence routes (post/delete rather than get/post) - Use button_to (rather than link_to) for controls to reduce JS dependency - Add tests for removing and clearing browser persistence
- Update OTP Credentials controller to apply remember_me value even when skipping challenge due to browser persistence; - Add test to confirm resolution;
Add support for Devise 5
Align this gem's Ruby version with the consumer (Castle's `web` app runs Ruby 3.3.10) so local development and tooling (asdf) pick the same interpreter when contributors switch between repos. Upstream's `required_ruby_version` is `>= 3.2.0`, so 3.3.10 is well within range; CI continues to test against 3.2 / 3.3 / 3.4 / 4.0.
- Bump VERSION to `2.0.0.castle.0` so the gem is visibly distinct from upstream `2.0.0` when resolved from Bundler. Subsequent Castle-only patches should bump the `castle.N` suffix. - Add a fork notice block at the top of the README pointing back to `wmlele/devise-otp` and listing the current Castle-only deltas, so it's clear at a glance what this fork carries and when it should be re-synced.
\`standardrb 1.35.0.1\` is an inoperative placeholder gem that prints a "this gem does nothing — pin a real version" warning on every \`bundle install\`. Upstream's Gemfile lists it alongside \`rubocop-rails-omakase\` and \`erb_lint\`, but nothing in the source tree, bin/, or .github/ actually invokes \`standardrb\` — confirmed via \`rg "standardrb"\`. The remaining formatters (\`rubocop-rails-omakase\` + \`erb_lint\`) are sufficient for CI and local linting. \`bundle install\` now installs 112 gems instead of 116 and emits no warnings. Removed from \`Gemfile\` and from all four \`gemfiles/rails_*.gemfile\` files since those are committed (not regenerated on CI). When the upstream Gemfile drops standardrb (or pins it to a working version), this commit can be reverted as part of a re-sync.
Document the initial Castle fork release at the top of the existing upstream CHANGELOG. Lists what this fork carries on top of upstream master and the re-sync policy.
The CI matrix lists Ruby 3.2 / 3.3 / 3.4 / 4.0 / head, but upstream left two stale \`exclude\` entries referencing Ruby 3.1 (\`3.1 × rails_8.0\` and \`3.1 × rails_8.1\`). Those have been no-ops since the matrix was renamed; the gemspec already enforces \`required_ruby_version >= 3.2.0\` independently. Removing the dead config so the workflow file actually reflects what we test against.
Upstream's \`.gitignore\` doesn't cover the local bundler-install path (\`vendor/bundle/\`) or the sqlite3 dummy database files. Running \`bundle install\` locally then \`git status\` floods with thousands of untracked vendored gem files; \`git add -A\` accidentally sweeps them in (~557MB). Adding the two patterns so the working tree stays clean. Both are genuinely build artifacts, not source.
\`appraisal\` was used as a regenerator for the per-Rails-version \`gemfiles/rails_*.gemfile\` files, but CI never invokes it — the workflow sets \`BUNDLE_GEMFILE\` directly. Outside of \`bundle exec appraisal generate\` in dev, the gem was unused, and the upstream Gemfile pinned it to a *git ref* (\`git: "https://github.com/thoughtbot/appraisal.git"\`) which is fragile: any new commit on thoughtbot/appraisal master can break our \`bundle install\`. This commit: - Removes \`gem "appraisal"\` from \`Gemfile\` and from all four \`gemfiles/rails_*.gemfile\` files. - Deletes the \`Appraisals\` DSL file. The per-Rails dep pins it contained (e.g. the Ruby 3.4+ \`logger\` workaround for Rails 7.1) are already rendered into the gemfiles. - Deletes \`bin/appraisal\` (binstub). - Updates README + CHANGELOG to reflect the change. The CI matrix is unchanged — it still tests Ruby 3.2/3.3/3.4/4.0/head \u00d7 Rails 7.1/7.2/8.0/8.1, just without appraisal as middleware. Mirrors the same approach \`castle_devise\` took in its 0.6.0 release. Going forward, when bumping a Rails version, edit the corresponding \`gemfiles/rails_X.Y.gemfile\` by hand (or copy from another version).
\`web\`'s \`.tool-versions\` lists both \`ruby 3.3.10\` and \`nodejs 22.22.3\`. Adding the nodejs line here keeps the two repos in lockstep so contributors switching between them don't have asdf reconfiguring node versions. The gem itself has no JS / asset build pipeline, so nodejs isn't strictly required to develop on devise-otp \u2014 this is purely a tooling ergonomics alignment.
The previous pattern \`vendor/bundle/\` was anchored to the repo root, so \`gemfiles/vendor/bundle/\` (created when running \`BUNDLE_GEMFILE=gemfiles/rails_X.Y.gemfile bundle install\`) wasn't matched. Using \`**/vendor/bundle/\` covers both paths.
Gemspec allows \`devise >= 4.8.0, < 6.0\`, but none of the per-Rails gemfiles pin a Devise version \u2014 bundler always picks the latest matching, so today every row in the CI matrix tests only Devise 5.0.x. Devise 4 compatibility is asserted by the gemspec but never actually verified. Pin per Rails era to exercise both: - gemfiles/rails_7.1.gemfile \u2192 devise ~> 4.9 (Devise 4 era) - gemfiles/rails_7.2.gemfile \u2192 devise ~> 4.9 - gemfiles/rails_8.0.gemfile \u2192 devise ~> 5.0 (Devise 5 era) - gemfiles/rails_8.1.gemfile \u2192 devise ~> 5.0 Result: CI now verifies 8 distinct (Rails, Devise) combinations \u00d7 5 Ruby versions. Cross-era combos (Devise 4 on Rails 8, Devise 5 on Rails 7) are intentionally not tested \u2014 they're not in any consumer's use case and would double the matrix. Also pin \`devise ~> 5.0\` in the top-level \`Gemfile\` so local \`bundle install\` and the dev test run default to the new major. Switching to Devise 4 locally is a single \`BUNDLE_GEMFILE=gemfiles/ rails_7.1.gemfile bundle exec rake test\` invocation. Both paths verified locally: - Devise 4.9.4 \u00d7 Rails 7.1.6: 68 tests, 188 assertions, 0 failures - Devise 5.0.4 \u00d7 Rails 8.1.3: 68 tests, 188 assertions, 0 failures
Refreshable hook - Upstream's hook guards on `defined?(record.class.otp_credentials_refresh)`, which is always truthy for any model that includes `:otp_authenticatable` (Devise::Models.config defines the accessor on the class). When `otp_credentials_refresh` was set to `false`, the hook still ran and attempted `Time.now + false`, raising `TypeError` on every sign-in. - Switch to a truthy-value check on the resolved setting so a disabled refresh short-circuits the assignment. otp_challenge_via_strategy config - The 2.0.0 redesign moved the OTP challenge redirect into `Devise::Strategies::DatabaseAuthenticatable`. That runs during `warden.authenticate!` and prevents applications from doing additional work between authentication and the OTP redirect (e.g. a risk check that decides which OTP path to take, or applying a custom redirect host). - Add `Devise.otp_challenge_via_strategy` (default `true`, preserving upstream behaviour). When `false`, the strategy succeeds without redirecting and the controller is responsible for driving the OTP challenge — matching the pre-2.0.0 flow. Bumps VERSION to 2.0.0.castle.1.
Upstream 2.0.0 stopped shipping any JavaScript — the QR code is now
rendered server-side as an inline SVG via `rqrcode` inside
`otp_authenticator_token_image`. The engine still registered
`devise-otp.js` for asset precompile though, pointing at a file that no
longer exists. Any app running `assets:precompile` on Sprockets >= 4
would inherit the registration and crash with
`Sprockets::FileNotFound: couldn't find file 'devise-otp.js'`.
This commit:
* Removes the `app.config.assets.precompile << "devise-otp.js"` block.
* Removes the now-unused `config.devise_otp.precompile_assets` opt-out
(its only consumer was the block above).
`app/assets/stylesheets/devise-otp.css` is left in place — it's still
referenced by the `qrcode-container` wrapper the helper emits — but
nothing was auto-registering it for precompile before either, so apps
that want it bundled need to opt in via their own `assets.precompile`
list (unchanged behaviour).
Bumps VERSION to 2.0.0.castle.2 and adds a CHANGELOG entry.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
13 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What and Why
This PR resyncs Castle's heavily-diverged fork of
devise-otp(last meaningful change was 5 years ago, while upstream shipped two breaking-change releases — v0.7.0 confirmation-required enable flow, v2.0.0 browser-persistence split + module rename) with upstreamwmlele/devise-otpmaster (HEAD atc2b96ed, 9 commits pastv2.0.0).By piggy-backing on the freshest upstream we get Rails 8.1, Devise 5, Ruby 4 CI, Hotwire/Turbo compat, Lockable integration, server-side SVG QR codes, the Refreshable Warden hook, Rubocop + ERB Lint, and Appraisal-based multi-Rails testing — all without us having to maintain any of it.
On top of upstream we keep only four Castle-only commits, each with a clear, narrow scope.
Branch is named
2.0.0.castlesowebcan pin it directly:(replacing the now-stale
branch: 'rails8').Castle-only commits (on top of upstream master)
a91c1a7chore: pin Ruby 3.3.10 via .tool-versions— align withweb287693achore: mark this as 2.0.0.castle.0 + add fork notice— version + README header0c14e99chore: drop standardrb placeholder gem (inoperative)— removes 4 dead deps72c826cdocs: add CHANGELOG entry for 2.0.0.castle.0Known limitations / heads-up for the web migration
Adopting this branch in
webis a breaking change at the call-site level (separate PR will follow):Devise::Otp::*→DeviseOtp::*— affectsusers/tokens_controller.rb,users/credentials_controller.rb.reset_otp_credentials!is removed upstream — replace withclear_otp_fields!. Used inapp/admin/user.rb,users/tokens_controller.rb,services/users/invite_to_organization.rb.is_otp_trusted_device_for?renamed tois_otp_trusted_browser_for?(used inviews/devise/otp/tokens/_trusted_devices.html.haml).web'sUsers::SessionsController#otp_createalready mirrors the upstreamDeviseOtpAuthenticatable::Hooks::Sessions#createflow, so no semantic differences there.Test plan
bundle installresolves cleanly with Ruby 3.3.10 + Devise 5.0.4 + Rails 8.1.3 (no "BIG SCARY WARNING" anymore)bundle exec rake test→ 68 tests, 188 assertions, 0 failures, 0 errors, 0 skips.github/workflows/ci.yml: Ruby 3.2/3.3/3.4/4.0/head × Rails 7.1/7.2/8.0/8.1)webagainst a real OTP login flow before merging this PR (kept as draft until then)