Commit 87fd342a2317567f7854570dd5624dd64dffebd4

Authored by Dmitriy Zaporozhets
2 parents 2af8ace1 47d6f705

Merge branch 'more-secure-api' into 'master'

More secure api

Dont expose user email via API. Fixes #1314
@@ -35,6 +35,7 @@ v 7.0.0 @@ -35,6 +35,7 @@ v 7.0.0
35 - Be more selective when killing stray Sidekiqs 35 - Be more selective when killing stray Sidekiqs
36 - Check LDAP user filter during sign-in 36 - Check LDAP user filter during sign-in
37 - Remove wall feature (no data loss - you can take it from database) 37 - Remove wall feature (no data loss - you can take it from database)
  38 + - Dont expose user emails via API unless you are admin
38 39
39 v 6.9.2 40 v 6.9.2
40 - Revert the commit that broke the LDAP user filter 41 - Revert the commit that broke the LDAP user filter
app/assets/javascripts/project_users_select.js.coffee
@@ -37,13 +37,9 @@ @@ -37,13 +37,9 @@
37 37
38 projectUserFormatResult: (user) -> 38 projectUserFormatResult: (user) ->
39 if user.avatar_url 39 if user.avatar_url
40 - avatar = gon.relative_url_root + user.avatar_url  
41 - else if gon.gravatar_enabled  
42 - avatar = gon.gravatar_url  
43 - avatar = avatar.replace('%{hash}', md5(user.email))  
44 - avatar = avatar.replace('%{size}', '24') 40 + avatar = user.avatar_url
45 else 41 else
46 - avatar = gon.relative_url_root + "#{image_path('no_avatar.png')}" 42 + avatar = gon.default_avatar_url
47 43
48 if user.id == '' 44 if user.id == ''
49 avatarMarkup = '' 45 avatarMarkup = ''
app/assets/javascripts/users_select.js.coffee
1 $ -> 1 $ ->
2 userFormatResult = (user) -> 2 userFormatResult = (user) ->
3 if user.avatar_url 3 if user.avatar_url
4 - avatar = gon.relative_url_root + user.avatar_url  
5 - else if gon.gravatar_enabled  
6 - avatar = gon.gravatar_url  
7 - avatar = avatar.replace('%{hash}', md5(user.email))  
8 - avatar = avatar.replace('%{size}', '24') 4 + avatar = user.avatar_url
9 else 5 else
10 - avatar = gon.relative_url_root + "#{image_path('no_avatar.png')}" 6 + avatar = gon.default_avatar_url
11 7
12 "<div class='user-result'> 8 "<div class='user-result'>
13 <div class='user-image'><img class='avatar s24' src='#{avatar}'></div> 9 <div class='user-image'><img class='avatar s24' src='#{avatar}'></div>
app/controllers/application_controller.rb
@@ -164,9 +164,8 @@ class ApplicationController &lt; ActionController::Base @@ -164,9 +164,8 @@ class ApplicationController &lt; ActionController::Base
164 def add_gon_variables 164 def add_gon_variables
165 gon.default_issues_tracker = Project.issues_tracker.default_value 165 gon.default_issues_tracker = Project.issues_tracker.default_value
166 gon.api_version = API::API.version 166 gon.api_version = API::API.version
167 - gon.gravatar_url = request.ssl? || Gitlab.config.gitlab.https ? Gitlab.config.gravatar.ssl_url : Gitlab.config.gravatar.plain_url  
168 gon.relative_url_root = Gitlab.config.gitlab.relative_url_root 167 gon.relative_url_root = Gitlab.config.gitlab.relative_url_root
169 - gon.gravatar_enabled = Gitlab.config.gravatar.enabled 168 + gon.default_avatar_url = URI::join(Gitlab.config.gitlab.url, ActionController::Base.helpers.image_path('no_avatar.png')).to_s
170 169
171 if current_user 170 if current_user
172 gon.current_user_id = current_user.id 171 gon.current_user_id = current_user.id
app/helpers/application_helper.rb
@@ -60,23 +60,21 @@ module ApplicationHelper @@ -60,23 +60,21 @@ module ApplicationHelper
60 60
61 def avatar_icon(user_email = '', size = nil) 61 def avatar_icon(user_email = '', size = nil)
62 user = User.find_by(email: user_email) 62 user = User.find_by(email: user_email)
63 - if user && user.avatar.present?  
64 - user.avatar.url 63 +
  64 + if user
  65 + user.avatar_url(size) || default_avatar
65 else 66 else
66 gravatar_icon(user_email, size) 67 gravatar_icon(user_email, size)
67 end 68 end
68 end 69 end
69 70
70 def gravatar_icon(user_email = '', size = nil) 71 def gravatar_icon(user_email = '', size = nil)
71 - size = 40 if size.nil? || size <= 0 72 + GravatarService.new.execute(user_email, size) ||
  73 + default_avatar
  74 + end
72 75
73 - if !Gitlab.config.gravatar.enabled || user_email.blank?  
74 - image_path('no_avatar.png')  
75 - else  
76 - gravatar_url = request.ssl? || gitlab_config.https ? Gitlab.config.gravatar.ssl_url : Gitlab.config.gravatar.plain_url  
77 - user_email.strip!  
78 - sprintf gravatar_url, hash: Digest::MD5.hexdigest(user_email.downcase), size: size, email: user_email  
79 - end 76 + def default_avatar
  77 + image_path('no_avatar.png')
80 end 78 end
81 79
82 def last_commit(project) 80 def last_commit(project)
app/models/user.rb
@@ -482,4 +482,12 @@ class User &lt; ActiveRecord::Base @@ -482,4 +482,12 @@ class User &lt; ActiveRecord::Base
482 def public_profile? 482 def public_profile?
483 authorized_projects.public_only.any? 483 authorized_projects.public_only.any?
484 end 484 end
  485 +
  486 + def avatar_url(size = nil)
  487 + if avatar.present?
  488 + URI::join(Gitlab.config.gitlab.url, avatar.url).to_s
  489 + else
  490 + GravatarService.new.execute(email, size)
  491 + end
  492 + end
485 end 493 end
app/services/gravatar_service.rb 0 → 100644
@@ -0,0 +1,28 @@ @@ -0,0 +1,28 @@
  1 +class GravatarService
  2 + def execute(email, size = nil)
  3 + if gravatar_config.enabled && email.present?
  4 + size = 40 if size.nil? || size <= 0
  5 +
  6 + sprintf gravatar_url,
  7 + hash: Digest::MD5.hexdigest(email.strip.downcase),
  8 + size: size,
  9 + email: email.strip
  10 + end
  11 + end
  12 +
  13 + def gitlab_config
  14 + Gitlab.config.gitlab
  15 + end
  16 +
  17 + def gravatar_config
  18 + Gitlab.config.gravatar
  19 + end
  20 +
  21 + def gravatar_url
  22 + if gitlab_config.https
  23 + gravatar_config.ssl_url
  24 + else
  25 + gravatar_config.plain_url
  26 + end
  27 + end
  28 +end
doc/api/users.md
@@ -6,6 +6,34 @@ Get a list of users. @@ -6,6 +6,34 @@ Get a list of users.
6 6
7 This function takes pagination parameters `page` and `per_page` to restrict the list of users. 7 This function takes pagination parameters `page` and `per_page` to restrict the list of users.
8 8
  9 +### For normal users:
  10 +
  11 +```
  12 +GET /users
  13 +```
  14 +
  15 +```json
  16 +[
  17 + {
  18 + "id": 1,
  19 + "username": "john_smith",
  20 + "name": "John Smith",
  21 + "state": "active",
  22 + "avatar_url": "http://localhost:3000/uploads/user/avatar/1/cd8.jpeg",
  23 + },
  24 + {
  25 + "id": 2,
  26 + "username": "jack_smith",
  27 + "name": "Jack Smith",
  28 + "state": "blocked",
  29 + "avatar_url": "http://gravatar.com/../e32131cd8.jpeg",
  30 + }
  31 +]
  32 +```
  33 +
  34 +
  35 +### For admins:
  36 +
9 ``` 37 ```
10 GET /users 38 GET /users
11 ``` 39 ```
@@ -29,6 +57,7 @@ GET /users @@ -29,6 +57,7 @@ GET /users
29 "theme_id": 1, 57 "theme_id": 1,
30 "color_scheme_id": 2, 58 "color_scheme_id": 2,
31 "is_admin": false, 59 "is_admin": false,
  60 + "avatar_url": "http://localhost:3000/uploads/user/avatar/1/cd8.jpeg",
32 "can_create_group": true 61 "can_create_group": true
33 }, 62 },
34 { 63 {
@@ -48,6 +77,7 @@ GET /users @@ -48,6 +77,7 @@ GET /users
48 "theme_id": 1, 77 "theme_id": 1,
49 "color_scheme_id": 3, 78 "color_scheme_id": 3,
50 "is_admin": false, 79 "is_admin": false,
  80 + "avatar_url": "http://localhost:3000/uploads/user/avatar/1/cd8.jpeg",
51 "can_create_group": true, 81 "can_create_group": true,
52 "can_create_project": true 82 "can_create_project": true
53 } 83 }
@@ -62,6 +92,29 @@ Also see `def search query` in `app/models/user.rb`. @@ -62,6 +92,29 @@ Also see `def search query` in `app/models/user.rb`.
62 92
63 Get a single user. 93 Get a single user.
64 94
  95 +#### For user:
  96 +
  97 +```
  98 +GET /users/:id
  99 +```
  100 +
  101 +Parameters:
  102 +
  103 +- `id` (required) - The ID of a user
  104 +
  105 +```json
  106 +{
  107 + "id": 1,
  108 + "username": "john_smith",
  109 + "name": "John Smith",
  110 + "state": "active",
  111 + "avatar_url": "http://localhost:3000/uploads/user/avatar/1/cd8.jpeg",
  112 +}
  113 +```
  114 +
  115 +
  116 +#### For admin:
  117 +
65 ``` 118 ```
66 GET /users/:id 119 GET /users/:id
67 ``` 120 ```
lib/api/entities.rb
1 module API 1 module API
2 module Entities 2 module Entities
3 - class User < Grape::Entity  
4 - expose :id, :username, :email, :name, :bio, :skype, :linkedin, :twitter, :website_url,  
5 - :theme_id, :color_scheme_id, :state, :created_at, :extern_uid, :provider  
6 - expose :is_admin?, as: :is_admin  
7 - expose :can_create_group?, as: :can_create_group  
8 - expose :can_create_project?, as: :can_create_project 3 + class UserSafe < Grape::Entity
  4 + expose :name, :username
  5 + end
9 6
10 - expose :avatar_url do |user, options|  
11 - if user.avatar.present?  
12 - user.avatar.url  
13 - end  
14 - end 7 + class UserBasic < UserSafe
  8 + expose :id, :state, :avatar_url
15 end 9 end
16 10
17 - class UserSafe < Grape::Entity  
18 - expose :name, :username 11 + class User < UserBasic
  12 + expose :created_at
  13 + expose :is_admin?, as: :is_admin
  14 + expose :bio, :skype, :linkedin, :twitter, :website_url
19 end 15 end
20 16
21 - class UserBasic < Grape::Entity  
22 - expose :id, :username, :email, :name, :state, :created_at 17 + class UserFull < User
  18 + expose :email
  19 + expose :theme_id, :color_scheme_id, :extern_uid, :provider
  20 + expose :can_create_group?, as: :can_create_group
  21 + expose :can_create_project?, as: :can_create_project
23 end 22 end
24 23
25 - class UserLogin < User 24 + class UserLogin < UserFull
26 expose :private_token 25 expose :private_token
27 end 26 end
28 27
lib/api/internal.rb
@@ -59,4 +59,3 @@ module API @@ -59,4 +59,3 @@ module API
59 end 59 end
60 end 60 end
61 end 61 end
62 -  
lib/api/projects.rb
@@ -209,7 +209,7 @@ module API @@ -209,7 +209,7 @@ module API
209 @users = User.where(id: user_project.team.users.map(&:id)) 209 @users = User.where(id: user_project.team.users.map(&:id))
210 @users = @users.search(params[:search]) if params[:search].present? 210 @users = @users.search(params[:search]) if params[:search].present?
211 @users = paginate @users 211 @users = paginate @users
212 - present @users, with: Entities::User 212 + present @users, with: Entities::UserBasic
213 end 213 end
214 214
215 # Get a project labels 215 # Get a project labels
lib/api/users.rb
@@ -13,7 +13,12 @@ module API @@ -13,7 +13,12 @@ module API
13 @users = @users.active if params[:active].present? 13 @users = @users.active if params[:active].present?
14 @users = @users.search(params[:search]) if params[:search].present? 14 @users = @users.search(params[:search]) if params[:search].present?
15 @users = paginate @users 15 @users = paginate @users
16 - present @users, with: Entities::User 16 +
  17 + if current_user.is_admin?
  18 + present @users, with: Entities::UserFull
  19 + else
  20 + present @users, with: Entities::UserBasic
  21 + end
17 end 22 end
18 23
19 # Get a single user 24 # Get a single user
@@ -24,7 +29,12 @@ module API @@ -24,7 +29,12 @@ module API
24 # GET /users/:id 29 # GET /users/:id
25 get ":id" do 30 get ":id" do
26 @user = User.find(params[:id]) 31 @user = User.find(params[:id])
27 - present @user, with: Entities::User 32 +
  33 + if current_user.is_admin?
  34 + present @user, with: Entities::UserFull
  35 + else
  36 + present @user, with: Entities::UserBasic
  37 + end
28 end 38 end
29 39
30 # Create user. Available only for admin 40 # Create user. Available only for admin
@@ -53,7 +63,7 @@ module API @@ -53,7 +63,7 @@ module API
53 admin = attrs.delete(:admin) 63 admin = attrs.delete(:admin)
54 user.admin = admin unless admin.nil? 64 user.admin = admin unless admin.nil?
55 if user.save 65 if user.save
56 - present user, with: Entities::User 66 + present user, with: Entities::UserFull
57 else 67 else
58 not_found! 68 not_found!
59 end 69 end
@@ -87,7 +97,7 @@ module API @@ -87,7 +97,7 @@ module API
87 admin = attrs.delete(:admin) 97 admin = attrs.delete(:admin)
88 user.admin = admin unless admin.nil? 98 user.admin = admin unless admin.nil?
89 if user.update_attributes(attrs, as: :admin) 99 if user.update_attributes(attrs, as: :admin)
90 - present user, with: Entities::User 100 + present user, with: Entities::UserFull
91 else 101 else
92 not_found! 102 not_found!
93 end 103 end
spec/helpers/application_helper_spec.rb
@@ -67,10 +67,9 @@ describe ApplicationHelper do @@ -67,10 +67,9 @@ describe ApplicationHelper do
67 end 67 end
68 68
69 it "should call gravatar_icon when no avatar is present" do 69 it "should call gravatar_icon when no avatar is present" do
70 - user = create(:user) 70 + user = create(:user, email: 'test@example.com')
71 user.save! 71 user.save!
72 - allow(self).to receive(:gravatar_icon).and_return('gravatar_method_called')  
73 - avatar_icon(user.email).to_s.should == "gravatar_method_called" 72 + avatar_icon(user.email).to_s.should == "http://www.gravatar.com/avatar/55502f40dc8b7c769880b10874abc9d0?s=40&d=mm"
74 end 73 end
75 end 74 end
76 75
@@ -87,12 +86,12 @@ describe ApplicationHelper do @@ -87,12 +86,12 @@ describe ApplicationHelper do
87 end 86 end
88 87
89 it "should return default gravatar url" do 88 it "should return default gravatar url" do
90 - allow(self).to receive(:request).and_return(double(:ssl? => false)) 89 + Gitlab.config.gitlab.stub(https: false)
91 gravatar_icon(user_email).should match('http://www.gravatar.com/avatar/b58c6f14d292556214bd64909bcdb118') 90 gravatar_icon(user_email).should match('http://www.gravatar.com/avatar/b58c6f14d292556214bd64909bcdb118')
92 end 91 end
93 92
94 it "should use SSL when appropriate" do 93 it "should use SSL when appropriate" do
95 - allow(self).to receive(:request).and_return(double(:ssl? => true)) 94 + Gitlab.config.gitlab.stub(https: true)
96 gravatar_icon(user_email).should match('https://secure.gravatar.com') 95 gravatar_icon(user_email).should match('https://secure.gravatar.com')
97 end 96 end
98 97
spec/requests/api/notes_spec.rb
@@ -93,7 +93,7 @@ describe API::API, api: true do @@ -93,7 +93,7 @@ describe API::API, api: true do
93 post api("/projects/#{project.id}/issues/#{issue.id}/notes", user), body: 'hi!' 93 post api("/projects/#{project.id}/issues/#{issue.id}/notes", user), body: 'hi!'
94 response.status.should == 201 94 response.status.should == 201
95 json_response['body'].should == 'hi!' 95 json_response['body'].should == 'hi!'
96 - json_response['author']['email'].should == user.email 96 + json_response['author']['username'].should == user.username
97 end 97 end
98 98
99 it "should return a 400 bad request error if body not given" do 99 it "should return a 400 bad request error if body not given" do
@@ -112,7 +112,7 @@ describe API::API, api: true do @@ -112,7 +112,7 @@ describe API::API, api: true do
112 post api("/projects/#{project.id}/snippets/#{snippet.id}/notes", user), body: 'hi!' 112 post api("/projects/#{project.id}/snippets/#{snippet.id}/notes", user), body: 'hi!'
113 response.status.should == 201 113 response.status.should == 201
114 json_response['body'].should == 'hi!' 114 json_response['body'].should == 'hi!'
115 - json_response['author']['email'].should == user.email 115 + json_response['author']['username'].should == user.username
116 end 116 end
117 117
118 it "should return a 400 bad request error if body not given" do 118 it "should return a 400 bad request error if body not given" do
spec/requests/api/project_members_spec.rb
@@ -21,7 +21,7 @@ describe API::API, api: true do @@ -21,7 +21,7 @@ describe API::API, api: true do
21 response.status.should == 200 21 response.status.should == 200
22 json_response.should be_an Array 22 json_response.should be_an Array
23 json_response.count.should == 2 23 json_response.count.should == 2
24 - json_response.map { |u| u['email'] }.should include user.email 24 + json_response.map { |u| u['username'] }.should include user.username
25 end 25 end
26 26
27 it "finds team members with query string" do 27 it "finds team members with query string" do
@@ -29,7 +29,7 @@ describe API::API, api: true do @@ -29,7 +29,7 @@ describe API::API, api: true do
29 response.status.should == 200 29 response.status.should == 200
30 json_response.should be_an Array 30 json_response.should be_an Array
31 json_response.count.should == 1 31 json_response.count.should == 1
32 - json_response.first['email'].should == user.email 32 + json_response.first['username'].should == user.username
33 end 33 end
34 34
35 it "should return a 404 error if id not found" do 35 it "should return a 404 error if id not found" do
@@ -44,7 +44,7 @@ describe API::API, api: true do @@ -44,7 +44,7 @@ describe API::API, api: true do
44 it "should return project team member" do 44 it "should return project team member" do
45 get api("/projects/#{project.id}/members/#{user.id}", user) 45 get api("/projects/#{project.id}/members/#{user.id}", user)
46 response.status.should == 200 46 response.status.should == 200
47 - json_response['email'].should == user.email 47 + json_response['username'].should == user.username
48 json_response['access_level'].should == UsersProject::MASTER 48 json_response['access_level'].should == UsersProject::MASTER
49 end 49 end
50 50
@@ -62,7 +62,7 @@ describe API::API, api: true do @@ -62,7 +62,7 @@ describe API::API, api: true do
62 }.to change { UsersProject.count }.by(1) 62 }.to change { UsersProject.count }.by(1)
63 63
64 response.status.should == 201 64 response.status.should == 201
65 - json_response['email'].should == user2.email 65 + json_response['username'].should == user2.username
66 json_response['access_level'].should == UsersProject::DEVELOPER 66 json_response['access_level'].should == UsersProject::DEVELOPER
67 end 67 end
68 68
@@ -75,7 +75,7 @@ describe API::API, api: true do @@ -75,7 +75,7 @@ describe API::API, api: true do
75 }.not_to change { UsersProject.count }.by(1) 75 }.not_to change { UsersProject.count }.by(1)
76 76
77 response.status.should == 201 77 response.status.should == 201
78 - json_response['email'].should == user2.email 78 + json_response['username'].should == user2.username
79 json_response['access_level'].should == UsersProject::DEVELOPER 79 json_response['access_level'].should == UsersProject::DEVELOPER
80 end 80 end
81 81
@@ -101,7 +101,7 @@ describe API::API, api: true do @@ -101,7 +101,7 @@ describe API::API, api: true do
101 it "should update project team member" do 101 it "should update project team member" do
102 put api("/projects/#{project.id}/members/#{user3.id}", user), access_level: UsersProject::MASTER 102 put api("/projects/#{project.id}/members/#{user3.id}", user), access_level: UsersProject::MASTER
103 response.status.should == 200 103 response.status.should == 200
104 - json_response['email'].should == user3.email 104 + json_response['username'].should == user3.username
105 json_response['access_level'].should == UsersProject::MASTER 105 json_response['access_level'].should == UsersProject::MASTER
106 end 106 end
107 107
spec/requests/api/projects_spec.rb
@@ -37,7 +37,7 @@ describe API::API, api: true do @@ -37,7 +37,7 @@ describe API::API, api: true do
37 response.status.should == 200 37 response.status.should == 200
38 json_response.should be_an Array 38 json_response.should be_an Array
39 json_response.first['name'].should == project.name 39 json_response.first['name'].should == project.name
40 - json_response.first['owner']['email'].should == user.email 40 + json_response.first['owner']['username'].should == user.username
41 end 41 end
42 end 42 end
43 end 43 end
@@ -65,7 +65,7 @@ describe API::API, api: true do @@ -65,7 +65,7 @@ describe API::API, api: true do
65 response.status.should == 200 65 response.status.should == 200
66 json_response.should be_an Array 66 json_response.should be_an Array
67 json_response.first['name'].should == project.name 67 json_response.first['name'].should == project.name
68 - json_response.first['owner']['email'].should == user.email 68 + json_response.first['owner']['username'].should == user.username
69 end 69 end
70 end 70 end
71 end 71 end
@@ -270,7 +270,7 @@ describe API::API, api: true do @@ -270,7 +270,7 @@ describe API::API, api: true do
270 get api("/projects/#{project.id}", user) 270 get api("/projects/#{project.id}", user)
271 response.status.should == 200 271 response.status.should == 200
272 json_response['name'].should == project.name 272 json_response['name'].should == project.name
273 - json_response['owner']['email'].should == user.email 273 + json_response['owner']['username'].should == user.username
274 end 274 end
275 275
276 it "should return a project by path name" do 276 it "should return a project by path name" do
spec/requests/api/users_spec.rb
@@ -20,7 +20,18 @@ describe API::API, api: true do @@ -20,7 +20,18 @@ describe API::API, api: true do
20 get api("/users", user) 20 get api("/users", user)
21 response.status.should == 200 21 response.status.should == 200
22 json_response.should be_an Array 22 json_response.should be_an Array
23 - json_response.first['email'].should == user.email 23 + json_response.first['username'].should == user.username
  24 + end
  25 + end
  26 +
  27 + context "when admin" do
  28 + it "should return an array of users" do
  29 + get api("/users", admin)
  30 + response.status.should == 200
  31 + json_response.should be_an Array
  32 + json_response.first.keys.should include 'email'
  33 + json_response.first.keys.should include 'extern_uid'
  34 + json_response.first.keys.should include 'can_create_project'
24 end 35 end
25 end 36 end
26 end 37 end
@@ -29,7 +40,7 @@ describe API::API, api: true do @@ -29,7 +40,7 @@ describe API::API, api: true do
29 it "should return a user by id" do 40 it "should return a user by id" do
30 get api("/users/#{user.id}", user) 41 get api("/users/#{user.id}", user)
31 response.status.should == 200 42 response.status.should == 200
32 - json_response['email'].should == user.email 43 + json_response['username'].should == user.username
33 end 44 end
34 45
35 it "should return a 401 if unauthenticated" do 46 it "should return a 401 if unauthenticated" do