diff options
author | Douwe Maan <douwe@gitlab.com> | 2017-02-07 07:10:13 +0300 |
---|---|---|
committer | Douwe Maan <douwe@gitlab.com> | 2017-02-07 07:10:13 +0300 |
commit | 50f5960c72b4ff8b553bb0f7e8f649ac413f6ce0 (patch) | |
tree | 3fce35b12e29a10b538ce030ad7d661d332ba3e0 | |
parent | 437b46b9a18b1eb26c0caecf0ff6863d8ad9faa7 (diff) | |
parent | f5a798c7434bf236f36b399347c49fa3edf1f04e (diff) |
Merge branch 'ee-1439-read-only-user' into 'master'
Backport changes from gitlab-org/gitlab-ee!998
See merge request !8984
-rw-r--r-- | .flayignore | 1 | ||||
-rw-r--r-- | app/controllers/admin/users_controller.rb | 2 | ||||
-rw-r--r-- | app/finders/group_projects_finder.rb | 2 | ||||
-rw-r--r-- | app/models/user.rb | 15 | ||||
-rw-r--r-- | app/policies/project_policy.rb | 47 | ||||
-rw-r--r-- | app/policies/project_snippet_policy.rb | 2 | ||||
-rw-r--r-- | app/views/admin/users/_access_levels.html.haml | 37 | ||||
-rw-r--r-- | app/views/admin/users/_form.html.haml | 23 | ||||
-rw-r--r-- | app/views/projects/notes/_notes_with_form.html.haml | 2 | ||||
-rw-r--r-- | lib/gitlab/visibility_level.rb | 14 | ||||
-rw-r--r-- | spec/features/admin/admin_users_spec.rb | 2 | ||||
-rw-r--r-- | spec/models/user_spec.rb | 33 | ||||
-rw-r--r-- | spec/policies/project_policy_spec.rb | 62 | ||||
-rw-r--r-- | spec/policies/project_snippet_policy_spec.rb | 101 | ||||
-rw-r--r-- | spec/services/groups/update_service_spec.rb | 2 |
15 files changed, 265 insertions, 80 deletions
diff --git a/.flayignore b/.flayignore index 44df2ba2371..fc64b0b5892 100644 --- a/.flayignore +++ b/.flayignore @@ -1,3 +1,4 @@ *.erb lib/gitlab/sanitizers/svg/whitelist.rb lib/gitlab/diff/position_tracer.rb +app/policies/project_policy.rb diff --git a/app/controllers/admin/users_controller.rb b/app/controllers/admin/users_controller.rb index aa0f8d434dc..1cd50852e89 100644 --- a/app/controllers/admin/users_controller.rb +++ b/app/controllers/admin/users_controller.rb @@ -175,7 +175,7 @@ class Admin::UsersController < Admin::ApplicationController def user_params_ce [ - :admin, + :access_level, :avatar, :bio, :can_create_group, diff --git a/app/finders/group_projects_finder.rb b/app/finders/group_projects_finder.rb index aa8f4c1d0e4..3b9a421b118 100644 --- a/app/finders/group_projects_finder.rb +++ b/app/finders/group_projects_finder.rb @@ -18,7 +18,7 @@ class GroupProjectsFinder < UnionFinder projects = [] if current_user - if @group.users.include?(current_user) || current_user.admin? + if @group.users.include?(current_user) projects << @group.projects unless only_shared projects << @group.shared_projects unless only_owned else diff --git a/app/models/user.rb b/app/models/user.rb index 6c98224de35..f64d0c17a45 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -904,6 +904,21 @@ class User < ActiveRecord::Base end end + def access_level + if admin? + :admin + else + :regular + end + end + + def access_level=(new_level) + new_level = new_level.to_s + return unless %w(admin regular).include?(new_level) + + self.admin = (new_level == 'admin') + end + private def ci_projects_union diff --git a/app/policies/project_policy.rb b/app/policies/project_policy.rb index f5fd50745aa..f8594e29547 100644 --- a/app/policies/project_policy.rb +++ b/app/policies/project_policy.rb @@ -218,25 +218,7 @@ class ProjectPolicy < BasePolicy def anonymous_rules return unless project.public? - can! :read_project - can! :read_board - can! :read_list - can! :read_wiki - can! :read_label - can! :read_milestone - can! :read_project_snippet - can! :read_project_member - can! :read_merge_request - can! :read_note - can! :read_pipeline - can! :read_commit_status - can! :read_container_image - can! :download_code - can! :download_wiki_code - can! :read_cycle_analytics - - # NOTE: may be overridden by IssuePolicy - can! :read_issue + base_readonly_access! # Allow to read builds by anonymous user if guests are allowed can! :read_build if project.public_builds? @@ -269,4 +251,31 @@ class ProjectPolicy < BasePolicy :"admin_#{name}" ] end + + private + + # A base set of abilities for read-only users, which + # is then augmented as necessary for anonymous and other + # read-only users. + def base_readonly_access! + can! :read_project + can! :read_board + can! :read_list + can! :read_wiki + can! :read_label + can! :read_milestone + can! :read_project_snippet + can! :read_project_member + can! :read_merge_request + can! :read_note + can! :read_pipeline + can! :read_commit_status + can! :read_container_image + can! :download_code + can! :download_wiki_code + can! :read_cycle_analytics + + # NOTE: may be overridden by IssuePolicy + can! :read_issue + end end diff --git a/app/policies/project_snippet_policy.rb b/app/policies/project_snippet_policy.rb index 57acccfafd9..3a96836917e 100644 --- a/app/policies/project_snippet_policy.rb +++ b/app/policies/project_snippet_policy.rb @@ -3,7 +3,7 @@ class ProjectSnippetPolicy < BasePolicy can! :read_project_snippet if @subject.public? return unless @user - if @user && @subject.author == @user || @user.admin? + if @user && (@subject.author == @user || @user.admin?) can! :read_project_snippet can! :update_project_snippet can! :admin_project_snippet diff --git a/app/views/admin/users/_access_levels.html.haml b/app/views/admin/users/_access_levels.html.haml new file mode 100644 index 00000000000..7855239dfe5 --- /dev/null +++ b/app/views/admin/users/_access_levels.html.haml @@ -0,0 +1,37 @@ +%fieldset + %legend Access + .form-group + = f.label :projects_limit, class: 'control-label' + .col-sm-10= f.number_field :projects_limit, min: 0, class: 'form-control' + + .form-group + = f.label :can_create_group, class: 'control-label' + .col-sm-10= f.check_box :can_create_group + + .form-group + = f.label :access_level, class: 'control-label' + .col-sm-10 + - editing_current_user = (current_user == @user) + + = f.radio_button :access_level, :regular, disabled: editing_current_user + = label_tag :regular do + Regular + %p.light + Regular users have access to their groups and projects + + = f.radio_button :access_level, :admin, disabled: editing_current_user + = label_tag :admin do + Admin + %p.light + Administrators have access to all groups, projects and users and can manage all features in this installation + - if editing_current_user + %p.light + You cannot remove your own admin rights. + + .form-group + = f.label :external, class: 'control-label' + .col-sm-10 + = f.check_box :external do + External + %p.light + External users cannot see internal or private projects unless access is explicitly granted. Also, external users cannot create projects or groups. diff --git a/app/views/admin/users/_form.html.haml b/app/views/admin/users/_form.html.haml index 3145212728f..e911af3f6f9 100644 --- a/app/views/admin/users/_form.html.haml +++ b/app/views/admin/users/_form.html.haml @@ -40,28 +40,7 @@ = f.label :password_confirmation, class: 'control-label' .col-sm-10= f.password_field :password_confirmation, disabled: f.object.force_random_password, class: 'form-control' - %fieldset - %legend Access - .form-group - = f.label :projects_limit, class: 'control-label' - .col-sm-10= f.number_field :projects_limit, min: 0, class: 'form-control' - - .form-group - = f.label :can_create_group, class: 'control-label' - .col-sm-10= f.check_box :can_create_group - - .form-group - = f.label :admin, class: 'control-label' - - if current_user == @user - .col-sm-10= f.check_box :admin, disabled: true - .col-sm-10 You cannot remove your own admin rights. - - else - .col-sm-10= f.check_box :admin - - .form-group - = f.label :external, class: 'control-label' - .col-sm-10= f.check_box :external - .col-sm-10 External users cannot see internal or private projects unless access is explicitly granted. Also, external users cannot create projects or groups. + = render partial: 'access_levels', locals: { f: f } %fieldset %legend Profile diff --git a/app/views/projects/notes/_notes_with_form.html.haml b/app/views/projects/notes/_notes_with_form.html.haml index fbd2bff5bbb..08c73d94a09 100644 --- a/app/views/projects/notes/_notes_with_form.html.haml +++ b/app/views/projects/notes/_notes_with_form.html.haml @@ -13,7 +13,7 @@ = image_tag avatar_icon(current_user), alt: current_user.to_reference, class: 'avatar s40' .timeline-content.timeline-content-form = render "projects/notes/form", view: diff_view - - else + - elsif !current_user .disabled-comment.text-center .disabled-comment-text.inline Please diff --git a/lib/gitlab/visibility_level.rb b/lib/gitlab/visibility_level.rb index c7953af29dd..a4e966e4016 100644 --- a/lib/gitlab/visibility_level.rb +++ b/lib/gitlab/visibility_level.rb @@ -13,7 +13,19 @@ module Gitlab scope :public_and_internal_only, -> { where(visibility_level: [PUBLIC, INTERNAL] ) } scope :non_public_only, -> { where.not(visibility_level: PUBLIC) } - scope :public_to_user, -> (user) { user && !user.external ? public_and_internal_only : public_only } + scope :public_to_user, -> (user) do + if user + if user.admin? + all + elsif !user.external? + public_and_internal_only + else + public_only + end + else + public_only + end + end end PRIVATE = 0 unless const_defined?(:PRIVATE) diff --git a/spec/features/admin/admin_users_spec.rb b/spec/features/admin/admin_users_spec.rb index a586f8d3184..c0807b8c507 100644 --- a/spec/features/admin/admin_users_spec.rb +++ b/spec/features/admin/admin_users_spec.rb @@ -211,7 +211,7 @@ describe "Admin::Users", feature: true do fill_in "user_email", with: "bigbang@mail.com" fill_in "user_password", with: "AValidPassword1" fill_in "user_password_confirmation", with: "AValidPassword1" - check "user_admin" + choose "user_access_level_admin" click_button "Save changes" end diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index 2d09d7c7fed..fe88ec63af6 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -1422,4 +1422,37 @@ describe User, models: true do expect(user.project_authorizations.where(access_level: Gitlab::Access::REPORTER).exists?).to eq(true) end end + + describe '#access_level=' do + let(:user) { build(:user) } + + it 'does nothing for an invalid access level' do + user.access_level = :invalid_access_level + + expect(user.access_level).to eq(:regular) + expect(user.admin).to be false + end + + it "assigns the 'admin' access level" do + user.access_level = :admin + + expect(user.access_level).to eq(:admin) + expect(user.admin).to be true + end + + it "doesn't clear existing access levels when an invalid access level is passed in" do + user.access_level = :admin + user.access_level = :invalid_access_level + + expect(user.access_level).to eq(:admin) + expect(user.admin).to be true + end + + it "accepts string values in addition to symbols" do + user.access_level = 'admin' + + expect(user.access_level).to eq(:admin) + expect(user.admin).to be true + end + end end diff --git a/spec/policies/project_policy_spec.rb b/spec/policies/project_policy_spec.rb index eeab9827d99..0a5edf35f59 100644 --- a/spec/policies/project_policy_spec.rb +++ b/spec/policies/project_policy_spec.rb @@ -10,61 +10,59 @@ describe ProjectPolicy, models: true do let(:project) { create(:empty_project, :public, namespace: owner.namespace) } let(:guest_permissions) do - [ - :read_project, :read_board, :read_list, :read_wiki, :read_issue, :read_label, - :read_milestone, :read_project_snippet, :read_project_member, - :read_note, :create_project, :create_issue, :create_note, - :upload_file + %i[ + read_project read_board read_list read_wiki read_issue read_label + read_milestone read_project_snippet read_project_member + read_note create_project create_issue create_note + upload_file ] end let(:reporter_permissions) do - [ - :download_code, :fork_project, :create_project_snippet, :update_issue, - :admin_issue, :admin_label, :admin_list, :read_commit_status, :read_build, - :read_container_image, :read_pipeline, :read_environment, :read_deployment, - :read_merge_request, :download_wiki_code + %i[ + download_code fork_project create_project_snippet update_issue + admin_issue admin_label admin_list read_commit_status read_build + read_container_image read_pipeline read_environment read_deployment + read_merge_request download_wiki_code ] end let(:team_member_reporter_permissions) do - [ - :build_download_code, :build_read_container_image - ] + %i[build_download_code build_read_container_image] end let(:developer_permissions) do - [ - :admin_merge_request, :update_merge_request, :create_commit_status, - :update_commit_status, :create_build, :update_build, :create_pipeline, - :update_pipeline, :create_merge_request, :create_wiki, :push_code, - :resolve_note, :create_container_image, :update_container_image, - :create_environment, :create_deployment + %i[ + admin_merge_request update_merge_request create_commit_status + update_commit_status create_build update_build create_pipeline + update_pipeline create_merge_request create_wiki push_code + resolve_note create_container_image update_container_image + create_environment create_deployment ] end let(:master_permissions) do - [ - :push_code_to_protected_branches, :update_project_snippet, :update_environment, - :update_deployment, :admin_milestone, :admin_project_snippet, - :admin_project_member, :admin_note, :admin_wiki, :admin_project, - :admin_commit_status, :admin_build, :admin_container_image, - :admin_pipeline, :admin_environment, :admin_deployment + %i[ + push_code_to_protected_branches update_project_snippet update_environment + update_deployment admin_milestone admin_project_snippet + admin_project_member admin_note admin_wiki admin_project + admin_commit_status admin_build admin_container_image + admin_pipeline admin_environment admin_deployment ] end let(:public_permissions) do - [ - :download_code, :fork_project, :read_commit_status, :read_pipeline, - :read_container_image, :build_download_code, :build_read_container_image, - :download_wiki_code + %i[ + download_code fork_project read_commit_status read_pipeline + read_container_image build_download_code build_read_container_image + download_wiki_code ] end let(:owner_permissions) do - [ - :change_namespace, :change_visibility_level, :rename_project, :remove_project, - :archive_project, :remove_fork_project, :destroy_merge_request, :destroy_issue + %i[ + change_namespace change_visibility_level rename_project remove_project + archive_project remove_fork_project destroy_merge_request destroy_issue ] end diff --git a/spec/policies/project_snippet_policy_spec.rb b/spec/policies/project_snippet_policy_spec.rb new file mode 100644 index 00000000000..d0758af57dd --- /dev/null +++ b/spec/policies/project_snippet_policy_spec.rb @@ -0,0 +1,101 @@ +require 'spec_helper' + +describe ProjectSnippetPolicy, models: true do + let(:current_user) { create(:user) } + + let(:author_permissions) do + [ + :update_project_snippet, + :admin_project_snippet + ] + end + + subject { described_class.abilities(current_user, project_snippet).to_set } + + context 'public snippet' do + let(:project_snippet) { create(:project_snippet, :public) } + + context 'no user' do + let(:current_user) { nil } + + it do + is_expected.to include(:read_project_snippet) + is_expected.not_to include(*author_permissions) + end + end + + context 'regular user' do + it do + is_expected.to include(:read_project_snippet) + is_expected.not_to include(*author_permissions) + end + end + end + + context 'internal snippet' do + let(:project_snippet) { create(:project_snippet, :internal) } + + context 'no user' do + let(:current_user) { nil } + + it do + is_expected.not_to include(:read_project_snippet) + is_expected.not_to include(*author_permissions) + end + end + + context 'regular user' do + it do + is_expected.to include(:read_project_snippet) + is_expected.not_to include(*author_permissions) + end + end + end + + context 'private snippet' do + let(:project_snippet) { create(:project_snippet, :private) } + + context 'no user' do + let(:current_user) { nil } + + it do + is_expected.not_to include(:read_project_snippet) + is_expected.not_to include(*author_permissions) + end + end + + context 'regular user' do + it do + is_expected.not_to include(:read_project_snippet) + is_expected.not_to include(*author_permissions) + end + end + + context 'snippet author' do + let(:project_snippet) { create(:project_snippet, :private, author: current_user) } + + it do + is_expected.to include(:read_project_snippet) + is_expected.to include(*author_permissions) + end + end + + context 'project team member' do + before { project_snippet.project.team << [current_user, :developer] } + + it do + is_expected.to include(:read_project_snippet) + is_expected.not_to include(*author_permissions) + end + end + + context 'admin user' do + let(:current_user) { create(:admin) } + + it do + is_expected.to include(:read_project_snippet) + is_expected.to include(*author_permissions) + end + end + end +end diff --git a/spec/services/groups/update_service_spec.rb b/spec/services/groups/update_service_spec.rb index 531180e48a1..7c0fccb9d41 100644 --- a/spec/services/groups/update_service_spec.rb +++ b/spec/services/groups/update_service_spec.rb @@ -51,7 +51,7 @@ describe Groups::UpdateService, services: true do end context 'rename group' do - let!(:service) { described_class.new(internal_group, user, path: 'new_path') } + let!(:service) { described_class.new(internal_group, user, path: SecureRandom.hex) } before do internal_group.add_user(user, Gitlab::Access::MASTER) |