Merge branch 'feature/custom-attributes' into 'master'

Support custom attributes on users

See merge request gitlab-org/gitlab-ce!13038

app/finders/concerns/custom_attributes_filter.rb

+module CustomAttributesFilter
+  def by_custom_attributes(items)
+    return items unless params[:custom_attributes].is_a?(Hash)
+    return items unless Ability.allowed?(current_user, :read_custom_attribute)
+
+    association = items.reflect_on_association(:custom_attributes)
+    attributes_table = association.klass.arel_table
+    attributable_table = items.model.arel_table
+
+    custom_attributes = association.klass.select('true').where(
+      attributes_table[association.foreign_key]
+        .eq(attributable_table[association.association_primary_key])
+    )
+
+    # perform a subquery for each attribute to be filtered
+    params[:custom_attributes].inject(items) do |scope, (key, value)|
+      scope.where('EXISTS (?)', custom_attributes.where(key: key, value: value))
+    end
+  end
+end

app/finders/users_finder.rb

@@ -15,6 +15,7 @@
 #
 class UsersFinder
   include CreatedAtFilter
+  include CustomAttributesFilter
 
   attr_accessor :current_user, :params
 
@@ -32,6 +33,7 @@ class UsersFinder
     users = by_external_identity(users)
     users = by_external(users)
     users = by_created_at(users)
+    users = by_custom_attributes(users)
 
     users
   end

app/models/user.rb

@@ -130,6 +130,8 @@ class User < ActiveRecord::Base
   has_many :assigned_issues, class_name: "Issue", through: :issue_assignees, source: :issue
   has_many :assigned_merge_requests,  dependent: :nullify, foreign_key: :assignee_id, class_name: "MergeRequest" # rubocop:disable Cop/ActiveRecordDependent
 
+  has_many :custom_attributes, class_name: 'UserCustomAttribute'
+
   #
   # Validations
   #

app/models/user_custom_attribute.rb

+class UserCustomAttribute < ActiveRecord::Base
+  belongs_to :user
+
+  validates :user_id, :key, :value, presence: true
+  validates :key, uniqueness: { scope: [:user_id] }
+end

app/policies/global_policy.rb

@@ -47,4 +47,9 @@ class GlobalPolicy < BasePolicy
   rule { ~(anonymous & restricted_public_level) }.policy do
     enable :read_users_list
   end
+
+  rule { admin }.policy do
+    enable :read_custom_attribute
+    enable :update_custom_attribute
+  end
 end

changelogs/unreleased/feature-custom-attributes.yml

+---
+title: Support custom attributes on users
+merge_request: 13038
+author: Markus Koller

db/migrate/20170720122741_create_user_custom_attributes.rb

+class CreateUserCustomAttributes < ActiveRecord::Migration
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  def change
+    create_table :user_custom_attributes do |t|
+      t.timestamps_with_timezone null: false
+      t.references :user, null: false, foreign_key: { on_delete: :cascade }
+      t.string :key, null: false
+      t.string :value, null: false
+
+      t.index [:user_id, :key], unique: true
+      t.index [:key, :value]
+    end
+  end
+end

db/schema.rb

@@ -1534,6 +1534,17 @@ ActiveRecord::Schema.define(version: 20170921115009) do
 
   add_index "user_agent_details", ["subject_id", "subject_type"], name: "index_user_agent_details_on_subject_id_and_subject_type", using: :btree
 
+  create_table "user_custom_attributes", force: :cascade do |t|
+    t.datetime "created_at", null: false
+    t.datetime "updated_at", null: false
+    t.integer "user_id", null: false
+    t.string "key", null: false
+    t.string "value", null: false
+  end
+
+  add_index "user_custom_attributes", ["key", "value"], name: "index_user_custom_attributes_on_key_and_value", using: :btree
+  add_index "user_custom_attributes", ["user_id", "key"], name: "index_user_custom_attributes_on_user_id_and_key", unique: true, using: :btree
+
   create_table "user_synced_attributes_metadata", force: :cascade do |t|
     t.boolean "name_synced", default: false
     t.boolean "email_synced", default: false
@@ -1760,6 +1771,7 @@ ActiveRecord::Schema.define(version: 20170921115009) do
   add_foreign_key "todos", "projects", name: "fk_45054f9c45", on_delete: :cascade
   add_foreign_key "trending_projects", "projects", on_delete: :cascade
   add_foreign_key "u2f_registrations", "users"
+  add_foreign_key "user_custom_attributes", "users", on_delete: :cascade
   add_foreign_key "user_synced_attributes_metadata", "users", on_delete: :cascade
   add_foreign_key "users_star_projects", "projects", name: "fk_22cd27ddfc", on_delete: :cascade
   add_foreign_key "web_hook_logs", "web_hooks", on_delete: :cascade

doc/api/README.md

@@ -14,6 +14,7 @@ following locations:
 - [Project-level Variables](project_level_variables.md)
 - [Group-level Variables](group_level_variables.md)
 - [Commits](commits.md)
+- [Custom Attributes](custom_attributes.md)
 - [Deployments](deployments.md)
 - [Deploy Keys](deploy_keys.md)
 - [Environments](environments.md)

doc/api/custom_attributes.md

+# Custom Attributes API
+
+Every API call to custom attributes must be authenticated as administrator.
+
+## List custom attributes
+
+Get all custom attributes on a user.
+
+```
+GET /users/:id/custom_attributes
+```
+
+| Attribute | Type | Required | Description |
+| --------- | ---- | -------- | ----------- |
+| `id` | integer | yes | The ID of a user |
+
+```bash
+curl --header "PRIVATE-TOKEN: 9koXpg98eAheJpvBs5tK" https://gitlab.example.com/api/v4/users/42/custom_attributes
+```
+
+Example response:
+
+```json
+[
+   {
+      "key": "location",
+      "value": "Antarctica"
+   },
+   {
+      "key": "role",
+      "value": "Developer"
+   }
+]
+```
+
+## Single custom attribute
+
+Get a single custom attribute on a user.
+
+```
+GET /users/:id/custom_attributes/:key
+```
+
+| Attribute | Type | Required | Description |
+| --------- | ---- | -------- | ----------- |
+| `id` | integer | yes | The ID of a user |
+| `key` | string | yes | The key of the custom attribute |
+
+```bash
+curl --header "PRIVATE-TOKEN: 9koXpg98eAheJpvBs5tK" https://gitlab.example.com/api/v4/users/42/custom_attributes/location
+```
+
+Example response:
+
+```json
+{
+   "key": "location",
+   "value": "Antarctica"
+}
+```
+
+## Set custom attribute
+
+Set a custom attribute on a user. The attribute will be updated if it already exists,
+or newly created otherwise.
+
+```
+PUT /users/:id/custom_attributes/:key
+```
+
+| Attribute | Type | Required | Description |
+| --------- | ---- | -------- | ----------- |
+| `id` | integer | yes | The ID of a user |
+| `key` | string | yes | The key of the custom attribute |
+| `value` | string | yes | The value of the custom attribute |
+
+```bash
+curl --request PUT --header "PRIVATE-TOKEN: 9koXpg98eAheJpvBs5tK" --data "value=Greenland" https://gitlab.example.com/api/v4/users/42/custom_attributes/location
+```
+
+Example response:
+
+```json
+{
+   "key": "location",
+   "value": "Greenland"
+}
+```
+
+## Delete custom attribute
+
+Delete a custom attribute on a user.
+
+```
+DELETE /users/:id/custom_attributes/:key
+```
+
+| Attribute | Type | Required | Description |
+| --------- | ---- | -------- | ----------- |
+| `id` | integer | yes | The ID of a user |
+| `key` | string | yes | The key of the custom attribute |
+
+```bash
+curl --request DELETE --header "PRIVATE-TOKEN: 9koXpg98eAheJpvBs5tK" https://gitlab.example.com/api/v4/users/42/custom_attributes/location
+```

doc/api/users.md

@@ -154,6 +154,12 @@ You can search users by creation date time range with:
 GET /users?created_before=2001-01-02T00:00:00.060Z&created_after=1999-01-02T00:00:00.060
 ```
 
+You can filter by [custom attributes](custom_attributes.md) with:
+
+```
+GET /users?custom_attributes[key]=value&custom_attributes[other_key]=other_value
+```
+
 ## Single user
 
 Get a single user.

lib/api/custom_attributes_endpoints.rb

+module API
+  module CustomAttributesEndpoints
+    extend ActiveSupport::Concern
+
+    included do
+      attributable_class = name.demodulize.singularize
+      attributable_key = attributable_class.underscore
+      attributable_name = attributable_class.humanize(capitalize: false)
+      attributable_finder = "find_#{attributable_key}"
+
+      helpers do
+        params :custom_attributes_key do
+          requires :key, type: String, desc: 'The key of the custom attribute'
+        end
+      end
+
+      desc "Get all custom attributes on a #{attributable_name}" do
+        success Entities::CustomAttribute
+      end
+      get ':id/custom_attributes' do
+        resource = public_send(attributable_finder, params[:id]) # rubocop:disable GitlabSecurity/PublicSend
+        authorize! :read_custom_attribute
+
+        present resource.custom_attributes, with: Entities::CustomAttribute
+      end
+
+      desc "Get a custom attribute on a #{attributable_name}" do
+        success Entities::CustomAttribute
+      end
+      params do
+        use :custom_attributes_key
+      end
+      get ':id/custom_attributes/:key' do
+        resource = public_send(attributable_finder, params[:id]) # rubocop:disable GitlabSecurity/PublicSend
+        authorize! :read_custom_attribute
+
+        custom_attribute = resource.custom_attributes.find_by!(key: params[:key])
+
+        present custom_attribute, with: Entities::CustomAttribute
+      end
+
+      desc "Set a custom attribute on a #{attributable_name}"
+      params do
+        use :custom_attributes_key
+        requires :value, type: String, desc: 'The value of the custom attribute'
+      end
+      put ':id/custom_attributes/:key' do
+        resource = public_send(attributable_finder, params[:id]) # rubocop:disable GitlabSecurity/PublicSend
+        authorize! :update_custom_attribute
+
+        custom_attribute = resource.custom_attributes
+          .find_or_initialize_by(key: params[:key])
+
+        custom_attribute.update(value: params[:value])
+
+        if custom_attribute.valid?
+          present custom_attribute, with: Entities::CustomAttribute
+        else
+          render_validation_error!(custom_attribute)
+        end
+      end
+
+      desc "Delete a custom attribute on a #{attributable_name}"
+      params do
+        use :custom_attributes_key
+      end
+      delete ':id/custom_attributes/:key' do
+        resource = public_send(attributable_finder, params[:id]) # rubocop:disable GitlabSecurity/PublicSend
+        authorize! :update_custom_attribute
+
+        resource.custom_attributes.find_by!(key: params[:key]).destroy
+
+        status 204
+      end
+    end
+  end
+end

lib/api/entities.rb

@@ -1036,5 +1036,10 @@ module API
       expose :failing_on_hosts
       expose :total_failures
     end
+
+    class CustomAttribute < Grape::Entity
+      expose :key
+      expose :value
+    end
   end
 end

lib/api/users.rb

@@ -6,6 +6,8 @@ module API
     allow_access_with_scope :read_user, if: -> (request) { request.get? }
 
     resource :users, requirements: { uid: /[0-9]*/, id: /[0-9]*/ } do
+      include CustomAttributesEndpoints
+
       before do
         authenticate_non_get!
       end

spec/factories/user_custom_attributes.rb

+FactoryGirl.define do
+  factory :user_custom_attribute do
+    user
+    sequence(:key) { |n| "key#{n}" }
+    sequence(:value) { |n| "value#{n}" }
+  end
+end

spec/finders/users_finder_spec.rb

@@ -56,6 +56,15 @@ describe UsersFinder do
 
         expect(users.map(&:username)).not_to include([filtered_user_before.username, filtered_user_after.username])
       end
+
+      it 'does not filter by custom attributes' do
+        users = described_class.new(
+          user,
+          custom_attributes: { foo: 'bar' }
+        ).execute
+
+        expect(users).to contain_exactly(user, user1, user2, omniauth_user)
+      end
     end
 
     context 'with an admin user' do
@@ -72,6 +81,19 @@ describe UsersFinder do
 
         expect(users).to contain_exactly(admin, user1, user2, external_user, omniauth_user)
       end
+
+      it 'filters by custom attributes' do
+        create :user_custom_attribute, user: user1, key: 'foo', value: 'foo'
+        create :user_custom_attribute, user: user1, key: 'bar', value: 'bar'
+        create :user_custom_attribute, user: user2, key: 'foo', value: 'foo'
+
+        users = described_class.new(
+          admin,
+          custom_attributes: { foo: 'foo', bar: 'bar' }
+        ).execute
+
+        expect(users).to contain_exactly(user1)
+      end
     end
   end
 end

spec/models/ci/pipeline_variable_spec.rb

 require 'spec_helper'
 
-describe Ci::PipelineVariable, models: true do
+describe Ci::PipelineVariable do
   subject { build(:ci_pipeline_variable) }
 
   it { is_expected.to include_module(HasVariable) }

spec/models/repository_spec.rb

 require 'spec_helper'
 
-describe Repository, models: true do
+describe Repository do
   include RepoHelpers
   TestBlob = Struct.new(:path)
 

spec/models/user_custom_attribute_spec.rb

+require 'spec_helper'
+
+describe UserCustomAttribute do
+  describe 'assocations' do
+    it { is_expected.to belong_to(:user) }
+  end
+
+  describe 'validations' do
+    subject { build :user_custom_attribute }
+
+    it { is_expected.to validate_presence_of(:user_id) }
+    it { is_expected.to validate_presence_of(:key) }
+    it { is_expected.to validate_presence_of(:value) }
+    it { is_expected.to validate_uniqueness_of(:key).scoped_to(:user_id) }
+  end
+end

spec/models/user_spec.rb

@@ -39,6 +39,7 @@ describe User do
     it { is_expected.to have_many(:chat_names).dependent(:destroy) }
     it { is_expected.to have_many(:uploads).dependent(:destroy) }
     it { is_expected.to have_many(:reported_abuse_reports).dependent(:destroy).class_name('AbuseReport') }
+    it { is_expected.to have_many(:custom_attributes).class_name('UserCustomAttribute') }
 
     describe "#abuse_report" do
       let(:current_user) { create(:user) }

spec/policies/global_policy_spec.rb

@@ -51,4 +51,18 @@ describe GlobalPolicy do
       end
     end
   end
+
+  describe 'custom attributes' do
+    context 'regular user' do
+      it { is_expected.not_to be_allowed(:read_custom_attribute) }
+      it { is_expected.not_to be_allowed(:update_custom_attribute) }
+    end
+
+    context 'admin' do
+      let(:current_user) { create(:user, :admin) }
+
+      it { is_expected.to be_allowed(:read_custom_attribute) }
+      it { is_expected.to be_allowed(:update_custom_attribute) }
+    end
+  end
 end

spec/requests/api/users_spec.rb

@@ -1905,4 +1905,8 @@ describe API::Users do
       expect(impersonation_token.reload.revoked).to be_truthy
     end
   end
+
+  include_examples 'custom attributes endpoints', 'users' do
+    let(:attributable) { user }
+  end
 end

spec/support/shared_examples/requests/api/custom_attributes_shared_examples.rb

+shared_examples 'custom attributes endpoints' do |attributable_name|
+  let!(:custom_attribute1) { attributable.custom_attributes.create key: 'foo', value: 'foo' }
+  let!(:custom_attribute2) { attributable.custom_attributes.create key: 'bar', value: 'bar' }
+
+  describe "GET /#{attributable_name} with custom attributes filter" do
+    let!(:other_attributable) { create attributable.class.name.underscore }
+
+    context 'with an unauthorized user' do
+      it 'does not filter by custom attributes' do
+        get api("/#{attributable_name}", user), custom_attributes: { foo: 'foo', bar: 'bar' }
+
+        expect(response).to have_http_status(200)
+        expect(json_response.size).to be 2
+      end
+    end
+
+    it 'filters by custom attributes' do
+      get api("/#{attributable_name}", admin), custom_attributes: { foo: 'foo', bar: 'bar' }
+
+      expect(response).to have_http_status(200)
+      expect(json_response.size).to be 1
+      expect(json_response.first['id']).to eq attributable.id
+    end
+  end
+
+  describe "GET /#{attributable_name}/:id/custom_attributes" do
+    context 'with an unauthorized user' do
+      subject { get api("/#{attributable_name}/#{attributable.id}/custom_attributes", user) }
+
+      it_behaves_like 'an unauthorized API user'
+    end
+
+    it 'returns all custom attributes' do
+      get api("/#{attributable_name}/#{attributable.id}/custom_attributes", admin)
+
+      expect(response).to have_http_status(200)
+      expect(json_response).to contain_exactly(
+        { 'key' => 'foo', 'value' => 'foo' },
+        { 'key' => 'bar', 'value' => 'bar' }
+      )
+    end
+  end
+
+  describe "GET /#{attributable_name}/:id/custom_attributes/:key" do
+    context 'with an unauthorized user' do
+      subject { get api("/#{attributable_name}/#{attributable.id}/custom_attributes/foo", user) }
+
+      it_behaves_like 'an unauthorized API user'
+    end
+
+    it 'returns a single custom attribute' do
+      get api("/#{attributable_name}/#{attributable.id}/custom_attributes/foo", admin)
+
+      expect(response).to have_http_status(200)
+      expect(json_response).to eq({ 'key' => 'foo', 'value' => 'foo' })
+    end
+  end
+
+  describe "PUT /#{attributable_name}/:id/custom_attributes/:key" do
+    context 'with an unauthorized user' do
+      subject { put api("/#{attributable_name}/#{attributable.id}/custom_attributes/foo", user), value: 'new' }
+
+      it_behaves_like 'an unauthorized API user'
+    end
+
+    it 'creates a new custom attribute' do
+      expect do
+        put api("/#{attributable_name}/#{attributable.id}/custom_attributes/new", admin), value: 'new'
+      end.to change { attributable.custom_attributes.count }.by(1)
+
+      expect(response).to have_http_status(200)
+      expect(json_response).to eq({ 'key' => 'new', 'value' => 'new' })
+      expect(attributable.custom_attributes.find_by(key: 'new').value).to eq 'new'
+    end
+
+    it 'updates an existing custom attribute' do
+      expect do
+        put api("/#{attributable_name}/#{attributable.id}/custom_attributes/foo", admin), value: 'new'
+      end.not_to change { attributable.custom_attributes.count }
+
+      expect(response).to have_http_status(200)
+      expect(json_response).to eq({ 'key' => 'foo', 'value' => 'new' })
+      expect(custom_attribute1.reload.value).to eq 'new'
+    end
+  end
+
+  describe "DELETE /#{attributable_name}/:id/custom_attributes/:key" do
+    context 'with an unauthorized user' do
+      subject { delete api("/#{attributable_name}/#{attributable.id}/custom_attributes/foo", user) }
+
+      it_behaves_like 'an unauthorized API user'
+    end
+
+    it 'deletes an existing custom attribute' do
+      expect do
+        delete api("/#{attributable_name}/#{attributable.id}/custom_attributes/foo", admin)
+      end.to change { attributable.custom_attributes.count }.by(-1)
+
+      expect(response).to have_http_status(204)
+      expect(attributable.custom_attributes.find_by(key: 'foo')).to be_nil
+    end
+  end
+end