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

Feature: MFA for admins #635

Merged
merged 3 commits into from
Dec 16, 2024
Merged

Feature: MFA for admins #635

merged 3 commits into from
Dec 16, 2024

Conversation

sfnelson
Copy link
Contributor

No description provided.

Copy link
Contributor

@mattr mattr left a comment

Choose a reason for hiding this comment

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

LGTM. Nice to see this flow enabled.

Some minor thoughts for additional tests, possible security, and clarity.

Comment on lines +75 to +70
drift_ahead: 15,
drift_behind: 15,
Copy link
Contributor

Choose a reason for hiding this comment

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

(opt) Use constants for magic numbers

redirect_to(url_from(params[:redirect].presence) || admin_dashboard_path, status: :see_other)
else
admin_user = Admin::User.new
admin_user.errors.add(:email, "Invalid token")
Copy link
Contributor

Choose a reason for hiding this comment

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

(opt) If we use a locale entry, it should be possible to overwrite this on a per-app basis

redirect_to(url_from(params[:redirect].presence) || admin_dashboard_path, status: :see_other)
else
admin_user = Admin::User.new
admin_user.errors.add(:email, "Invalid login credentials")
Copy link
Contributor

Choose a reason for hiding this comment

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

as above

app/controllers/admin/sessions_controller.rb Outdated Show resolved Hide resolved
app/models/concerns/koi/model/otp.rb Show resolved Hide resolved
app/models/admin/user.rb Outdated Show resolved Hide resolved
app/views/admin/otps/_form.html.erb Outdated Show resolved Hide resolved
spec/requests/admin/otps_controller_spec.rb Show resolved Hide resolved
@sfnelson sfnelson merged commit c82b38b into main Dec 16, 2024
1 check failed
@sfnelson sfnelson deleted the feature/mfa branch December 16, 2024 02:18
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