FIX: Users should able check the emails for self

FIX: Users should able check the emails for self

diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb
index fe87d7d..5d7c99b 100644
--- a/app/controllers/users_controller.rb
+++ b/app/controllers/users_controller.rb
@@ -148,9 +148,11 @@ class UsersController < ApplicationController
 
   def check_emails
     user = fetch_user_from_params(include_inactive: true)
-    guardian.ensure_can_check_emails!(user)
 
-    StaffActionLogger.new(current_user).log_check_email(user, context: params[:context])
+    unless user == current_user
+      guardian.ensure_can_check_emails!(user)
+      StaffActionLogger.new(current_user).log_check_email(user, context: params[:context])
+    end
 
     email, *secondary_emails = user.emails
 
diff --git a/spec/requests/users_controller_spec.rb b/spec/requests/users_controller_spec.rb
index 6752147..11dda38 100644
--- a/spec/requests/users_controller_spec.rb
+++ b/spec/requests/users_controller_spec.rb
@@ -2072,6 +2072,19 @@ describe UsersController do
         expect(response).to be_forbidden
       end
 
+      it "returns emails and associated_accounts for self" do
+        user = Fabricate(:user)
+        sign_in(user)
+
+        get "/u/#{user.username}/emails.json"
+
+        expect(response.status).to eq(200)
+        json = JSON.parse(response.body)
+        expect(json["email"]).to eq(user.email)
+        expect(json["secondary_emails"]).to eq(user.secondary_emails)
+        expect(json["associated_accounts"]).to eq([])
+      end
+
       it "returns emails and associated_accounts when you're allowed to see them" do
         user = Fabricate(:user)
         sign_in_admin

GitHub sha: e7821a63

1 Like

I’m curious why this was required? The user email should be included in the current user serializer.

api.getCurrentUser().findDetails().then(user => {
  console.log(user.get("email"))
});

Associated accounts should be there as well.

No, current user serializer doesn’t have these values since it inherits the BasicUserSerializer. Maybe we should include the emails in current user serializer too.

Sorry, I meant “The user serializer for the current user”, rather than “CurrentUserSerializer”. Email and associated accounts are included there, because we display them in the user preferences.

Yes, currentUser.findDetails() method working fine. But it also demands a network request (/u/USERNAME.json) to get email field value same like currentUser.checkEmail(). That’s why I used it in theme code and later realized that it won’t work for non-staff users. Anyway in API perspective it should be possible to check emails and I shouldn’t get logged for my own email checks right?

check-email

I am able to understand your concern. Previously checkEmail method was only allowed for staffs. If it’s not good for security then I will revert the commit and will use findDetails method instead.

1 Like

Ah I see, so the checkEmail response is going to be slightly smaller/faster than than the findDetails response :+1:

It is no problem for security - the change looks good to me, and I agree there is no need to log checking your own email. Thanks for explaining :heart:

2 Likes