From e9eae3eb0dd25e4a34c9a4b6bcc7de312dde4489 Mon Sep 17 00:00:00 2001 From: Markus Koller Date: Thu, 28 Sep 2017 16:49:42 +0000 Subject: [PATCH] Support custom attributes on users --- .../concerns/custom_attributes_filter.rb | 20 ++++ app/finders/users_finder.rb | 2 + app/models/user.rb | 2 + app/models/user_custom_attribute.rb | 6 + app/policies/global_policy.rb | 5 + .../unreleased/feature-custom-attributes.yml | 4 + ...720122741_create_user_custom_attributes.rb | 17 +++ db/schema.rb | 12 ++ doc/api/README.md | 1 + doc/api/custom_attributes.md | 105 ++++++++++++++++++ doc/api/users.md | 6 + lib/api/custom_attributes_endpoints.rb | 77 +++++++++++++ lib/api/entities.rb | 5 + lib/api/users.rb | 2 + spec/factories/user_custom_attributes.rb | 7 ++ spec/finders/users_finder_spec.rb | 22 ++++ spec/models/ci/pipeline_variable_spec.rb | 2 +- spec/models/repository_spec.rb | 2 +- spec/models/user_custom_attribute_spec.rb | 16 +++ spec/models/user_spec.rb | 1 + spec/policies/global_policy_spec.rb | 14 +++ spec/requests/api/users_spec.rb | 4 + .../api/custom_attributes_shared_examples.rb | 103 +++++++++++++++++ 23 files changed, 433 insertions(+), 2 deletions(-) create mode 100644 app/finders/concerns/custom_attributes_filter.rb create mode 100644 app/models/user_custom_attribute.rb create mode 100644 changelogs/unreleased/feature-custom-attributes.yml create mode 100644 db/migrate/20170720122741_create_user_custom_attributes.rb create mode 100644 doc/api/custom_attributes.md create mode 100644 lib/api/custom_attributes_endpoints.rb create mode 100644 spec/factories/user_custom_attributes.rb create mode 100644 spec/models/user_custom_attribute_spec.rb create mode 100644 spec/support/shared_examples/requests/api/custom_attributes_shared_examples.rb diff --git a/app/finders/concerns/custom_attributes_filter.rb b/app/finders/concerns/custom_attributes_filter.rb new file mode 100644 index 00000000000..5bbf9ca242d --- /dev/null +++ b/app/finders/concerns/custom_attributes_filter.rb @@ -0,0 +1,20 @@ +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 diff --git a/app/finders/users_finder.rb b/app/finders/users_finder.rb index 33f7ae90598..1a7e97004fb 100644 --- a/app/finders/users_finder.rb +++ b/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 diff --git a/app/models/user.rb b/app/models/user.rb index 103ac78783f..4d523aa983f 100644 --- a/app/models/user.rb +++ b/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 # diff --git a/app/models/user_custom_attribute.rb b/app/models/user_custom_attribute.rb new file mode 100644 index 00000000000..eff25b31f9b --- /dev/null +++ b/app/models/user_custom_attribute.rb @@ -0,0 +1,6 @@ +class UserCustomAttribute < ActiveRecord::Base + belongs_to :user + + validates :user_id, :key, :value, presence: true + validates :key, uniqueness: { scope: [:user_id] } +end diff --git a/app/policies/global_policy.rb b/app/policies/global_policy.rb index 1be7bbe9953..8f7c01bb71f 100644 --- a/app/policies/global_policy.rb +++ b/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 diff --git a/changelogs/unreleased/feature-custom-attributes.yml b/changelogs/unreleased/feature-custom-attributes.yml new file mode 100644 index 00000000000..98736bc8d72 --- /dev/null +++ b/changelogs/unreleased/feature-custom-attributes.yml @@ -0,0 +1,4 @@ +--- +title: Support custom attributes on users +merge_request: 13038 +author: Markus Koller diff --git a/db/migrate/20170720122741_create_user_custom_attributes.rb b/db/migrate/20170720122741_create_user_custom_attributes.rb new file mode 100644 index 00000000000..b1c0bebc633 --- /dev/null +++ b/db/migrate/20170720122741_create_user_custom_attributes.rb @@ -0,0 +1,17 @@ +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 diff --git a/db/schema.rb b/db/schema.rb index 330336e8e61..63c8b8b3f49 100644 --- a/db/schema.rb +++ b/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 diff --git a/doc/api/README.md b/doc/api/README.md index 6cbea29bda6..3fd4c97e536 100644 --- a/doc/api/README.md +++ b/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) diff --git a/doc/api/custom_attributes.md b/doc/api/custom_attributes.md new file mode 100644 index 00000000000..8b26f7093ab --- /dev/null +++ b/doc/api/custom_attributes.md @@ -0,0 +1,105 @@ +# 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 +``` diff --git a/doc/api/users.md b/doc/api/users.md index 6d5db16b36a..1643c584244 100644 --- a/doc/api/users.md +++ b/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. diff --git a/lib/api/custom_attributes_endpoints.rb b/lib/api/custom_attributes_endpoints.rb new file mode 100644 index 00000000000..5000aa0d9ac --- /dev/null +++ b/lib/api/custom_attributes_endpoints.rb @@ -0,0 +1,77 @@ +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 diff --git a/lib/api/entities.rb b/lib/api/entities.rb index 7f4736a08cb..5d45b14f592 100644 --- a/lib/api/entities.rb +++ b/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 diff --git a/lib/api/users.rb b/lib/api/users.rb index 99024d1f0ad..d07dc302717 100644 --- a/lib/api/users.rb +++ b/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 diff --git a/spec/factories/user_custom_attributes.rb b/spec/factories/user_custom_attributes.rb new file mode 100644 index 00000000000..278cf290d4f --- /dev/null +++ b/spec/factories/user_custom_attributes.rb @@ -0,0 +1,7 @@ +FactoryGirl.define do + factory :user_custom_attribute do + user + sequence(:key) { |n| "key#{n}" } + sequence(:value) { |n| "value#{n}" } + end +end diff --git a/spec/finders/users_finder_spec.rb b/spec/finders/users_finder_spec.rb index 1bab6d64388..4249c52c481 100644 --- a/spec/finders/users_finder_spec.rb +++ b/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 diff --git a/spec/models/ci/pipeline_variable_spec.rb b/spec/models/ci/pipeline_variable_spec.rb index 2ce78e34b0c..889c243c8d8 100644 --- a/spec/models/ci/pipeline_variable_spec.rb +++ b/spec/models/ci/pipeline_variable_spec.rb @@ -1,6 +1,6 @@ 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) } diff --git a/spec/models/repository_spec.rb b/spec/models/repository_spec.rb index 76bb658b10d..07d9b66060e 100644 --- a/spec/models/repository_spec.rb +++ b/spec/models/repository_spec.rb @@ -1,6 +1,6 @@ require 'spec_helper' -describe Repository, models: true do +describe Repository do include RepoHelpers TestBlob = Struct.new(:path) diff --git a/spec/models/user_custom_attribute_spec.rb b/spec/models/user_custom_attribute_spec.rb new file mode 100644 index 00000000000..37fc3cb64f0 --- /dev/null +++ b/spec/models/user_custom_attribute_spec.rb @@ -0,0 +1,16 @@ +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 diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index c1affa812aa..62890dd5002 100644 --- a/spec/models/user_spec.rb +++ b/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) } diff --git a/spec/policies/global_policy_spec.rb b/spec/policies/global_policy_spec.rb index a6bf70c1e09..983f0e52d31 100644 --- a/spec/policies/global_policy_spec.rb +++ b/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 diff --git a/spec/requests/api/users_spec.rb b/spec/requests/api/users_spec.rb index c30188b1b41..69c8aa4482a 100644 --- a/spec/requests/api/users_spec.rb +++ b/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 diff --git a/spec/support/shared_examples/requests/api/custom_attributes_shared_examples.rb b/spec/support/shared_examples/requests/api/custom_attributes_shared_examples.rb new file mode 100644 index 00000000000..c9302f7b750 --- /dev/null +++ b/spec/support/shared_examples/requests/api/custom_attributes_shared_examples.rb @@ -0,0 +1,103 @@ +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