-
Notifications
You must be signed in to change notification settings - Fork 45
Sudo Mode Implementation Exploration #10671
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
base: main
Are you sure you want to change the base?
Conversation
7f4b0f9
to
4784129
Compare
sms: false, | ||
).run | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sms: false, | |
).run | |
sms: false, | |
).run | |
end | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
end | |
end | |
end | |
end |
end | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
end | |
end | |
end | |
end |
<%= form_tag(request.path, method: request.request_method_symbol) do %> | ||
<% | ||
Rack::Utils | ||
.parse_query(request.request_parameters.to_query) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<%= form_tag(request.path, method: request.request_method_symbol) do %> | |
<% | |
Rack::Utils | |
.parse_query(request.request_parameters.to_query) | |
<%= form_tag(request.path, method: request.request_method_symbol) do %> | |
Rack::Utils | |
.parse_query(request.request_parameters.to_query) |
@@ -20,6 +20,7 @@ class UsersController < ApplicationController | |||
:complete_sms_auth_verification, | |||
:start_sms_auth_verification] | |||
before_action :set_shown_private_feature_previews, only: [:edit, :edit_featurepreviews, :edit_security, :edit_admin] | |||
before_action :enforce_sudo_mode, only: [:update] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is super clean - thank you for implementing it this way
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The changes to this model LGTM - only comment is I would have implemented preceding_login
instead of inital
Rack::Utils | ||
.parse_query(request.request_parameters.to_query) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we could make this a partial that we include in all the forms rendered from the LoginsController
and then have some shared logic in LoginsController#complete
to handle input from that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Model changes LGTM - as you mentioned in the video, I think the big thing will be reusing as much of the login controller code as possible. I'm happy to jump on a call to talk through that stuff at some point.
This is proof-of-concept code. See #10653 for details.