From 00b41437b01e740c59203d73b3d74654a8d62d0a Mon Sep 17 00:00:00 2001 From: Arpit Jalan Date: Wed, 11 Nov 2020 00:42:44 +0530 Subject: [PATCH] FIX: hide sso email behind a button click and log views (#11186) --- .../addon/controllers/admin-user-index.js | 11 ++++++++ .../admin/addon/templates/user-index.hbs | 13 +++++++-- app/controllers/users_controller.rb | 18 +++++++++++- .../single_sign_on_record_serializer.rb | 7 +---- config/locales/server.en.yml | 1 + config/routes.rb | 1 + lib/guardian/user_guardian.rb | 4 +++ spec/requests/users_controller_spec.rb | 28 +++++++++++++++++++ .../single_sign_on_record_serializer_spec.rb | 14 ---------- 9 files changed, 74 insertions(+), 23 deletions(-) diff --git a/app/assets/javascripts/admin/addon/controllers/admin-user-index.js b/app/assets/javascripts/admin/addon/controllers/admin-user-index.js index 8ab62307fd0..121324711c7 100644 --- a/app/assets/javascripts/admin/addon/controllers/admin-user-index.js +++ b/app/assets/javascripts/admin/addon/controllers/admin-user-index.js @@ -19,6 +19,7 @@ export default Controller.extend(CanCheckEmails, { customGroupIdsBuffer: null, availableGroups: null, userTitleValue: null, + ssoExternalEmail: null, showBadges: setting("enable_badges"), hasLockedTrustLevel: notEmpty("model.manual_locked_trust_level"), @@ -339,5 +340,15 @@ export default Controller.extend(CanCheckEmails, { } ); }, + + checkSsoEmail() { + return ajax(userPath(`${this.model.username_lower}/sso-email.json`), { + data: { context: window.location.pathname }, + }).then((result) => { + if (result) { + this.set("ssoExternalEmail", result.email); + } + }); + }, }, }); diff --git a/app/assets/javascripts/admin/addon/templates/user-index.hbs b/app/assets/javascripts/admin/addon/templates/user-index.hbs index c043ce529f1..cc25a5b0cab 100644 --- a/app/assets/javascripts/admin/addon/templates/user-index.hbs +++ b/app/assets/javascripts/admin/addon/templates/user-index.hbs @@ -671,10 +671,19 @@
{{i18n "admin.user.sso.external_name"}}
{{sso.external_name}}
- {{#if sso.external_email}} + {{#if canAdminCheckEmails}}
{{i18n "admin.user.sso.external_email"}}
-
{{sso.external_email}}
+ {{#if ssoExternalEmail}} +
{{ssoExternalEmail}}
+ {{else}} + {{d-button + class="btn-default" + action=(action "checkSsoEmail") + actionParam=model icon="far-envelope" + label="admin.users.check_email.text" + title="admin.users.check_email.title"}} + {{/if}}
{{/if}}
diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index a98c986819a..8beea25b2f2 100644 --- a/app/controllers/users_controller.rb +++ b/app/controllers/users_controller.rb @@ -11,7 +11,7 @@ class UsersController < ApplicationController :update_second_factor, :create_second_factor_backup, :select_avatar, :notification_level, :revoke_auth_token, :register_second_factor_security_key, :create_second_factor_security_key, :feature_topic, :clear_featured_topic, - :bookmarks, :invited, :invite_links + :bookmarks, :invited, :invite_links, :check_sso_email ] skip_before_action :check_xhr, only: [ @@ -206,6 +206,22 @@ class UsersController < ApplicationController render json: failed_json, status: 403 end + def check_sso_email + user = fetch_user_from_params(include_inactive: true) + + unless user == current_user + guardian.ensure_can_check_sso_email!(user) + StaffActionLogger.new(current_user).log_check_email(user, context: params[:context]) + end + + email = user&.single_sign_on_record&.external_email + email = I18n.t("user.email.does_not_exist") if email.blank? + + render json: { email: email } + rescue Discourse::InvalidAccess + render json: failed_json, status: 403 + end + def update_primary_email if !SiteSetting.enable_secondary_emails return render json: failed_json, status: 410 diff --git a/app/serializers/single_sign_on_record_serializer.rb b/app/serializers/single_sign_on_record_serializer.rb index bf33523a35e..f9a9241e732 100644 --- a/app/serializers/single_sign_on_record_serializer.rb +++ b/app/serializers/single_sign_on_record_serializer.rb @@ -4,12 +4,7 @@ class SingleSignOnRecordSerializer < ApplicationSerializer attributes :user_id, :external_id, :last_payload, :created_at, :updated_at, :external_username, - :external_email, :external_name, - :external_avatar_url, + :external_name, :external_avatar_url, :external_profile_background_url, :external_card_background_url - - def include_external_email? - scope.is_admin? - end end diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml index ece6fe50595..66bfadabde3 100644 --- a/config/locales/server.en.yml +++ b/config/locales/server.en.yml @@ -2501,6 +2501,7 @@ en: not_allowed: "is not allowed from that email provider. Please use another email address." blocked: "is not allowed." revoked: "Won't be sending emails to '%{email}' until %{date}." + does_not_exist: "N/A" ip_address: blocked: "New registrations are not allowed from your IP address." max_new_accounts_per_registration_ip: "New registrations are not allowed from your IP address (maximum limit reached). Contact a staff member." diff --git a/config/routes.rb b/config/routes.rb index b9df3de5516..a752a2cae5b 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -446,6 +446,7 @@ Discourse::Application.routes.draw do get({ "#{root_path}/:username" => "users#show", constraints: { username: RouteFormat.username } }.merge(index == 1 ? { as: 'user' } : {})) put "#{root_path}/:username" => "users#update", constraints: { username: RouteFormat.username }, defaults: { format: :json } get "#{root_path}/:username/emails" => "users#check_emails", constraints: { username: RouteFormat.username } + get "#{root_path}/:username/sso-email" => "users#check_sso_email", constraints: { username: RouteFormat.username } get "#{root_path}/:username/preferences" => "users#preferences", constraints: { username: RouteFormat.username } get "#{root_path}/:username/preferences/email" => "users_email#index", constraints: { username: RouteFormat.username } get "#{root_path}/:username/preferences/account" => "users#preferences", constraints: { username: RouteFormat.username } diff --git a/lib/guardian/user_guardian.rb b/lib/guardian/user_guardian.rb index a133e82f188..c26ec89aa8e 100644 --- a/lib/guardian/user_guardian.rb +++ b/lib/guardian/user_guardian.rb @@ -92,6 +92,10 @@ module UserGuardian is_admin? || (is_staff? && SiteSetting.moderators_view_emails) end + def can_check_sso_email?(user) + user && is_admin? + end + def restrict_user_fields?(user) user.trust_level == TrustLevel[0] && anonymous? end diff --git a/spec/requests/users_controller_spec.rb b/spec/requests/users_controller_spec.rb index 261bd59d793..bc0f0a054b5 100644 --- a/spec/requests/users_controller_spec.rb +++ b/spec/requests/users_controller_spec.rb @@ -2667,6 +2667,34 @@ describe UsersController do end end + describe '#check_sso_email' do + it 'raises an error when not logged in' do + get "/u/zogstrip/sso-email.json" + expect(response.status).to eq(403) + end + + context 'while logged in' do + let(:sign_in_admin) { sign_in(Fabricate(:admin)) } + let(:user) { Fabricate(:user) } + + it "raises an error when you aren't allowed to check sso email" do + sign_in(Fabricate(:user)) + get "/u/#{user.username}/sso-email.json" + expect(response).to be_forbidden + end + + it "returns emails and associated_accounts when you're allowed to see them" do + user.single_sign_on_record = SingleSignOnRecord.create(user_id: user.id, external_email: "foobar@example.com", external_id: "example", last_payload: "looks good") + sign_in_admin + + get "/u/#{user.username}/sso-email.json" + + expect(response.status).to eq(200) + expect(response.parsed_body["email"]).to eq("foobar@example.com") + end + end + end + describe '#update_primary_email' do fab!(:user) { Fabricate(:user) } fab!(:user_email) { user.primary_email } diff --git a/spec/serializers/single_sign_on_record_serializer_spec.rb b/spec/serializers/single_sign_on_record_serializer_spec.rb index 6af81b0f4f7..fffb6dff6bf 100644 --- a/spec/serializers/single_sign_on_record_serializer_spec.rb +++ b/spec/serializers/single_sign_on_record_serializer_spec.rb @@ -14,20 +14,6 @@ RSpec.describe SingleSignOnRecordSerializer do SingleSignOnRecordSerializer.new(sso, scope: Guardian.new(admin), root: false) end - it "should include user sso info" do - payload = serializer.as_json - expect(payload[:user_id]).to eq(user.id) - expect(payload[:external_id]).to eq('12345') - expect(payload[:external_email]).to eq(user.email) - end - end - - context "moderator" do - fab!(:moderator) { Fabricate(:moderator) } - let :serializer do - SingleSignOnRecordSerializer.new(sso, scope: Guardian.new(moderator), root: false) - end - it "should include user sso info" do payload = serializer.as_json expect(payload[:user_id]).to eq(user.id)