Code-review followups: restore PR #10 commit + harden inactive_message#11
Merged
Conversation
Follow-ups to PRs #9 and #10 surfaced by the code review. - Apply the same OmniAuth.config.path_prefix substitution to both install-generator templates. New installs were still shipping the hardcoded path PR #10 fixed in the live view. - Default the InactiveError key to :inactive when the model's inactive_message returns nil/blank. The empty key used to render an empty flash; now the user always sees a reason. - Stop leaking custom inactive_message symbols. If the host returns a symbol like :locked_by_admin with no devise.failure translation, the raw symbol used to land in a public flash. Fall back to devise.failure.inactive instead. The disabled-user request spec is consolidated via let helpers (auth_hash, disabling_hook, post_callback) and now uses new_admin_user_session_path / OmniAuth.config.path_prefix instead of hardcoded URLs. Adds a regression spec for the retry leg + an inactive winner row, documenting the expected redirect-and-no- warden-session behaviour.
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
1 participant
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.
Follow-ups surfaced by a code-review pass (regression, minimalism, bug, security agents).
Depends on #12 — that PR restores the orphaned OmniAuth.config.path_prefix view fix; some of the wins here (generator templates, spec hardcoded paths) sit on top of it.
What's in here
Generator templates still hardcode
/admin/auth/oidc. PR #10 fixed the live view but the install generator was shipping the broken hardcoded path to every new project. SameOmniAuth.config.path_prefixsubstitution applied to bothsessions_new.html.erbandsessions_new_v4.html.erbtemplates.InactiveErrorcollapsed wheninactive_messagewas blank. A host's override that legitimately returns nil on some branch produced an empty key, which then rendered an empty flash. Default to:inactiveinside the error's initializer so the user always sees a reason.Raw
inactive_messagesymbols leaked to the public flash. If the host returned:locked_by_adminand the translation was missing, the controller'sdefault: e.inactive_message_key.to_sfallback rendered the raw symbol name. Fall back toI18n.t("devise.failure.inactive")instead.Disabled-user spec consolidated. Repeated
ActiveAdmin::Oidc.configureandbuild_auth_hashblocks collapsed intolethelpers (disabling_hook,auth_hash) and apost_callbackhelper. Hardcoded paths replaced withnew_admin_user_session_pathandOmniAuth.config.path_prefix. Inline references to "Code-review followup" / "HIGH #2" stripped — those belong in PR descriptions, not source.Regression spec for the retry leg + inactive winner. The review flagged it as a potential gap; turns out Devise's
after_set_usercallback catches it transparently. Spec documents the expected behaviour (redirect + no Warden session) so future refactors can't quietly regress it.Findings declined
email_verified == falsestrict mode (require_verified_email_for_adoptionflag) — no evidence of real IdPs shipping"false"strings; would add config surface for a defense-in-depth nice-to-have.@retriedivar restructure — discussed with the minimalism agent; concluded the ivar with single raise/set/read is more local than the proposedrescue RetryProvisioning → find_existing!form.Test plan
bundle exec rake spec:all— 106 + 7 + 5 = 118/0 (the +1 example lives in Restore orphaned PR #10 merge: source SSO login path from OmniAuth.config.path_prefix #12)<%%= %>escape