Skip to content

Commit bccf4e0

Browse files
ZogStriPZogStriP
authored
FIX: improve "read only" modes (#33521)
The reasons for these changes is https://meta.discourse.org/t/-/89605 broke and admins were not able to log back in if they had previously enabled the "read only" mode. Thus ensued a deep dive into how all the "read only" modes worked, which was made difficult due to the lack of tests. The "cornerstone" of this PR is the `read_only_mixin.rb` file which was improved to be able to differentiate between the "readonly" mode and the "staff writes only" mode. I then made use of the `allow_in_readonly_mode` and `allow_in_staff_writes_only_mode` method to **explicitely** list all the actions that should work in those modes. I also added the "readonly" mixin to the `WebhooksController` since it doesn't inherit from the `ApplicationController`. I improved the security of the `/u/admin-login` endpoint by always sending the same message no matter if we found or not an admin account with the provided email address. I added two system specs: 1. for ensuring that admins can log in via /u/admin-lgoin and then clicking the link in the email they received while the site is in readonly mode. 2. for ensuring the "staff writes only mode" is _actually_ tested by ensuring a moderator can log in and create a topic while the site is in that mode. Plenty of specs were updated to ensure 100% converage of the various "read only" modes.
1 parent 0dcbbe0 commit bccf4e0

22 files changed

+519
-124
lines changed

‎app/controllers/admin/backups_controller.rb

Lines changed: 14 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,20 @@ class Admin::BackupsController < Admin::AdminController
88

99
before_action :ensure_backups_enabled
1010
skip_before_action :check_xhr, only: %i[index show logs check_backup_chunk upload_backup_chunk]
11-
skip_before_action :ensure_backups_enabled, only: %w[show status index email]
11+
skip_before_action :ensure_backups_enabled, only: %i[show status index email]
12+
13+
allow_in_readonly_mode :create,
14+
:cancel,
15+
:email,
16+
:destroy,
17+
:restore,
18+
:rollback,
19+
:readonly,
20+
:upload_backup_chunk,
21+
:create_multipart,
22+
:complete_multipart,
23+
:abort_multipart,
24+
:batch_presign_multipart_parts
1225

1326
def index
1427
respond_to do |format|

‎app/controllers/application_controller.rb

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -145,7 +145,7 @@ class PluginDisabled < StandardError
145145
rescue_from PG::ReadOnlySqlTransaction do |e|
146146
Discourse.received_postgres_readonly!
147147
Rails.logger.error("#{e.class} #{e.message}: #{e.backtrace.join("\n")}")
148-
rescue_with_handler(Discourse::ReadOnly.new) || raise
148+
rescue_with_handler(Discourse::ReadOnly) || raise
149149
end
150150

151151
rescue_from ActionController::ParameterMissing do |e|
@@ -580,9 +580,7 @@ def handle_permalink(path)
580580

581581
def rate_limit_second_factor!(user)
582582
return if params[:second_factor_token].blank?
583-
584583
RateLimiter.new(nil, "second-factor-min-#{request.remote_ip}", 6, 1.minute).performed!
585-
586584
RateLimiter.new(nil, "second-factor-min-#{user.username}", 6, 1.minute).performed! if user
587585
end
588586

‎app/controllers/categories_controller.rb

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,9 @@ class CategoriesController < ApplicationController
2929
]
3030
skip_before_action :verify_authenticity_token, only: %i[search]
3131

32+
# The front-end is POSTing data to this endpoint, but we're not modifying anything
33+
allow_in_readonly_mode :search
34+
3235
SYMMETRICAL_CATEGORIES_TO_TOPICS_FACTOR = 1.5
3336
MIN_CATEGORIES_TOPICS = 5
3437
MAX_CATEGORIES_LIMIT = 25

‎app/controllers/presence_controller.rb

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -42,8 +42,6 @@ def get
4242
end
4343

4444
def update
45-
raise Discourse::ReadOnly if @readonly_mode
46-
4745
client_id = params[:client_id]
4846
if !client_id.is_a?(String) || client_id.blank?
4947
raise Discourse::InvalidParameters.new(:client_id)

‎app/controllers/session_controller.rb

Lines changed: 13 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,8 @@ class SessionController < ApplicationController
1212

1313
skip_before_action :check_xhr, only: %i[second_factor_auth_show]
1414

15-
allow_in_staff_writes_only_mode :create, :email_login, :forgot_password
15+
allow_in_readonly_mode :email_login
16+
allow_in_staff_writes_only_mode :create, :forgot_password
1617

1718
ACTIVATE_USER_KEY = "activate_user"
1819
FORGOT_PASSWORD_EMAIL_LIMIT_PER_DAY = 6
@@ -107,7 +108,6 @@ def sso_provider(payload = nil, confirmed_2fa_during_login = false)
107108

108109
def become
109110
raise Discourse::InvalidAccess if Rails.env.production?
110-
raise Discourse::ReadOnly if @readonly_mode
111111

112112
if ENV["DISCOURSE_DEV_ALLOW_ANON_TO_IMPERSONATE"] != "1"
113113
return render plain: <<~TEXT, status: 403
@@ -166,7 +166,7 @@ def test_second_factor_restricted_route
166166

167167
def sso_login
168168
raise Discourse::NotFound unless SiteSetting.enable_discourse_connect
169-
raise Discourse::ReadOnly if @readonly_mode && !staff_writes_only_mode?
169+
raise Discourse::ReadOnly if @readonly_mode && !@staff_writes_only_mode
170170

171171
params.require(:sso)
172172
params.require(:sig)
@@ -207,7 +207,7 @@ def sso_login
207207
invite = validate_invitiation!(sso)
208208

209209
if user = sso.lookup_or_create_user(request.remote_ip)
210-
raise Discourse::ReadOnly if staff_writes_only_mode? && !user&.staff?
210+
raise Discourse::ReadOnly if @staff_writes_only_mode && !user&.staff?
211211

212212
if user.suspended?
213213
render_sso_error(text: failed_to_login(user)[:error], status: 403)
@@ -332,7 +332,7 @@ def create
332332

333333
user = User.find_by_username_or_email(normalized_login_param)
334334

335-
raise Discourse::ReadOnly if staff_writes_only_mode? && !user&.staff?
335+
raise Discourse::ReadOnly if @staff_writes_only_mode && !user&.staff?
336336

337337
rate_limit_second_factor!(user)
338338

@@ -446,24 +446,27 @@ def email_login_info
446446

447447
def email_login
448448
token = params[:token]
449-
matched_token = EmailToken.confirmable(token, scope: EmailToken.scopes[:email_login])
450-
user = matched_token&.user
449+
user = EmailToken.confirmable(token, scope: EmailToken.scopes[:email_login])&.user
451450

452451
check_local_login_allowed(user: user, check_login_via_email: true)
453-
454452
rate_limit_second_factor!(user)
455453

456454
if user.present? && !authenticate_second_factor(user).ok
457455
return render(json: @second_factor_failure_payload)
458456
end
459457

460458
if user = EmailToken.confirm(token, scope: EmailToken.scopes[:email_login])
459+
if @staff_writes_only_mode
460+
raise Discourse::ReadOnly unless user.staff?
461+
elsif @readonly_mode
462+
raise Discourse::ReadOnly unless user.admin?
463+
end
464+
461465
if login_not_approved_for?(user)
462466
return render json: login_not_approved
463467
elsif payload = login_error_check(user)
464468
return render json: payload
465469
else
466-
raise Discourse::ReadOnly if staff_writes_only_mode? && !user&.staff?
467470
user.update_timezone_if_missing(params[:timezone])
468471
log_on_user(user)
469472
return render json: success_json
@@ -636,7 +639,7 @@ def forgot_password
636639
end
637640

638641
if user
639-
raise Discourse::ReadOnly if staff_writes_only_mode? && !user.staff?
642+
raise Discourse::ReadOnly if @staff_writes_only_mode && !user.staff?
640643
enqueue_password_reset_for_user(user)
641644
else
642645
RateLimiter.new(

‎app/controllers/users/omniauth_callbacks_controller.rb

Lines changed: 5 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,3 @@
1-
# -*- encoding : utf-8 -*-
21
# frozen_string_literal: true
32

43
class Users::OmniauthCallbacksController < ApplicationController
@@ -13,6 +12,7 @@ class Users::OmniauthCallbacksController < ApplicationController
1312
# will not have a CSRF token, however the payload is all validated so its safe
1413
skip_before_action :verify_authenticity_token, only: :complete
1514

15+
# These are usually GET requests but some providers use POST requests
1616
allow_in_staff_writes_only_mode :complete
1717

1818
def confirm_request
@@ -21,17 +21,15 @@ def confirm_request
2121
end
2222

2323
def complete
24-
auth = request.env["omniauth.auth"]
25-
raise Discourse::NotFound unless request.env["omniauth.auth"]
26-
raise Discourse::ReadOnly if @readonly_mode && !staff_writes_only_mode?
24+
raise Discourse::ReadOnly if @readonly_mode && !@staff_writes_only_mode
25+
raise Discourse::NotFound unless auth = request.env["omniauth.auth"]
2726

2827
auth[:session] = session
2928

3029
authenticator = self.class.find_authenticator(params[:provider])
3130

3231
if session.delete(:auth_reconnect) && authenticator.can_connect_existing_user? && current_user
33-
path = persist_auth_token(auth)
34-
return redirect_to path
32+
return redirect_to persist_auth_token(auth)
3533
else
3634
DiscourseEvent.trigger(:before_auth, authenticator, auth, session, cookies, request)
3735
@auth_result = authenticator.after_authenticate(auth)
@@ -71,7 +69,7 @@ def complete
7169

7270
return render_auth_result_failure if @auth_result.failed?
7371

74-
raise Discourse::ReadOnly if staff_writes_only_mode? && !@auth_result.user&.staff?
72+
raise Discourse::ReadOnly if @staff_writes_only_mode && !@auth_result.user&.staff?
7573

7674
complete_response_data
7775

‎app/controllers/users_controller.rb

Lines changed: 18 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -107,7 +107,8 @@ class UsersController < ApplicationController
107107

108108
before_action :add_noindex_header, only: %i[show my_redirect]
109109

110-
allow_in_staff_writes_only_mode :admin_login, :email_login, :password_reset_update
110+
allow_in_readonly_mode :admin_login
111+
allow_in_staff_writes_only_mode :email_login, :password_reset_update
111112

112113
MAX_RECENT_SEARCHES = 5
113114

@@ -862,7 +863,6 @@ def remove_password
862863
RateLimiter.new(nil, "remove-password-hr-#{user.username}", 6, 1.hour).performed!
863864

864865
raise Discourse::NotFound if !user || !user.user_password
865-
raise Discourse::ReadOnly if staff_writes_only_mode? && !user.staff?
866866
raise Discourse::InvalidAccess if !session_is_trusted?
867867

868868
user.remove_password
@@ -872,16 +872,15 @@ def remove_password
872872

873873
def password_reset_update
874874
expires_now
875+
875876
token = params[:token]
876-
password_reset_find_user(token, committing_change: true)
877877

878+
password_reset_find_user(token, committing_change: true)
878879
rate_limit_second_factor!(@user)
879880

880-
# no point doing anything else if we can't even find
881-
# a user from the token
882-
if @user
883-
raise Discourse::ReadOnly if staff_writes_only_mode? && !@user.staff?
881+
raise Discourse::ReadOnly if @staff_writes_only_mode && !@user&.staff?
884882

883+
if @user
885884
if !secure_session["second-factor-#{token}"]
886885
second_factor_authentication_result =
887886
@user.authenticate_second_factor(params, secure_session)
@@ -1008,21 +1007,17 @@ def admin_login
10081007
RateLimiter.new(nil, "admin-login-hr-#{request.remote_ip}", 6, 1.hour).performed!
10091008
RateLimiter.new(nil, "admin-login-min-#{request.remote_ip}", 3, 1.minute).performed!
10101009

1011-
if user = User.with_email(params[:email]).admins.human_users.first
1010+
if user = User.real.admins.with_email(params[:email]).first
10121011
email_token =
1013-
user.email_tokens.create!(email: user.email, scope: EmailToken.scopes[:email_login])
1014-
token_string = email_token.token
1015-
token_string += "?safe_mode=no_plugins,no_themes" if params["use_safe_mode"]
1016-
Jobs.enqueue(
1017-
:critical_user_email,
1018-
type: "admin_login",
1019-
user_id: user.id,
1020-
email_token: token_string,
1021-
)
1022-
@message = I18n.t("admin_login.success")
1023-
else
1024-
@message = I18n.t("admin_login.errors.unknown_email_address")
1012+
user.email_tokens.create!(email: user.email, scope: EmailToken.scopes[:email_login]).token
1013+
email_token += "?safe_mode=no_plugins,no_themes" if params["use_safe_mode"]
1014+
Jobs.enqueue(:critical_user_email, type: "admin_login", user_id: user.id, email_token:)
10251015
end
1016+
1017+
# NOTE: we don't check for `readonly` mode here because it might leak information about whether
1018+
# an email address is a registered admin account.
1019+
1020+
@message = I18n.t("admin_login.acknowledgement", email: params[:email])
10261021
end
10271022

10281023
render layout: "no_ember"
@@ -1040,13 +1035,15 @@ def email_login
10401035

10411036
RateLimiter.new(nil, "email-login-hour-#{request.remote_ip}", 6, 1.hour).performed!
10421037
RateLimiter.new(nil, "email-login-min-#{request.remote_ip}", 3, 1.minute).performed!
1043-
user = User.human_users.find_by_username_or_email(params[:login])
1038+
user = User.real.find_by_username_or_email(params[:login])
10441039
user_presence = user.present? && !user.staged
10451040

10461041
if user
10471042
RateLimiter.new(nil, "email-login-hour-#{user.id}", 6, 1.hour).performed!
10481043
RateLimiter.new(nil, "email-login-min-#{user.id}", 3, 1.minute).performed!
10491044

1045+
raise Discourse::ReadOnly if @staff_writes_only_mode && !user.staff?
1046+
10501047
if user_presence
10511048
DiscourseEvent.trigger(:before_email_login, user)
10521049

‎app/controllers/webhooks_controller.rb

Lines changed: 10 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,17 @@
11
# frozen_string_literal: true
22

3-
require "openssl"
4-
53
class WebhooksController < ActionController::Base
4+
include ReadOnlyMixin
5+
66
skip_before_action :verify_authenticity_token
77

8+
before_action :check_readonly_mode
9+
before_action :block_if_readonly_mode
10+
11+
rescue_from Discourse::ReadOnly do
12+
head :service_unavailable
13+
end
14+
815
def mailgun
916
return signature_failure if SiteSetting.mailgun_api_key.blank?
1017

@@ -279,7 +286,7 @@ def valid_sendgrid_signature?
279286

280287
begin
281288
public_key = OpenSSL::PKey::EC.new(Base64.decode64(SiteSetting.sendgrid_verification_key))
282-
rescue StandardError => err
289+
rescue StandardError
283290
Rails.logger.error("Invalid Sendgrid verification key")
284291
return false
285292
end

‎config/locales/server.en.yml

Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -5307,10 +5307,7 @@ en:
53075307
badge_title_metadata: "%{display_name} badge on %{site_title}"
53085308
53095309
admin_login:
5310-
success: "Email Sent"
5311-
errors:
5312-
unknown_email_address: "Unknown email address."
5313-
invalid_token: "Invalid token."
5310+
acknowledgement: "If an admin account with the email address %{email} exists, you will receive an email with a link to log in."
53145311
email_input: "Admin Email"
53155312
submit_button: "Send Email"
53165313
safe_mode: "Safe Mode: disable all themes/plugins when logging in"

‎lib/application_layout_preloader.rb

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -109,13 +109,14 @@ def preload_current_user_data
109109
end
110110

111111
def preload_anonymous_data
112+
check_readonly_mode if @readonly_mode.nil?
112113
@preloaded["site"] = Site.json_for(@guardian)
113114
@preloaded["siteSettings"] = SiteSetting.client_settings_json
114115
@preloaded["customHTML"] = custom_html_json
115116
@preloaded["banner"] = banner_json
116117
@preloaded["customEmoji"] = custom_emoji
117-
@preloaded["isReadOnly"] = get_or_check_readonly_mode.to_json
118-
@preloaded["isStaffWritesOnly"] = get_or_check_staff_writes_only_mode.to_json
118+
@preloaded["isReadOnly"] = @readonly_mode.to_json
119+
@preloaded["isStaffWritesOnly"] = @staff_writes_only_mode.to_json
119120
@preloaded["activatedThemes"] = activated_themes_json
120121
end
121122

0 commit comments

Comments
 (0)