From bed948321173b49564f39837e97212ee4dd96e03 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rub=C3=A9n=20D=C3=A1vila?= Date: Wed, 7 Feb 2018 08:00:53 -0500 Subject: Backport of LFS File Locking API --- app/controllers/concerns/lfs_request.rb | 6 +- app/controllers/projects/lfs_api_controller.rb | 2 +- .../projects/lfs_locks_api_controller.rb | 70 +++++++++ app/models/lfs_file_lock.rb | 12 ++ app/models/project.rb | 1 + app/models/repository.rb | 7 + app/serializers/lfs_file_lock_entity.rb | 11 ++ app/serializers/lfs_file_lock_serializer.rb | 3 + app/services/lfs/lock_file_service.rb | 39 +++++ app/services/lfs/locks_finder_service.rb | 17 +++ app/services/lfs/unlock_file_service.rb | 43 ++++++ .../35856-implement-file-locking-api.yml | 5 + config/initializers/mime_types.rb | 2 +- config/routes/git_http.rb | 7 + db/migrate/20180116193854_create_lfs_file_locks.rb | 30 ++++ db/schema.rb | 12 ++ .../lfs/manage_large_binaries_with_git_lfs.md | 66 +++++++++ lib/gitlab/checks/change_access.rb | 29 +++- lib/gitlab/checks/commit_check.rb | 61 ++++++++ lib/gitlab/import_export/import_export.yml | 2 + spec/factories/lfs_file_locks.rb | 7 + spec/lib/gitlab/checks/change_access_spec.rb | 39 +++++ spec/lib/gitlab/import_export/all_models.yml | 3 + .../gitlab/import_export/safe_model_attributes.yml | 6 + spec/models/lfs_file_lock_spec.rb | 57 ++++++++ spec/models/project_spec.rb | 1 + spec/models/repository_spec.rb | 22 +++ spec/requests/lfs_http_spec.rb | 2 +- spec/requests/lfs_locks_api_spec.rb | 159 +++++++++++++++++++++ spec/serializers/lfs_file_lock_entity_spec.rb | 19 +++ spec/services/lfs/lock_file_service_spec.rb | 62 ++++++++ spec/services/lfs/locks_finder_service_spec.rb | 101 +++++++++++++ spec/services/lfs/unlock_file_service_spec.rb | 105 ++++++++++++++ 33 files changed, 1002 insertions(+), 6 deletions(-) create mode 100644 app/controllers/projects/lfs_locks_api_controller.rb create mode 100644 app/models/lfs_file_lock.rb create mode 100644 app/serializers/lfs_file_lock_entity.rb create mode 100644 app/serializers/lfs_file_lock_serializer.rb create mode 100644 app/services/lfs/lock_file_service.rb create mode 100644 app/services/lfs/locks_finder_service.rb create mode 100644 app/services/lfs/unlock_file_service.rb create mode 100644 changelogs/unreleased/35856-implement-file-locking-api.yml create mode 100644 db/migrate/20180116193854_create_lfs_file_locks.rb create mode 100644 lib/gitlab/checks/commit_check.rb create mode 100644 spec/factories/lfs_file_locks.rb create mode 100644 spec/models/lfs_file_lock_spec.rb create mode 100644 spec/requests/lfs_locks_api_spec.rb create mode 100644 spec/serializers/lfs_file_lock_entity_spec.rb create mode 100644 spec/services/lfs/lock_file_service_spec.rb create mode 100644 spec/services/lfs/locks_finder_service_spec.rb create mode 100644 spec/services/lfs/unlock_file_service_spec.rb diff --git a/app/controllers/concerns/lfs_request.rb b/app/controllers/concerns/lfs_request.rb index 4311f9d4db9..5e4e8a87153 100644 --- a/app/controllers/concerns/lfs_request.rb +++ b/app/controllers/concerns/lfs_request.rb @@ -10,6 +10,8 @@ module LfsRequest extend ActiveSupport::Concern + CONTENT_TYPE = 'application/vnd.git-lfs+json'.freeze + included do before_action :require_lfs_enabled! before_action :lfs_check_access! @@ -50,7 +52,7 @@ module LfsRequest message: 'Access forbidden. Check your access level.', documentation_url: help_url }, - content_type: "application/vnd.git-lfs+json", + content_type: CONTENT_TYPE, status: 403 ) end @@ -61,7 +63,7 @@ module LfsRequest message: 'Not found.', documentation_url: help_url }, - content_type: "application/vnd.git-lfs+json", + content_type: CONTENT_TYPE, status: 404 ) end diff --git a/app/controllers/projects/lfs_api_controller.rb b/app/controllers/projects/lfs_api_controller.rb index 536f908d2c5..c77f10ef1dd 100644 --- a/app/controllers/projects/lfs_api_controller.rb +++ b/app/controllers/projects/lfs_api_controller.rb @@ -98,7 +98,7 @@ class Projects::LfsApiController < Projects::GitHttpClientController json: { message: lfs_read_only_message }, - content_type: 'application/vnd.git-lfs+json', + content_type: LfsRequest::CONTENT_TYPE, status: 403 ) end diff --git a/app/controllers/projects/lfs_locks_api_controller.rb b/app/controllers/projects/lfs_locks_api_controller.rb new file mode 100644 index 00000000000..3fff0fd69ae --- /dev/null +++ b/app/controllers/projects/lfs_locks_api_controller.rb @@ -0,0 +1,70 @@ +class Projects::LfsLocksApiController < Projects::GitHttpClientController + include LfsRequest + + def create + @result = Lfs::LockFileService.new(project, user, params).execute + + render_json(@result[:lock]) + end + + def unlock + @result = Lfs::UnlockFileService.new(project, user, params).execute + + render_json(@result[:lock]) + end + + def index + @result = Lfs::LocksFinderService.new(project, user, params).execute + + render_json(@result[:locks]) + end + + def verify + @result = Lfs::LocksFinderService.new(project, user, {}).execute + + ours, theirs = split_by_owner(@result[:locks]) + + render_json({ ours: ours, theirs: theirs }, false) + end + + private + + def render_json(data, process = true) + render json: build_payload(data, process), + content_type: LfsRequest::CONTENT_TYPE, + status: @result[:http_status] + end + + def build_payload(data, process) + data = LfsFileLockSerializer.new.represent(data) if process + + return data if @result[:status] == :success + + # When the locking failed due to an existent Lock, the existent record + # is returned in `@result[:lock]` + error_payload(@result[:message], @result[:lock] ? data : {}) + end + + def error_payload(message, custom_attrs = {}) + custom_attrs.merge({ + message: message, + documentation_url: help_url + }) + end + + def split_by_owner(locks) + groups = locks.partition { |lock| lock.user_id == user.id } + + groups.map! do |records| + LfsFileLockSerializer.new.represent(records, root: false) + end + end + + def download_request? + params[:action] == 'index' + end + + def upload_request? + %w(create unlock verify).include?(params[:action]) + end +end diff --git a/app/models/lfs_file_lock.rb b/app/models/lfs_file_lock.rb new file mode 100644 index 00000000000..50bb6ca382d --- /dev/null +++ b/app/models/lfs_file_lock.rb @@ -0,0 +1,12 @@ +class LfsFileLock < ActiveRecord::Base + belongs_to :project + belongs_to :user + + validates :project_id, :user_id, :path, presence: true + + def can_be_unlocked_by?(current_user, forced = false) + return true if current_user.id == user_id + + forced && current_user.can?(:admin_project, project) + end +end diff --git a/app/models/project.rb b/app/models/project.rb index fd8917467e9..0590cc1c720 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -179,6 +179,7 @@ class Project < ActiveRecord::Base has_many :releases has_many :lfs_objects_projects, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent has_many :lfs_objects, through: :lfs_objects_projects + has_many :lfs_file_locks has_many :project_group_links has_many :invited_groups, through: :project_group_links, source: :group has_many :pages_domains diff --git a/app/models/repository.rb b/app/models/repository.rb index 3d6f8f0c305..1cf55fd4332 100644 --- a/app/models/repository.rb +++ b/app/models/repository.rb @@ -164,6 +164,13 @@ class Repository commits end + # Returns a list of commits that are not present in any reference + def new_commits(newrev) + refs = ::Gitlab::Git::RevList.new(raw, newrev: newrev).new_refs + + refs.map { |sha| commit(sha.strip) } + end + # Gitaly migration: https://gitlab.com/gitlab-org/gitaly/issues/384 def find_commits_by_message(query, ref = nil, path = nil, limit = 1000, offset = 0) unless exists? && has_visible_content? && query.present? diff --git a/app/serializers/lfs_file_lock_entity.rb b/app/serializers/lfs_file_lock_entity.rb new file mode 100644 index 00000000000..264a77adc3f --- /dev/null +++ b/app/serializers/lfs_file_lock_entity.rb @@ -0,0 +1,11 @@ +class LfsFileLockEntity < Grape::Entity + root 'locks', 'lock' + + expose :path + expose(:id) { |entity| entity.id.to_s } + expose(:created_at, as: :locked_at) { |entity| entity.created_at.to_s(:iso8601) } + + expose :owner do + expose(:name) { |entity| entity.user&.name } + end +end diff --git a/app/serializers/lfs_file_lock_serializer.rb b/app/serializers/lfs_file_lock_serializer.rb new file mode 100644 index 00000000000..ba8fb1a461d --- /dev/null +++ b/app/serializers/lfs_file_lock_serializer.rb @@ -0,0 +1,3 @@ +class LfsFileLockSerializer < BaseSerializer + entity LfsFileLockEntity +end diff --git a/app/services/lfs/lock_file_service.rb b/app/services/lfs/lock_file_service.rb new file mode 100644 index 00000000000..bbe10f84ef4 --- /dev/null +++ b/app/services/lfs/lock_file_service.rb @@ -0,0 +1,39 @@ +module Lfs + class LockFileService < BaseService + def execute + unless can?(current_user, :push_code, project) + raise Gitlab::GitAccess::UnauthorizedError, 'You have no permissions' + end + + create_lock! + rescue ActiveRecord::RecordNotUnique + error('already locked', 409, current_lock) + rescue Gitlab::GitAccess::UnauthorizedError => ex + error(ex.message, 403) + rescue => ex + error(ex.message, 500) + end + + private + + def current_lock + project.lfs_file_locks.find_by(path: params[:path]) + end + + def create_lock! + lock = project.lfs_file_locks.create!(user: current_user, + path: params[:path]) + + success(http_status: 201, lock: lock) + end + + def error(message, http_status, lock = nil) + { + status: :error, + message: message, + http_status: http_status, + lock: lock + } + end + end +end diff --git a/app/services/lfs/locks_finder_service.rb b/app/services/lfs/locks_finder_service.rb new file mode 100644 index 00000000000..13c6cc6f81c --- /dev/null +++ b/app/services/lfs/locks_finder_service.rb @@ -0,0 +1,17 @@ +module Lfs + class LocksFinderService < BaseService + def execute + success(locks: find_locks) + rescue => ex + error(ex.message, 500) + end + + private + + def find_locks + options = params.slice(:id, :path).compact.symbolize_keys + + project.lfs_file_locks.where(options) + end + end +end diff --git a/app/services/lfs/unlock_file_service.rb b/app/services/lfs/unlock_file_service.rb new file mode 100644 index 00000000000..6c93dc69bb0 --- /dev/null +++ b/app/services/lfs/unlock_file_service.rb @@ -0,0 +1,43 @@ +module Lfs + class UnlockFileService < BaseService + def execute + unless can?(current_user, :push_code, project) + raise Gitlab::GitAccess::UnauthorizedError, 'You have no permissions' + end + + unlock_file + rescue Gitlab::GitAccess::UnauthorizedError => ex + error(ex.message, 403) + rescue ActiveRecord::RecordNotFound + error('Lock not found', 404) + rescue => ex + error(ex.message, 500) + end + + private + + def unlock_file + forced = params[:force] == true + + if lock.can_be_unlocked_by?(current_user, forced) + lock.destroy! + + success(lock: lock, http_status: :ok) + elsif forced + error('You must have master access to force delete a lock', 403) + else + error("#{lock.path} is locked by GitLab User #{lock.user_id}", 403) + end + end + + def lock + return @lock if defined?(@lock) + + @lock = if params[:id].present? + project.lfs_file_locks.find(params[:id]) + elsif params[:path].present? + project.lfs_file_locks.find_by!(path: params[:path]) + end + end + end +end diff --git a/changelogs/unreleased/35856-implement-file-locking-api.yml b/changelogs/unreleased/35856-implement-file-locking-api.yml new file mode 100644 index 00000000000..fa848ad9ed8 --- /dev/null +++ b/changelogs/unreleased/35856-implement-file-locking-api.yml @@ -0,0 +1,5 @@ +--- +title: Backport of LFS File Locking API +merge_request: 16935 +author: +type: added diff --git a/config/initializers/mime_types.rb b/config/initializers/mime_types.rb index 5e3e4c966cb..e9326653cbe 100644 --- a/config/initializers/mime_types.rb +++ b/config/initializers/mime_types.rb @@ -14,4 +14,4 @@ Mime::Type.register "video/webm", :webm Mime::Type.register "video/ogg", :ogv Mime::Type.unregister :json -Mime::Type.register 'application/json', :json, %w(application/vnd.git-lfs+json application/json) +Mime::Type.register 'application/json', :json, [LfsRequest::CONTENT_TYPE, 'application/json'] diff --git a/config/routes/git_http.rb b/config/routes/git_http.rb index a53c94326d4..ff51823897d 100644 --- a/config/routes/git_http.rb +++ b/config/routes/git_http.rb @@ -16,6 +16,13 @@ scope(path: '*namespace_id/:project_id', get '/*oid', action: :deprecated end + scope(path: 'info/lfs') do + resources :lfs_locks, controller: :lfs_locks_api, path: 'locks' do + post :unlock, on: :member + post :verify, on: :collection + end + end + # GitLab LFS object storage scope(path: 'gitlab-lfs/objects/*oid', controller: :lfs_storage, constraints: { oid: /[a-f0-9]{64}/ }) do get '/', action: :download diff --git a/db/migrate/20180116193854_create_lfs_file_locks.rb b/db/migrate/20180116193854_create_lfs_file_locks.rb new file mode 100644 index 00000000000..23b0c90484b --- /dev/null +++ b/db/migrate/20180116193854_create_lfs_file_locks.rb @@ -0,0 +1,30 @@ +class CreateLfsFileLocks < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + + DOWNTIME = false + + disable_ddl_transaction! + + def up + create_table :lfs_file_locks do |t| + t.references :project, null: false, foreign_key: { on_delete: :cascade } + t.references :user, null: false, index: true, foreign_key: { on_delete: :cascade } + t.datetime :created_at, null: false + t.string :path, limit: 511 + end + + add_index :lfs_file_locks, [:project_id, :path], unique: true + end + + def down + if foreign_keys_for(:lfs_file_locks, :project_id).any? + remove_foreign_key :lfs_file_locks, column: :project_id + end + + if index_exists?(:lfs_file_locks, [:project_id, :path]) + remove_concurrent_index :lfs_file_locks, [:project_id, :path] + end + + drop_table :lfs_file_locks + end +end diff --git a/db/schema.rb b/db/schema.rb index 9ab4dabfb7d..d07a4c31618 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -947,6 +947,16 @@ ActiveRecord::Schema.define(version: 20180206200543) do add_index "labels", ["title"], name: "index_labels_on_title", using: :btree add_index "labels", ["type", "project_id"], name: "index_labels_on_type_and_project_id", using: :btree + create_table "lfs_file_locks", force: :cascade do |t| + t.integer "project_id", null: false + t.integer "user_id", null: false + t.datetime "created_at", null: false + t.string "path", limit: 511 + end + + add_index "lfs_file_locks", ["project_id", "path"], name: "index_lfs_file_locks_on_project_id_and_path", unique: true, using: :btree + add_index "lfs_file_locks", ["user_id"], name: "index_lfs_file_locks_on_user_id", using: :btree + create_table "lfs_objects", force: :cascade do |t| t.string "oid", null: false t.integer "size", limit: 8, null: false @@ -1998,6 +2008,8 @@ ActiveRecord::Schema.define(version: 20180206200543) do add_foreign_key "label_priorities", "projects", on_delete: :cascade add_foreign_key "labels", "namespaces", column: "group_id", on_delete: :cascade add_foreign_key "labels", "projects", name: "fk_7de4989a69", on_delete: :cascade + add_foreign_key "lfs_file_locks", "projects", on_delete: :cascade + add_foreign_key "lfs_file_locks", "users", on_delete: :cascade add_foreign_key "lists", "boards", name: "fk_0d3f677137", on_delete: :cascade add_foreign_key "lists", "labels", name: "fk_7a5553d60f", on_delete: :cascade add_foreign_key "members", "users", name: "fk_2e88fb7ce9", on_delete: :cascade diff --git a/doc/workflow/lfs/manage_large_binaries_with_git_lfs.md b/doc/workflow/lfs/manage_large_binaries_with_git_lfs.md index ce7895780c3..8fff3d591fe 100644 --- a/doc/workflow/lfs/manage_large_binaries_with_git_lfs.md +++ b/doc/workflow/lfs/manage_large_binaries_with_git_lfs.md @@ -83,6 +83,72 @@ that are on the remote repository, eg. from branch `master`: git lfs fetch master ``` +## File Locking + +The first thing to do before using File Locking is to tell Git LFS which +kind of files are lockable. The following command will store PNG files +in LFS and flag them as lockable: + +```bash +git lfs track "*.png" --lockable +``` + +After executing the above command a file named `.gitattributes` will be +created or updated with the following content: + +```bash +*.png filter=lfs diff=lfs merge=lfs -text lockable +``` + +You can also register a file type as lockable without using LFS +(In order to be able to lock/unlock a file you need a remote server that implements the LFS File Locking API), +in order to do that you can edit the `.gitattributes` file manually: + +```bash +*.pdf lockable +``` + +After a file type has been registered as lockable, Git LFS will make +them readonly on the file system automatically. This means you will +need to lock the file before editing it. + +### Managing Locked Files + +Once you're ready to edit your file you need to lock it first: + +```bash +git lfs lock images/banner.png +Locked images/banner.png +``` + +This will register the file as locked in your name on the server: + +```bash +git lfs locks +images/banner.png joe ID:123 +``` + +Once you have pushed your changes, you can unlock the file so others can +also edit it: + +```bash +git lfs unlock images/banner.png +``` + +You can also unlock by id: + +```bash +git lfs unlock --id=123 +``` + +If for some reason you need to unlock a file that was not locked by you, +you can use the `--force` flag as long as you have a `master` access on +the project: + +```bash +git lfs unlock --id=123 --force +``` + ## Troubleshooting ### error: Repository or object not found diff --git a/lib/gitlab/checks/change_access.rb b/lib/gitlab/checks/change_access.rb index 945d70e7a24..d75e73dac10 100644 --- a/lib/gitlab/checks/change_access.rb +++ b/lib/gitlab/checks/change_access.rb @@ -31,13 +31,14 @@ module Gitlab @protocol = protocol end - def exec + def exec(skip_commits_check: false) return true if skip_authorization push_checks branch_checks tag_checks lfs_objects_exist_check + commits_check unless skip_commits_check true end @@ -117,6 +118,24 @@ module Gitlab end end + def commits_check + return if deletion? || newrev.nil? + + # n+1: https://gitlab.com/gitlab-org/gitlab-ee/issues/3593 + ::Gitlab::GitalyClient.allow_n_plus_1_calls do + commits.each do |commit| + commit_check.validate(commit, validations_for_commit(commit)) + end + end + + commit_check.validate_file_paths + end + + # Method overwritten in EE to inject custom validations + def validations_for_commit(_) + [] + end + private def updated_from_web? @@ -150,6 +169,14 @@ module Gitlab raise GitAccess::UnauthorizedError, ERROR_MESSAGES[:lfs_objects_missing] end end + + def commit_check + @commit_check ||= Gitlab::Checks::CommitCheck.new(project, user_access.user, newrev, oldrev) + end + + def commits + project.repository.new_commits(newrev) + end end end end diff --git a/lib/gitlab/checks/commit_check.rb b/lib/gitlab/checks/commit_check.rb new file mode 100644 index 00000000000..ae0cd142378 --- /dev/null +++ b/lib/gitlab/checks/commit_check.rb @@ -0,0 +1,61 @@ +module Gitlab + module Checks + class CommitCheck + include Gitlab::Utils::StrongMemoize + + attr_reader :project, :user, :newrev, :oldrev + + def initialize(project, user, newrev, oldrev) + @project = project + @user = user + @newrev = user + @oldrev = user + @file_paths = [] + end + + def validate(commit, validations) + return if validations.empty? && path_validations.empty? + + commit.raw_deltas.each do |diff| + @file_paths << (diff.new_path || diff.old_path) + + validations.each do |validation| + if error = validation.call(diff) + raise ::Gitlab::GitAccess::UnauthorizedError, error + end + end + end + end + + def validate_file_paths + path_validations.each do |validation| + if error = validation.call(@file_paths) + raise ::Gitlab::GitAccess::UnauthorizedError, error + end + end + end + + private + + def validate_lfs_file_locks? + strong_memoize(:validate_lfs_file_locks) do + project.lfs_enabled? && project.lfs_file_locks.any? && newrev && oldrev + end + end + + def lfs_file_locks_validation + lambda do |paths| + lfs_lock = project.lfs_file_locks.where(path: paths).where.not(user_id: user.id).first + + if lfs_lock + return "The path '#{lfs_lock.path}' is locked in Git LFS by #{lfs_lock.user.name}" + end + end + end + + def path_validations + validate_lfs_file_locks? ? [lfs_file_locks_validation] : [] + end + end + end +end diff --git a/lib/gitlab/import_export/import_export.yml b/lib/gitlab/import_export/import_export.yml index 2daed10f678..9f404003125 100644 --- a/lib/gitlab/import_export/import_export.yml +++ b/lib/gitlab/import_export/import_export.yml @@ -27,6 +27,8 @@ project_tree: - :releases - project_members: - :user + - lfs_file_locks: + - :user - merge_requests: - notes: - :author diff --git a/spec/factories/lfs_file_locks.rb b/spec/factories/lfs_file_locks.rb new file mode 100644 index 00000000000..b9d24f82b65 --- /dev/null +++ b/spec/factories/lfs_file_locks.rb @@ -0,0 +1,7 @@ +FactoryBot.define do + factory :lfs_file_lock do + user + project + path 'README.md' + end +end diff --git a/spec/lib/gitlab/checks/change_access_spec.rb b/spec/lib/gitlab/checks/change_access_spec.rb index c2bca816aae..475b5c5cfb2 100644 --- a/spec/lib/gitlab/checks/change_access_spec.rb +++ b/spec/lib/gitlab/checks/change_access_spec.rb @@ -177,5 +177,44 @@ describe Gitlab::Checks::ChangeAccess do expect { subject.exec }.not_to raise_error end end + + context 'LFS file lock check' do + let(:owner) { create(:user) } + let!(:lock) { create(:lfs_file_lock, user: owner, project: project, path: 'README') } + + before do + allow(project.repository).to receive(:new_commits).and_return( + project.repository.commits_between('be93687618e4b132087f430a4d8fc3a609c9b77c', '54fcc214b94e78d7a41a9a8fe6d87a5e59500e51') + ) + end + + context 'with LFS not enabled' do + it 'skips the validation' do + expect_any_instance_of(described_class).not_to receive(:lfs_file_locks_validation) + + subject.exec + end + end + + context 'with LFS enabled' do + before do + allow(project).to receive(:lfs_enabled?).and_return(true) + end + + context 'when change is sent by a different user' do + it 'raises an error if the user is not allowed to update the file' do + expect { subject.exec }.to raise_error(Gitlab::GitAccess::UnauthorizedError, "The path 'README' is locked in Git LFS by #{lock.user.name}") + end + end + + context 'when change is sent by the author od the lock' do + let(:user) { owner } + + it "doesn't raise any error" do + expect { subject.exec }.not_to raise_error + end + end + end + end end end diff --git a/spec/lib/gitlab/import_export/all_models.yml b/spec/lib/gitlab/import_export/all_models.yml index 0ecb50f7110..41a55027f4d 100644 --- a/spec/lib/gitlab/import_export/all_models.yml +++ b/spec/lib/gitlab/import_export/all_models.yml @@ -276,6 +276,7 @@ project: - fork_network_member - fork_network - custom_attributes +- lfs_file_locks award_emoji: - awardable - user @@ -290,3 +291,5 @@ push_event_payload: issue_assignees: - issue - assignee +lfs_file_locks: +- user diff --git a/spec/lib/gitlab/import_export/safe_model_attributes.yml b/spec/lib/gitlab/import_export/safe_model_attributes.yml index 5a33fa3fd53..feaab6673cd 100644 --- a/spec/lib/gitlab/import_export/safe_model_attributes.yml +++ b/spec/lib/gitlab/import_export/safe_model_attributes.yml @@ -530,3 +530,9 @@ ProjectCustomAttribute: - project_id - key - value +LfsFileLock: +- id +- path +- user_id +- project_id +- created_at diff --git a/spec/models/lfs_file_lock_spec.rb b/spec/models/lfs_file_lock_spec.rb new file mode 100644 index 00000000000..ce87b01b49c --- /dev/null +++ b/spec/models/lfs_file_lock_spec.rb @@ -0,0 +1,57 @@ +require 'rails_helper' + +describe LfsFileLock do + set(:lfs_file_lock) { create(:lfs_file_lock) } + subject { lfs_file_lock } + + it { is_expected.to belong_to(:project) } + it { is_expected.to belong_to(:user) } + + it { is_expected.to validate_presence_of(:project_id) } + it { is_expected.to validate_presence_of(:user_id) } + it { is_expected.to validate_presence_of(:path) } + + describe '#can_be_unlocked_by?' do + let(:developer) { create(:user) } + let(:master) { create(:user) } + + before do + project = lfs_file_lock.project + + project.add_developer(developer) + project.add_master(master) + end + + context "when it's forced" do + it 'can be unlocked by the author' do + user = lfs_file_lock.user + + expect(lfs_file_lock.can_be_unlocked_by?(user, true)).to eq(true) + end + + it 'can be unlocked by a master' do + expect(lfs_file_lock.can_be_unlocked_by?(master, true)).to eq(true) + end + + it "can't be unlocked by other user" do + expect(lfs_file_lock.can_be_unlocked_by?(developer, true)).to eq(false) + end + end + + context "when it isn't forced" do + it 'can be unlocked by the author' do + user = lfs_file_lock.user + + expect(lfs_file_lock.can_be_unlocked_by?(user)).to eq(true) + end + + it "can't be unlocked by a master" do + expect(lfs_file_lock.can_be_unlocked_by?(master)).to eq(false) + end + + it "can't be unlocked by other user" do + expect(lfs_file_lock.can_be_unlocked_by?(developer)).to eq(false) + end + end + end +end diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index 9dca7f326d3..c6ca038a2ba 100644 --- a/spec/models/project_spec.rb +++ b/spec/models/project_spec.rb @@ -80,6 +80,7 @@ describe Project do it { is_expected.to have_many(:members_and_requesters) } it { is_expected.to have_many(:clusters) } it { is_expected.to have_many(:custom_attributes).class_name('ProjectCustomAttribute') } + it { is_expected.to have_many(:lfs_file_locks) } context 'after initialized' do it "has a project_feature" do diff --git a/spec/models/repository_spec.rb b/spec/models/repository_spec.rb index 02a5ee54262..a6d48e369ac 100644 --- a/spec/models/repository_spec.rb +++ b/spec/models/repository_spec.rb @@ -262,6 +262,28 @@ describe Repository do end end + describe '#new_commits' do + let(:new_refs) do + double(:git_rev_list, new_refs: %w[ + c1acaa58bbcbc3eafe538cb8274ba387047b69f8 + 5937ac0a7beb003549fc5fd26fc247adbce4a52e + ]) + end + + it 'delegates to Gitlab::Git::RevList' do + expect(Gitlab::Git::RevList).to receive(:new).with( + repository.raw, + newrev: 'aaaabbbbccccddddeeeeffffgggghhhhiiiijjjj').and_return(new_refs) + + commits = repository.new_commits('aaaabbbbccccddddeeeeffffgggghhhhiiiijjjj') + + expect(commits).to eq([ + repository.commit('c1acaa58bbcbc3eafe538cb8274ba387047b69f8'), + repository.commit('5937ac0a7beb003549fc5fd26fc247adbce4a52e') + ]) + end + end + describe '#commits_by' do set(:project) { create(:project, :repository) } diff --git a/spec/requests/lfs_http_spec.rb b/spec/requests/lfs_http_spec.rb index 930ef49b7f3..971b45c411d 100644 --- a/spec/requests/lfs_http_spec.rb +++ b/spec/requests/lfs_http_spec.rb @@ -1208,7 +1208,7 @@ describe 'Git LFS API and storage' do end def post_lfs_json(url, body = nil, headers = nil) - post(url, body.try(:to_json), (headers || {}).merge('Content-Type' => 'application/vnd.git-lfs+json')) + post(url, body.try(:to_json), (headers || {}).merge('Content-Type' => LfsRequest::CONTENT_TYPE)) end def json_response diff --git a/spec/requests/lfs_locks_api_spec.rb b/spec/requests/lfs_locks_api_spec.rb new file mode 100644 index 00000000000..e44a11a7232 --- /dev/null +++ b/spec/requests/lfs_locks_api_spec.rb @@ -0,0 +1,159 @@ +require 'spec_helper' + +describe 'Git LFS File Locking API' do + include WorkhorseHelpers + + let(:project) { create(:project) } + let(:master) { create(:user) } + let(:developer) { create(:user) } + let(:guest) { create(:user) } + let(:path) { 'README.md' } + let(:headers) do + { + 'Authorization' => authorization + }.compact + end + + shared_examples 'unauthorized request' do + context 'when user is not authorized' do + let(:authorization) { authorize_user(guest) } + + it 'returns a forbidden 403 response' do + post_lfs_json url, body, headers + + expect(response).to have_gitlab_http_status(403) + end + end + end + + before do + allow(Gitlab.config.lfs).to receive(:enabled).and_return(true) + + project.add_developer(master) + project.add_developer(developer) + project.add_guest(guest) + end + + describe 'Create File Lock endpoint' do + let(:url) { "#{project.http_url_to_repo}/info/lfs/locks" } + let(:authorization) { authorize_user(developer) } + let(:body) { { path: path } } + + include_examples 'unauthorized request' + + context 'with an existent lock' do + before do + lock_file('README.md', developer) + end + + it 'return an error message' do + post_lfs_json url, body, headers + + expect(response).to have_gitlab_http_status(409) + + expect(json_response.keys).to match_array(%w(lock message documentation_url)) + expect(json_response['message']).to match(/already locked/) + end + + it 'returns the existen lock' do + post_lfs_json url, body, headers + + expect(json_response['lock']['path']).to eq('README.md') + end + end + + context 'without an existent lock' do + it 'creates the lock' do + post_lfs_json url, body, headers + + expect(response).to have_gitlab_http_status(201) + + expect(json_response['lock'].keys).to match_array(%w(id path locked_at owner)) + end + end + end + + describe 'Listing File Locks endpoint' do + let(:url) { "#{project.http_url_to_repo}/info/lfs/locks" } + let(:authorization) { authorize_user(developer) } + + include_examples 'unauthorized request' + + it 'returns the list of locked files' do + lock_file('README.md', developer) + lock_file('README', developer) + + do_get url, nil, headers + + expect(response).to have_gitlab_http_status(200) + + expect(json_response['locks'].size).to eq(2) + expect(json_response['locks'].first.keys).to match_array(%w(id path locked_at owner)) + end + end + + describe 'List File Locks for verification endpoint' do + let(:url) { "#{project.http_url_to_repo}/info/lfs/locks/verify" } + let(:authorization) { authorize_user(developer) } + + include_examples 'unauthorized request' + + it 'returns the list of locked files grouped by owner' do + lock_file('README.md', master) + lock_file('README', developer) + + post_lfs_json url, nil, headers + + expect(response).to have_gitlab_http_status(200) + + expect(json_response['ours'].size).to eq(1) + expect(json_response['ours'].first['path']).to eq('README') + expect(json_response['theirs'].size).to eq(1) + expect(json_response['theirs'].first['path']).to eq('README.md') + end + end + + describe 'Delete File Lock endpoint' do + let!(:lock) { lock_file('README.md', developer) } + let(:url) { "#{project.http_url_to_repo}/info/lfs/locks/#{lock[:id]}/unlock" } + let(:authorization) { authorize_user(developer) } + + include_examples 'unauthorized request' + + context 'with an existent lock' do + it 'deletes the lock' do + post_lfs_json url, nil, headers + + expect(response).to have_gitlab_http_status(200) + end + + it 'returns the deleted lock' do + post_lfs_json url, nil, headers + + expect(json_response['lock'].keys).to match_array(%w(id path locked_at owner)) + end + end + end + + def lock_file(path, author) + result = Lfs::LockFileService.new(project, author, { path: path }).execute + + result[:lock] + end + + def authorize_user(user) + ActionController::HttpAuthentication::Basic.encode_credentials(user.username, user.password) + end + + def post_lfs_json(url, body = nil, headers = nil) + post(url, body.try(:to_json), (headers || {}).merge('Content-Type' => LfsRequest::CONTENT_TYPE)) + end + + def do_get(url, params = nil, headers = nil) + get(url, (params || {}), (headers || {}).merge('Content-Type' => LfsRequest::CONTENT_TYPE)) + end + + def json_response + @json_response ||= JSON.parse(response.body) + end +end diff --git a/spec/serializers/lfs_file_lock_entity_spec.rb b/spec/serializers/lfs_file_lock_entity_spec.rb new file mode 100644 index 00000000000..5919f473a90 --- /dev/null +++ b/spec/serializers/lfs_file_lock_entity_spec.rb @@ -0,0 +1,19 @@ +require 'spec_helper' + +describe LfsFileLockEntity do + let(:user) { create(:user) } + let(:resource) { create(:lfs_file_lock, user: user) } + + let(:request) { double('request', current_user: user) } + + subject { described_class.new(resource, request: request).as_json } + + it 'exposes basic attrs of the lock' do + expect(subject).to include(:id, :path, :locked_at) + end + + it 'exposes the owner info' do + expect(subject).to include(:owner) + expect(subject[:owner][:name]).to eq(user.name) + end +end diff --git a/spec/services/lfs/lock_file_service_spec.rb b/spec/services/lfs/lock_file_service_spec.rb new file mode 100644 index 00000000000..3e58eea2501 --- /dev/null +++ b/spec/services/lfs/lock_file_service_spec.rb @@ -0,0 +1,62 @@ +require 'spec_helper' + +describe Lfs::LockFileService do + let(:project) { create(:project) } + let(:current_user) { create(:user) } + + subject { described_class.new(project, current_user, params) } + + describe '#execute' do + let(:params) { { path: 'README.md' } } + + context 'when not authorized' do + it "doesn't succeed" do + result = subject.execute + + expect(result[:status]).to eq(:error) + expect(result[:http_status]).to eq(403) + expect(result[:message]).to eq('You have no permissions') + end + end + + context 'when authorized' do + before do + project.add_developer(current_user) + end + + context 'with an existent lock' do + let!(:lock) { create(:lfs_file_lock, project: project) } + + it "doesn't succeed" do + expect(subject.execute[:status]).to eq(:error) + end + + it "doesn't create the Lock" do + expect do + subject.execute + end.not_to change { LfsFileLock.count } + end + end + + context 'without an existent lock' do + it "succeeds" do + expect(subject.execute[:status]).to eq(:success) + end + + it "creates the Lock" do + expect do + subject.execute + end.to change { LfsFileLock.count }.by(1) + end + end + + context 'when an error is raised' do + it "doesn't succeed" do + allow_any_instance_of(described_class).to receive(:create_lock!).and_raise(StandardError) + + expect(subject.execute[:status]).to eq(:error) + end + end + end + end +end diff --git a/spec/services/lfs/locks_finder_service_spec.rb b/spec/services/lfs/locks_finder_service_spec.rb new file mode 100644 index 00000000000..e409b77babf --- /dev/null +++ b/spec/services/lfs/locks_finder_service_spec.rb @@ -0,0 +1,101 @@ +require 'spec_helper' + +describe Lfs::LocksFinderService do + let(:project) { create(:project) } + let(:user) { create(:user) } + let(:params) { {} } + + subject { described_class.new(project, user, params) } + + shared_examples 'no results' do + it 'returns an empty list' do + result = subject.execute + + expect(result[:status]).to eq(:success) + expect(result[:locks]).to be_blank + end + end + + describe '#execute' do + let!(:lock_1) { create(:lfs_file_lock, project: project) } + let!(:lock_2) { create(:lfs_file_lock, project: project, path: 'README') } + + context 'find by id' do + context 'with results' do + let(:params) do + { id: lock_1.id } + end + + it 'returns the record' do + result = subject.execute + + expect(result[:status]).to eq(:success) + expect(result[:locks].size).to eq(1) + expect(result[:locks].first).to eq(lock_1) + end + end + + context 'without results' do + let(:params) do + { id: 123 } + end + + include_examples 'no results' + end + end + + context 'find by path' do + context 'with results' do + let(:params) do + { path: lock_1.path } + end + + it 'returns the record' do + result = subject.execute + + expect(result[:status]).to eq(:success) + expect(result[:locks].size).to eq(1) + expect(result[:locks].first).to eq(lock_1) + end + end + + context 'without results' do + let(:params) do + { path: 'not-found' } + end + + include_examples 'no results' + end + end + + context 'find all' do + context 'with results' do + it 'returns all the records' do + result = subject.execute + + expect(result[:status]).to eq(:success) + expect(result[:locks].size).to eq(2) + end + end + + context 'without results' do + before do + LfsFileLock.delete_all + end + + include_examples 'no results' + end + end + + context 'when an error is raised' do + it "doesn't succeed" do + allow_any_instance_of(described_class).to receive(:find_locks).and_raise(StandardError) + + result = subject.execute + + expect(result[:status]).to eq(:error) + expect(result[:locks]).to be_blank + end + end + end +end diff --git a/spec/services/lfs/unlock_file_service_spec.rb b/spec/services/lfs/unlock_file_service_spec.rb new file mode 100644 index 00000000000..4bea112b9c6 --- /dev/null +++ b/spec/services/lfs/unlock_file_service_spec.rb @@ -0,0 +1,105 @@ +require 'spec_helper' + +describe Lfs::UnlockFileService do + let(:project) { create(:project) } + let(:current_user) { create(:user) } + let(:lock_author) { create(:user) } + let!(:lock) { create(:lfs_file_lock, user: lock_author, project: project) } + let(:params) { {} } + + subject { described_class.new(project, current_user, params) } + + describe '#execute' do + context 'when not authorized' do + it "doesn't succeed" do + result = subject.execute + + expect(result[:status]).to eq(:error) + expect(result[:http_status]).to eq(403) + expect(result[:message]).to eq('You have no permissions') + end + end + + context 'when authorized' do + before do + project.add_developer(current_user) + end + + context 'when lock does not exists' do + let(:params) { { id: 123 } } + it "doesn't succeed" do + result = subject.execute + + expect(result[:status]).to eq(:error) + expect(result[:http_status]).to eq(404) + end + end + + context 'when unlocked by the author' do + let(:current_user) { lock_author } + let(:params) { { id: lock.id } } + + it "succeeds" do + result = subject.execute + + expect(result[:status]).to eq(:success) + expect(result[:lock]).to be_present + end + end + + context 'when unlocked by a different user' do + let(:current_user) { create(:user) } + let(:params) { { id: lock.id } } + + it "doesn't succeed" do + result = subject.execute + + expect(result[:status]).to eq(:error) + expect(result[:message]).to match(/is locked by GitLab User #{lock_author.id}/) + expect(result[:http_status]).to eq(403) + end + end + + context 'when forced' do + let(:developer) { create(:user) } + let(:master) { create(:user) } + + before do + project.add_developer(developer) + project.add_master(master) + end + + context 'by a regular user' do + let(:current_user) { developer } + let(:params) do + { id: lock.id, + force: true } + end + + it "doesn't succeed" do + result = subject.execute + + expect(result[:status]).to eq(:error) + expect(result[:message]).to match(/You must have master access/) + expect(result[:http_status]).to eq(403) + end + end + + context 'by a master user' do + let(:current_user) { master } + let(:params) do + { id: lock.id, + force: true } + end + + it "succeeds" do + result = subject.execute + + expect(result[:status]).to eq(:success) + expect(result[:lock]).to be_present + end + end + end + end + end +end -- cgit v1.2.1