Skip to content

Commit

Permalink
FIX: hide sso email behind a button click and log views (#11186)
Browse files Browse the repository at this point in the history
  • Loading branch information
arpitjalan committed Nov 10, 2020
1 parent cf4be10 commit 00b4143
Show file tree
Hide file tree
Showing 9 changed files with 74 additions and 23 deletions.
11 changes: 11 additions & 0 deletions app/assets/javascripts/admin/addon/controllers/admin-user-index.js
Expand Up @@ -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"),
Expand Down Expand Up @@ -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);
}
});
},
},
});
13 changes: 11 additions & 2 deletions app/assets/javascripts/admin/addon/templates/user-index.hbs
Expand Up @@ -671,10 +671,19 @@
<div class="field">{{i18n "admin.user.sso.external_name"}}</div>
<div class="value">{{sso.external_name}}</div>
</div>
{{#if sso.external_email}}
{{#if canAdminCheckEmails}}
<div class="display-row">
<div class="field">{{i18n "admin.user.sso.external_email"}}</div>
<div class="value">{{sso.external_email}}</div>
{{#if ssoExternalEmail}}
<div class="value">{{ssoExternalEmail}}</div>
{{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}}
</div>
{{/if}}
<div class="display-row">
Expand Down
18 changes: 17 additions & 1 deletion app/controllers/users_controller.rb
Expand Up @@ -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: [
Expand Down Expand Up @@ -206,6 +206,22 @@ def check_emails
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
Expand Down
7 changes: 1 addition & 6 deletions app/serializers/single_sign_on_record_serializer.rb
Expand Up @@ -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
1 change: 1 addition & 0 deletions config/locales/server.en.yml
Expand Up @@ -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."
Expand Down
1 change: 1 addition & 0 deletions config/routes.rb
Expand Up @@ -446,6 +446,7 @@
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 }
Expand Down
4 changes: 4 additions & 0 deletions lib/guardian/user_guardian.rb
Expand Up @@ -92,6 +92,10 @@ def can_check_emails?(user)
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
Expand Down
28 changes: 28 additions & 0 deletions spec/requests/users_controller_spec.rb
Expand Up @@ -2667,6 +2667,34 @@ def post_user(extra_params = {})
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 }
Expand Down
14 changes: 0 additions & 14 deletions spec/serializers/single_sign_on_record_serializer_spec.rb
Expand Up @@ -14,20 +14,6 @@
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)
Expand Down

2 comments on commit 00b4143

@discoursebot
Copy link

Choose a reason for hiding this comment

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

This commit has been mentioned on Discourse Meta. There might be relevant details there:

https://meta.discourse.org/t/user-email-is-not-hidden-under-single-sign-on-area-of-admin-page/160657/7

@discoursebot
Copy link

Choose a reason for hiding this comment

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

This commit has been mentioned on Discourse Meta. There might be relevant details there:

https://meta.discourse.org/t/moderators-should-not-see-emails-in-sso-section/81356/13

Please sign in to comment.