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

More secure api

Dont expose user email via API. Fixes #1314

17 files changed, 161 insertions, 64 deletions

diff --git a/CHANGELOG b/CHANGELOG
index 4a299827c11..c353c3b770c 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 382f9b37992..cfbcd5108c8 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 da66a4ba7f2..86318bd7d94 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 685d41a5520..603e89a5e29 100644
--- a/app/controllers/application_controller.rb
+++ b/app/controllers/application_controller.rb
@@ -164,9 +164,8 @@ class ApplicationController < ActionController::Base
   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 13120d2e581..c3d89eb1b82 100644
--- a/app/helpers/application_helper.rb
+++ b/app/helpers/application_helper.rb
@@ -60,23 +60,21 @@ module ApplicationHelper
 
   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 0fbc9284dd8..2352f8c050b 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -482,4 +482,12 @@ class User < ActiveRecord::Base
   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 00000000000..a69c7c78377
--- /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 94af37629ff..4ddbf739774 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 f15fe185ae0..b190646a1e3 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 06c66ba0b35..5850892df07 100644
--- a/lib/api/internal.rb
+++ b/lib/api/internal.rb
@@ -59,4 +59,3 @@ module API
     end
   end
 end
-
diff --git a/lib/api/projects.rb b/lib/api/projects.rb
index 9a7f22b536f..732c969d7ef 100644
--- a/lib/api/projects.rb
+++ b/lib/api/projects.rb
@@ -209,7 +209,7 @@ module API
         @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 6ed2740c333..92dbe97f0a4 100644
--- a/lib/api/users.rb
+++ b/lib/api/users.rb
@@ -13,7 +13,12 @@ module 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 @@ module 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 @@ module 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 @@ module 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 6401ec0710a..6a11414a023 100644
--- a/spec/helpers/application_helper_spec.rb
+++ b/spec/helpers/application_helper_spec.rb
@@ -67,10 +67,9 @@ describe ApplicationHelper do
     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 @@ describe ApplicationHelper do
     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 11796183474..2875db04ee4 100644
--- a/spec/requests/api/notes_spec.rb
+++ b/spec/requests/api/notes_spec.rb
@@ -93,7 +93,7 @@ describe API::API, api: true  do
         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 @@ describe API::API, api: true  do
         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 ec2d6e85096..032f850010c 100644
--- a/spec/requests/api/project_members_spec.rb
+++ b/spec/requests/api/project_members_spec.rb
@@ -21,7 +21,7 @@ describe API::API, api: true  do
       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 @@ describe API::API, api: true  do
       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 @@ describe API::API, api: true  do
     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 @@ describe API::API, api: true  do
       }.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 @@ describe API::API, api: true  do
       }.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 @@ describe API::API, api: true  do
     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 43915d8684b..415735091c3 100644
--- a/spec/requests/api/projects_spec.rb
+++ b/spec/requests/api/projects_spec.rb
@@ -37,7 +37,7 @@ describe API::API, api: true  do
         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 @@ describe API::API, api: true  do
         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 @@ describe API::API, api: true  do
       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 a6d300b099b..c3eec56d133 100644
--- a/spec/requests/api/users_spec.rb
+++ b/spec/requests/api/users_spec.rb
@@ -20,7 +20,18 @@ describe API::API, api: true  do
         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 @@ describe API::API, api: true  do
     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