diff --git a/CHANGELOG b/CHANGELOG index 4a299827c1112756699184d29a1e2b152598eb55..c353c3b770c993023e5dfd9d577fce6b2888dea3 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -35,6 +35,7 @@ v 7.0.0 - Be more selective when killing stray Sidekiqs - Check LDAP user filter during sign-in - Remove wall feature (no data loss - you can take it from database) + - Dont expose user emails via API unless you are admin v 6.9.2 - Revert the commit that broke the LDAP user filter diff --git a/app/assets/javascripts/project_users_select.js.coffee b/app/assets/javascripts/project_users_select.js.coffee index 382f9b37992176811e42e7ce4276e33180a7afd0..cfbcd5108c83237dd3d7103ed4ba479df12e6296 100644 --- a/app/assets/javascripts/project_users_select.js.coffee +++ b/app/assets/javascripts/project_users_select.js.coffee @@ -37,13 +37,9 @@ projectUserFormatResult: (user) -> if user.avatar_url - avatar = gon.relative_url_root + user.avatar_url - else if gon.gravatar_enabled - avatar = gon.gravatar_url - avatar = avatar.replace('%{hash}', md5(user.email)) - avatar = avatar.replace('%{size}', '24') + avatar = user.avatar_url else - avatar = gon.relative_url_root + "#{image_path('no_avatar.png')}" + avatar = gon.default_avatar_url if user.id == '' avatarMarkup = '' diff --git a/app/assets/javascripts/users_select.js.coffee b/app/assets/javascripts/users_select.js.coffee index da66a4ba7f2971825f81e6524c2ca4cb8640bb64..86318bd7d94849e14fa2b7bacde4b24d3f13b1f7 100644 --- a/app/assets/javascripts/users_select.js.coffee +++ b/app/assets/javascripts/users_select.js.coffee @@ -1,13 +1,9 @@ $ -> userFormatResult = (user) -> if user.avatar_url - avatar = gon.relative_url_root + user.avatar_url - else if gon.gravatar_enabled - avatar = gon.gravatar_url - avatar = avatar.replace('%{hash}', md5(user.email)) - avatar = avatar.replace('%{size}', '24') + avatar = user.avatar_url else - avatar = gon.relative_url_root + "#{image_path('no_avatar.png')}" + avatar = gon.default_avatar_url "<div class='user-result'> <div class='user-image'><img class='avatar s24' src='#{avatar}'></div> diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index 685d41a55205eece7fe16bc0a9fa980797faa187..603e89a5e290355c21fd00b300e9d66d8aa5bbb7 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -164,9 +164,8 @@ def default_headers def add_gon_variables gon.default_issues_tracker = Project.issues_tracker.default_value gon.api_version = API::API.version - gon.gravatar_url = request.ssl? || Gitlab.config.gitlab.https ? Gitlab.config.gravatar.ssl_url : Gitlab.config.gravatar.plain_url gon.relative_url_root = Gitlab.config.gitlab.relative_url_root - gon.gravatar_enabled = Gitlab.config.gravatar.enabled + gon.default_avatar_url = URI::join(Gitlab.config.gitlab.url, ActionController::Base.helpers.image_path('no_avatar.png')).to_s if current_user gon.current_user_id = current_user.id diff --git a/app/helpers/application_helper.rb b/app/helpers/application_helper.rb index 13120d2e581a1a9853e05f71af21980778c58eca..c3d89eb1b82d59cb3bdc55fa3f5f35ad44b9f3af 100644 --- a/app/helpers/application_helper.rb +++ b/app/helpers/application_helper.rb @@ -60,23 +60,21 @@ def group_icon(group_path) def avatar_icon(user_email = '', size = nil) user = User.find_by(email: user_email) - if user && user.avatar.present? - user.avatar.url + + if user + user.avatar_url(size) || default_avatar else gravatar_icon(user_email, size) end end def gravatar_icon(user_email = '', size = nil) - size = 40 if size.nil? || size <= 0 + GravatarService.new.execute(user_email, size) || + default_avatar + end - if !Gitlab.config.gravatar.enabled || user_email.blank? - image_path('no_avatar.png') - else - gravatar_url = request.ssl? || gitlab_config.https ? Gitlab.config.gravatar.ssl_url : Gitlab.config.gravatar.plain_url - user_email.strip! - sprintf gravatar_url, hash: Digest::MD5.hexdigest(user_email.downcase), size: size, email: user_email - end + def default_avatar + image_path('no_avatar.png') end def last_commit(project) diff --git a/app/models/user.rb b/app/models/user.rb index 0fbc9284dd80432f32a2aa5eb48dd157dbcc67ba..2352f8c050b8890e0d7b1cdd27032b74d7a790e2 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -482,4 +482,12 @@ def generate_tmp_oauth_email def public_profile? authorized_projects.public_only.any? end + + def avatar_url(size = nil) + if avatar.present? + URI::join(Gitlab.config.gitlab.url, avatar.url).to_s + else + GravatarService.new.execute(email, size) + end + end end diff --git a/app/services/gravatar_service.rb b/app/services/gravatar_service.rb new file mode 100644 index 0000000000000000000000000000000000000000..a69c7c78377e13739a68ae45533c51699259cf49 --- /dev/null +++ b/app/services/gravatar_service.rb @@ -0,0 +1,28 @@ +class GravatarService + def execute(email, size = nil) + if gravatar_config.enabled && email.present? + size = 40 if size.nil? || size <= 0 + + sprintf gravatar_url, + hash: Digest::MD5.hexdigest(email.strip.downcase), + size: size, + email: email.strip + end + end + + def gitlab_config + Gitlab.config.gitlab + end + + def gravatar_config + Gitlab.config.gravatar + end + + def gravatar_url + if gitlab_config.https + gravatar_config.ssl_url + else + gravatar_config.plain_url + end + end +end diff --git a/doc/api/users.md b/doc/api/users.md index 94af37629ff9e7a5fb491432f0b6e0d38e3d5718..4ddbf7397745b6f5d8865b346b3e7d88e581d963 100644 --- a/doc/api/users.md +++ b/doc/api/users.md @@ -6,6 +6,34 @@ Get a list of users. This function takes pagination parameters `page` and `per_page` to restrict the list of users. +### For normal users: + +``` +GET /users +``` + +```json +[ + { + "id": 1, + "username": "john_smith", + "name": "John Smith", + "state": "active", + "avatar_url": "http://localhost:3000/uploads/user/avatar/1/cd8.jpeg", + }, + { + "id": 2, + "username": "jack_smith", + "name": "Jack Smith", + "state": "blocked", + "avatar_url": "http://gravatar.com/../e32131cd8.jpeg", + } +] +``` + + +### For admins: + ``` GET /users ``` @@ -29,6 +57,7 @@ GET /users "theme_id": 1, "color_scheme_id": 2, "is_admin": false, + "avatar_url": "http://localhost:3000/uploads/user/avatar/1/cd8.jpeg", "can_create_group": true }, { @@ -48,6 +77,7 @@ GET /users "theme_id": 1, "color_scheme_id": 3, "is_admin": false, + "avatar_url": "http://localhost:3000/uploads/user/avatar/1/cd8.jpeg", "can_create_group": true, "can_create_project": true } @@ -62,6 +92,29 @@ Also see `def search query` in `app/models/user.rb`. Get a single user. +#### For user: + +``` +GET /users/:id +``` + +Parameters: + +- `id` (required) - The ID of a user + +```json +{ + "id": 1, + "username": "john_smith", + "name": "John Smith", + "state": "active", + "avatar_url": "http://localhost:3000/uploads/user/avatar/1/cd8.jpeg", +} +``` + + +#### For admin: + ``` GET /users/:id ``` diff --git a/lib/api/entities.rb b/lib/api/entities.rb index f15fe185ae0a30e60faf916d4388382ac759af0e..b190646a1e3188186dea2169f5b87bce5130392e 100644 --- a/lib/api/entities.rb +++ b/lib/api/entities.rb @@ -1,28 +1,27 @@ module API module Entities - class User < Grape::Entity - expose :id, :username, :email, :name, :bio, :skype, :linkedin, :twitter, :website_url, - :theme_id, :color_scheme_id, :state, :created_at, :extern_uid, :provider - expose :is_admin?, as: :is_admin - expose :can_create_group?, as: :can_create_group - expose :can_create_project?, as: :can_create_project + class UserSafe < Grape::Entity + expose :name, :username + end - expose :avatar_url do |user, options| - if user.avatar.present? - user.avatar.url - end - end + class UserBasic < UserSafe + expose :id, :state, :avatar_url end - class UserSafe < Grape::Entity - expose :name, :username + class User < UserBasic + expose :created_at + expose :is_admin?, as: :is_admin + expose :bio, :skype, :linkedin, :twitter, :website_url end - class UserBasic < Grape::Entity - expose :id, :username, :email, :name, :state, :created_at + class UserFull < User + expose :email + expose :theme_id, :color_scheme_id, :extern_uid, :provider + expose :can_create_group?, as: :can_create_group + expose :can_create_project?, as: :can_create_project end - class UserLogin < User + class UserLogin < UserFull expose :private_token end diff --git a/lib/api/internal.rb b/lib/api/internal.rb index 06c66ba0b352c8f2b7b49b91465f09627ddfb1e6..5850892df07eed2ae9cebbd490afbdc3b46fd011 100644 --- a/lib/api/internal.rb +++ b/lib/api/internal.rb @@ -59,4 +59,3 @@ class Internal < Grape::API end end end - diff --git a/lib/api/projects.rb b/lib/api/projects.rb index 9a7f22b536f15033fda83cad822c3f8675b06ff0..732c969d7ef0398491b86d3129109aaa67a73db3 100644 --- a/lib/api/projects.rb +++ b/lib/api/projects.rb @@ -209,7 +209,7 @@ def map_public_to_visibility_level(attrs) @users = User.where(id: user_project.team.users.map(&:id)) @users = @users.search(params[:search]) if params[:search].present? @users = paginate @users - present @users, with: Entities::User + present @users, with: Entities::UserBasic end # Get a project labels diff --git a/lib/api/users.rb b/lib/api/users.rb index 6ed2740c3330281e75071333b15dfec5a17ee5c7..92dbe97f0a468b18ecb423aa324c7f7c5fd71279 100644 --- a/lib/api/users.rb +++ b/lib/api/users.rb @@ -13,7 +13,12 @@ class Users < Grape::API @users = @users.active if params[:active].present? @users = @users.search(params[:search]) if params[:search].present? @users = paginate @users - present @users, with: Entities::User + + if current_user.is_admin? + present @users, with: Entities::UserFull + else + present @users, with: Entities::UserBasic + end end # Get a single user @@ -24,7 +29,12 @@ class Users < Grape::API # GET /users/:id get ":id" do @user = User.find(params[:id]) - present @user, with: Entities::User + + if current_user.is_admin? + present @user, with: Entities::UserFull + else + present @user, with: Entities::UserBasic + end end # Create user. Available only for admin @@ -53,7 +63,7 @@ class Users < Grape::API admin = attrs.delete(:admin) user.admin = admin unless admin.nil? if user.save - present user, with: Entities::User + present user, with: Entities::UserFull else not_found! end @@ -87,7 +97,7 @@ class Users < Grape::API admin = attrs.delete(:admin) user.admin = admin unless admin.nil? if user.update_attributes(attrs, as: :admin) - present user, with: Entities::User + present user, with: Entities::UserFull else not_found! end diff --git a/spec/helpers/application_helper_spec.rb b/spec/helpers/application_helper_spec.rb index 6401ec0710ac7461eca9c65e940c09dcb56a3d96..6a11414a0239fe9115796a62f757c3ba7a601e7b 100644 --- a/spec/helpers/application_helper_spec.rb +++ b/spec/helpers/application_helper_spec.rb @@ -67,10 +67,9 @@ end it "should call gravatar_icon when no avatar is present" do - user = create(:user) + user = create(:user, email: 'test@example.com') user.save! - allow(self).to receive(:gravatar_icon).and_return('gravatar_method_called') - avatar_icon(user.email).to_s.should == "gravatar_method_called" + avatar_icon(user.email).to_s.should == "http://www.gravatar.com/avatar/55502f40dc8b7c769880b10874abc9d0?s=40&d=mm" end end @@ -87,12 +86,12 @@ end it "should return default gravatar url" do - allow(self).to receive(:request).and_return(double(:ssl? => false)) + Gitlab.config.gitlab.stub(https: false) gravatar_icon(user_email).should match('http://www.gravatar.com/avatar/b58c6f14d292556214bd64909bcdb118') end it "should use SSL when appropriate" do - allow(self).to receive(:request).and_return(double(:ssl? => true)) + Gitlab.config.gitlab.stub(https: true) gravatar_icon(user_email).should match('https://secure.gravatar.com') end diff --git a/spec/requests/api/notes_spec.rb b/spec/requests/api/notes_spec.rb index 117961834745d266dc290c134899d5f774d059b9..2875db04ee45a22fd429829ea498a4439a17357d 100644 --- a/spec/requests/api/notes_spec.rb +++ b/spec/requests/api/notes_spec.rb @@ -93,7 +93,7 @@ post api("/projects/#{project.id}/issues/#{issue.id}/notes", user), body: 'hi!' response.status.should == 201 json_response['body'].should == 'hi!' - json_response['author']['email'].should == user.email + json_response['author']['username'].should == user.username end it "should return a 400 bad request error if body not given" do @@ -112,7 +112,7 @@ post api("/projects/#{project.id}/snippets/#{snippet.id}/notes", user), body: 'hi!' response.status.should == 201 json_response['body'].should == 'hi!' - json_response['author']['email'].should == user.email + json_response['author']['username'].should == user.username end it "should return a 400 bad request error if body not given" do diff --git a/spec/requests/api/project_members_spec.rb b/spec/requests/api/project_members_spec.rb index ec2d6e850966c38e582d00b830d72c50f72c7fbc..032f850010c7a46ab94f78f9f2347728d7c634ca 100644 --- a/spec/requests/api/project_members_spec.rb +++ b/spec/requests/api/project_members_spec.rb @@ -21,7 +21,7 @@ response.status.should == 200 json_response.should be_an Array json_response.count.should == 2 - json_response.map { |u| u['email'] }.should include user.email + json_response.map { |u| u['username'] }.should include user.username end it "finds team members with query string" do @@ -29,7 +29,7 @@ response.status.should == 200 json_response.should be_an Array json_response.count.should == 1 - json_response.first['email'].should == user.email + json_response.first['username'].should == user.username end it "should return a 404 error if id not found" do @@ -44,7 +44,7 @@ it "should return project team member" do get api("/projects/#{project.id}/members/#{user.id}", user) response.status.should == 200 - json_response['email'].should == user.email + json_response['username'].should == user.username json_response['access_level'].should == UsersProject::MASTER end @@ -62,7 +62,7 @@ }.to change { UsersProject.count }.by(1) response.status.should == 201 - json_response['email'].should == user2.email + json_response['username'].should == user2.username json_response['access_level'].should == UsersProject::DEVELOPER end @@ -75,7 +75,7 @@ }.not_to change { UsersProject.count }.by(1) response.status.should == 201 - json_response['email'].should == user2.email + json_response['username'].should == user2.username json_response['access_level'].should == UsersProject::DEVELOPER end @@ -101,7 +101,7 @@ it "should update project team member" do put api("/projects/#{project.id}/members/#{user3.id}", user), access_level: UsersProject::MASTER response.status.should == 200 - json_response['email'].should == user3.email + json_response['username'].should == user3.username json_response['access_level'].should == UsersProject::MASTER end diff --git a/spec/requests/api/projects_spec.rb b/spec/requests/api/projects_spec.rb index 43915d8684b647d87cc58b02f301e7eff7b5cb7e..415735091c353899a4786b972640979ae6d82320 100644 --- a/spec/requests/api/projects_spec.rb +++ b/spec/requests/api/projects_spec.rb @@ -37,7 +37,7 @@ response.status.should == 200 json_response.should be_an Array json_response.first['name'].should == project.name - json_response.first['owner']['email'].should == user.email + json_response.first['owner']['username'].should == user.username end end end @@ -65,7 +65,7 @@ response.status.should == 200 json_response.should be_an Array json_response.first['name'].should == project.name - json_response.first['owner']['email'].should == user.email + json_response.first['owner']['username'].should == user.username end end end @@ -270,7 +270,7 @@ get api("/projects/#{project.id}", user) response.status.should == 200 json_response['name'].should == project.name - json_response['owner']['email'].should == user.email + json_response['owner']['username'].should == user.username end it "should return a project by path name" do diff --git a/spec/requests/api/users_spec.rb b/spec/requests/api/users_spec.rb index a6d300b099b21e2a6626fe6d746a2d89ca87b11e..c3eec56d133c44733514f4473ab3120ca9d3756c 100644 --- a/spec/requests/api/users_spec.rb +++ b/spec/requests/api/users_spec.rb @@ -20,7 +20,18 @@ get api("/users", user) response.status.should == 200 json_response.should be_an Array - json_response.first['email'].should == user.email + json_response.first['username'].should == user.username + end + end + + context "when admin" do + it "should return an array of users" do + get api("/users", admin) + response.status.should == 200 + json_response.should be_an Array + json_response.first.keys.should include 'email' + json_response.first.keys.should include 'extern_uid' + json_response.first.keys.should include 'can_create_project' end end end @@ -29,7 +40,7 @@ it "should return a user by id" do get api("/users/#{user.id}", user) response.status.should == 200 - json_response['email'].should == user.email + json_response['username'].should == user.username end it "should return a 401 if unauthenticated" do