diff options
-rw-r--r-- | app/graphql/types/user_interface.rb | 16 | ||||
-rw-r--r-- | app/helpers/search_helper.rb | 24 | ||||
-rw-r--r-- | app/policies/project_policy.rb | 13 | ||||
-rw-r--r-- | app/views/layouts/_search.html.haml | 5 | ||||
-rw-r--r-- | app/views/projects/show.html.haml | 2 | ||||
-rw-r--r-- | doc/api/graphql/reference/index.md | 8 | ||||
-rw-r--r-- | lib/api/entities/user_safe.rb | 12 | ||||
-rw-r--r-- | lib/gitlab/git_access_wiki.rb | 7 | ||||
-rw-r--r-- | lib/sidebars/projects/menus/analytics_menu.rb | 2 | ||||
-rw-r--r-- | spec/features/projects/members/list_spec.rb | 2 | ||||
-rw-r--r-- | spec/features/projects_spec.rb | 18 | ||||
-rw-r--r-- | spec/graphql/types/user_type_spec.rb | 80 | ||||
-rw-r--r-- | spec/helpers/search_helper_spec.rb | 16 | ||||
-rw-r--r-- | spec/lib/api/entities/user_spec.rb | 45 | ||||
-rw-r--r-- | spec/lib/gitlab/git_access_wiki_spec.rb | 25 | ||||
-rw-r--r-- | spec/lib/sidebars/projects/menus/analytics_menu_spec.rb | 6 | ||||
-rw-r--r-- | spec/requests/api/graphql/user_query_spec.rb | 14 | ||||
-rw-r--r-- | spec/support/helpers/features/members_helpers.rb | 4 |
18 files changed, 277 insertions, 22 deletions
diff --git a/app/graphql/types/user_interface.rb b/app/graphql/types/user_interface.rb index 8c67275eb73..7cc201b6df4 100644 --- a/app/graphql/types/user_interface.rb +++ b/app/graphql/types/user_interface.rb @@ -29,7 +29,10 @@ module Types field :name, type: GraphQL::Types::String, null: false, - description: 'Human-readable name of the user.' + resolver_method: :redacted_name, + description: 'Human-readable name of the user. ' \ + 'Will return `****` if the user is a project bot and the requester does not have permission to read resource access tokens.' + field :state, type: Types::UserStateEnum, null: false, @@ -121,5 +124,16 @@ module Types ::Types::UserType end end + + def redacted_name + return object.name unless object.project_bot? + + return object.name if context[:current_user]&.can?(:read_resource_access_tokens, object.projects.first) + + # If the requester does not have permission to read the project bot name, + # the API returns an arbitrary string. UI changes will be addressed in a follow up issue: + # https://gitlab.com/gitlab-org/gitlab/-/issues/346058 + '****' + end end end diff --git a/app/helpers/search_helper.rb b/app/helpers/search_helper.rb index cb28025c900..aedb7df9e4f 100644 --- a/app/helpers/search_helper.rb +++ b/app/helpers/search_helper.rb @@ -201,18 +201,30 @@ module SearchHelper if @project && @project.repository.root_ref ref = @ref || @project.repository.root_ref - result = [ - { category: "In this project", label: _("Files"), url: project_tree_path(@project, ref) }, - { category: "In this project", label: _("Commits"), url: project_commits_path(@project, ref) }, - { category: "In this project", label: _("Network"), url: project_network_path(@project, ref) }, - { category: "In this project", label: _("Graph"), url: project_graph_path(@project, ref) }, + result = [] + + if can?(current_user, :download_code, @project) + result.concat([ + { category: "In this project", label: _("Files"), url: project_tree_path(@project, ref) }, + { category: "In this project", label: _("Commits"), url: project_commits_path(@project, ref) } + ]) + end + + if can?(current_user, :read_repository_graphs, @project) + result.concat([ + { category: "In this project", label: _("Network"), url: project_network_path(@project, ref) }, + { category: "In this project", label: _("Graph"), url: project_graph_path(@project, ref) } + ]) + end + + result.concat([ { category: "In this project", label: _("Issues"), url: project_issues_path(@project) }, { category: "In this project", label: _("Merge requests"), url: project_merge_requests_path(@project) }, { category: "In this project", label: _("Milestones"), url: project_milestones_path(@project) }, { category: "In this project", label: _("Snippets"), url: project_snippets_path(@project) }, { category: "In this project", label: _("Members"), url: project_project_members_path(@project) }, { category: "In this project", label: _("Wiki"), url: project_wikis_path(@project) } - ] + ]) if can?(current_user, :read_feature_flag, @project) result << { category: "In this project", label: _("Feature Flags"), url: project_feature_flags_path(@project) } diff --git a/app/policies/project_policy.rb b/app/policies/project_policy.rb index d81db357162..b3aa49a00ae 100644 --- a/app/policies/project_policy.rb +++ b/app/policies/project_policy.rb @@ -93,6 +93,11 @@ class ProjectPolicy < BasePolicy user.is_a?(DeployToken) && user.has_access_to?(project) && user.write_package_registry end + desc "Deploy token with read access" + condition(:download_code_deploy_token) do + user.is_a?(DeployToken) && user.has_access_to?(project) + end + desc "If user is authenticated via CI job token then the target project should be in scope" condition(:project_allowed_for_job_token) do !@user&.from_ci_job_token? || @user.ci_job_token_scope.includes?(project) @@ -506,6 +511,10 @@ class ProjectPolicy < BasePolicy prevent(:download_wiki_code) end + rule { download_code_deploy_token }.policy do + enable :download_wiki_code + end + rule { builds_disabled | repository_disabled }.policy do prevent(*create_read_update_admin_destroy(:build)) prevent(*create_read_update_admin_destroy(:pipeline_schedule)) @@ -687,12 +696,14 @@ class ProjectPolicy < BasePolicy rule { project_bot }.enable :project_bot_access + rule { can?(:read_all_resources) }.enable :read_resource_access_tokens + rule { can?(:admin_project) & resource_access_token_feature_available }.policy do enable :read_resource_access_tokens enable :destroy_resource_access_tokens end - rule { can?(:read_resource_access_tokens) & resource_access_token_creation_allowed }.policy do + rule { can?(:admin_project) & resource_access_token_feature_available & resource_access_token_creation_allowed }.policy do enable :create_resource_access_tokens end diff --git a/app/views/layouts/_search.html.haml b/app/views/layouts/_search.html.haml index 2d186dfbd91..0350dc82e46 100644 --- a/app/views/layouts/_search.html.haml +++ b/app/views/layouts/_search.html.haml @@ -29,8 +29,9 @@ = hidden_field_tag :scope, search_context.scope = hidden_field_tag :search_code, search_context.code_search? + - ref = search_context.ref if can?(current_user, :download_code, search_context.project) = hidden_field_tag :snippets, search_context.for_snippets? - = hidden_field_tag :repository_ref, search_context.ref + = hidden_field_tag :repository_ref, ref = hidden_field_tag :nav_source, 'navbar' -# workaround for non-JS feature specs, see spec/support/helpers/search_helpers.rb @@ -38,4 +39,4 @@ %noscript= button_tag 'Search' .search-autocomplete-opts.hide{ :'data-autocomplete-path' => search_autocomplete_path, :'data-autocomplete-project-id' => search_context.project.try(:id), - :'data-autocomplete-project-ref' => search_context.ref } + :'data-autocomplete-project-ref' => ref } diff --git a/app/views/projects/show.html.haml b/app/views/projects/show.html.haml index e515f1e7320..1cbb061784e 100644 --- a/app/views/projects/show.html.haml +++ b/app/views/projects/show.html.haml @@ -1,5 +1,4 @@ - current_route_path = request.fullpath.match(%r{-/tree/[^/]+/(.+$)}).to_a[1] -- add_page_startup_graphql_call('repository/path_last_commit', { projectPath: @project.full_path, ref: current_ref, path: current_route_path || "" }) - @content_class = "limit-container-width" unless fluid_layout - @skip_current_level_breadcrumb = true - add_page_specific_style 'page_bundles/project' @@ -14,6 +13,7 @@ = render "home_panel" - if can?(current_user, :download_code, @project) && @project.repository_languages.present? + - add_page_startup_graphql_call('repository/path_last_commit', { projectPath: @project.full_path, ref: current_ref, path: current_route_path || "" }) = repository_languages_bar(@project.repository_languages) = render "archived_notice", project: @project diff --git a/doc/api/graphql/reference/index.md b/doc/api/graphql/reference/index.md index 34af9736056..905b6d418a9 100644 --- a/doc/api/graphql/reference/index.md +++ b/doc/api/graphql/reference/index.md @@ -11651,7 +11651,7 @@ A user assigned to a merge request. | <a id="mergerequestassigneeid"></a>`id` | [`ID!`](#id) | ID of the user. | | <a id="mergerequestassigneelocation"></a>`location` | [`String`](#string) | Location of the user. | | <a id="mergerequestassigneemergerequestinteraction"></a>`mergeRequestInteraction` | [`UserMergeRequestInteraction`](#usermergerequestinteraction) | Details of this user's interactions with the merge request. | -| <a id="mergerequestassigneename"></a>`name` | [`String!`](#string) | Human-readable name of the user. | +| <a id="mergerequestassigneename"></a>`name` | [`String!`](#string) | Human-readable name of the user. Will return `****` if the user is a project bot and the requester does not have permission to read resource access tokens. | | <a id="mergerequestassigneenamespace"></a>`namespace` | [`Namespace`](#namespace) | Personal namespace of the user. | | <a id="mergerequestassigneeprojectmemberships"></a>`projectMemberships` | [`ProjectMemberConnection`](#projectmemberconnection) | Project memberships of the user. (see [Connections](#connections)) | | <a id="mergerequestassigneepublicemail"></a>`publicEmail` | [`String`](#string) | User's public email. | @@ -11903,7 +11903,7 @@ A user assigned to a merge request as a reviewer. | <a id="mergerequestreviewerid"></a>`id` | [`ID!`](#id) | ID of the user. | | <a id="mergerequestreviewerlocation"></a>`location` | [`String`](#string) | Location of the user. | | <a id="mergerequestreviewermergerequestinteraction"></a>`mergeRequestInteraction` | [`UserMergeRequestInteraction`](#usermergerequestinteraction) | Details of this user's interactions with the merge request. | -| <a id="mergerequestreviewername"></a>`name` | [`String!`](#string) | Human-readable name of the user. | +| <a id="mergerequestreviewername"></a>`name` | [`String!`](#string) | Human-readable name of the user. Will return `****` if the user is a project bot and the requester does not have permission to read resource access tokens. | | <a id="mergerequestreviewernamespace"></a>`namespace` | [`Namespace`](#namespace) | Personal namespace of the user. | | <a id="mergerequestreviewerprojectmemberships"></a>`projectMemberships` | [`ProjectMemberConnection`](#projectmemberconnection) | Project memberships of the user. (see [Connections](#connections)) | | <a id="mergerequestreviewerpublicemail"></a>`publicEmail` | [`String`](#string) | User's public email. | @@ -14970,7 +14970,7 @@ Core represention of a GitLab user. | <a id="usercoregroupmemberships"></a>`groupMemberships` | [`GroupMemberConnection`](#groupmemberconnection) | Group memberships of the user. (see [Connections](#connections)) | | <a id="usercoreid"></a>`id` | [`ID!`](#id) | ID of the user. | | <a id="usercorelocation"></a>`location` | [`String`](#string) | Location of the user. | -| <a id="usercorename"></a>`name` | [`String!`](#string) | Human-readable name of the user. | +| <a id="usercorename"></a>`name` | [`String!`](#string) | Human-readable name of the user. Will return `****` if the user is a project bot and the requester does not have permission to read resource access tokens. | | <a id="usercorenamespace"></a>`namespace` | [`Namespace`](#namespace) | Personal namespace of the user. | | <a id="usercoreprojectmemberships"></a>`projectMemberships` | [`ProjectMemberConnection`](#projectmemberconnection) | Project memberships of the user. (see [Connections](#connections)) | | <a id="usercorepublicemail"></a>`publicEmail` | [`String`](#string) | User's public email. | @@ -18058,7 +18058,7 @@ Implementations: | <a id="usergroupmemberships"></a>`groupMemberships` | [`GroupMemberConnection`](#groupmemberconnection) | Group memberships of the user. (see [Connections](#connections)) | | <a id="userid"></a>`id` | [`ID!`](#id) | ID of the user. | | <a id="userlocation"></a>`location` | [`String`](#string) | Location of the user. | -| <a id="username"></a>`name` | [`String!`](#string) | Human-readable name of the user. | +| <a id="username"></a>`name` | [`String!`](#string) | Human-readable name of the user. Will return `****` if the user is a project bot and the requester does not have permission to read resource access tokens. | | <a id="usernamespace"></a>`namespace` | [`Namespace`](#namespace) | Personal namespace of the user. | | <a id="userprojectmemberships"></a>`projectMemberships` | [`ProjectMemberConnection`](#projectmemberconnection) | Project memberships of the user. (see [Connections](#connections)) | | <a id="userpublicemail"></a>`publicEmail` | [`String`](#string) | User's public email. | diff --git a/lib/api/entities/user_safe.rb b/lib/api/entities/user_safe.rb index feb01767fd6..6006a076020 100644 --- a/lib/api/entities/user_safe.rb +++ b/lib/api/entities/user_safe.rb @@ -3,7 +3,17 @@ module API module Entities class UserSafe < Grape::Entity - expose :id, :name, :username + expose :id, :username + expose :name do |user| + next user.name unless user.project_bot? + + next user.name if options[:current_user]&.can?(:read_resource_access_tokens, user.projects.first) + + # If the requester does not have permission to read the project bot name, + # the API returns an arbitrary string. UI changes will be addressed in a follow up issue: + # https://gitlab.com/gitlab-org/gitlab/-/issues/346058 + '****' + end end end end diff --git a/lib/gitlab/git_access_wiki.rb b/lib/gitlab/git_access_wiki.rb index 0963eb6b72a..f8f61511265 100644 --- a/lib/gitlab/git_access_wiki.rb +++ b/lib/gitlab/git_access_wiki.rb @@ -27,6 +27,13 @@ module Gitlab :create_wiki end + override :check_download_access! + def check_download_access! + super + + raise ForbiddenError, download_forbidden_message if deploy_token && !deploy_token.can?(:download_wiki_code, container) + end + override :check_change_access! def check_change_access! raise ForbiddenError, write_to_wiki_message unless user_can_push? diff --git a/lib/sidebars/projects/menus/analytics_menu.rb b/lib/sidebars/projects/menus/analytics_menu.rb index b13b25d1cfe..2a89dc66219 100644 --- a/lib/sidebars/projects/menus/analytics_menu.rb +++ b/lib/sidebars/projects/menus/analytics_menu.rb @@ -60,7 +60,7 @@ module Sidebars end def repository_analytics_menu_item - if context.project.empty_repo? + if context.project.empty_repo? || !can?(context.current_user, :read_repository_graphs, context.project) return ::Sidebars::NilMenuItem.new(item_id: :repository_analytics) end diff --git a/spec/features/projects/members/list_spec.rb b/spec/features/projects/members/list_spec.rb index 25598146604..308098c72a1 100644 --- a/spec/features/projects/members/list_spec.rb +++ b/spec/features/projects/members/list_spec.rb @@ -147,7 +147,7 @@ RSpec.describe 'Project members list', :js do it 'does not show form used to change roles and "Expiration date" or the remove user button', :aggregate_failures do visit_members_page - page.within find_member_row(project_bot) do + page.within find_username_row(project_bot) do expect(page).not_to have_button('Maintainer') expect(page).to have_field('Expiration date', disabled: true) expect(page).not_to have_button('Remove member') diff --git a/spec/features/projects_spec.rb b/spec/features/projects_spec.rb index c4619b5498e..26deca9c8f1 100644 --- a/spec/features/projects_spec.rb +++ b/spec/features/projects_spec.rb @@ -383,6 +383,24 @@ RSpec.describe 'Project' do { form: '.rspec-merge-request-settings', input: '#project_printing_merge_request_link_enabled' }] end + describe 'view for a user without an access to a repo' do + let(:project) { create(:project, :repository) } + let(:user) { create(:user) } + + it 'does not contain default branch information in its content' do + default_branch = 'merge-commit-analyze-side-branch' + + project.add_guest(user) + project.change_head(default_branch) + + sign_in(user) + visit project_path(project) + + lines_with_default_branch = page.html.lines.select { |line| line.include?(default_branch) } + expect(lines_with_default_branch).to eq([]) + end + end + def remove_with_confirm(button_text, confirm_with, confirm_button_text = 'Confirm') click_button button_text fill_in 'confirm_name_input', with: confirm_with diff --git a/spec/graphql/types/user_type_spec.rb b/spec/graphql/types/user_type_spec.rb index 0bad8c95ba2..4e3f442dc71 100644 --- a/spec/graphql/types/user_type_spec.rb +++ b/spec/graphql/types/user_type_spec.rb @@ -44,6 +44,86 @@ RSpec.describe GitlabSchema.types['User'] do expect(described_class).to have_graphql_fields(*expected_fields) end + describe 'name field' do + let_it_be(:admin) { create(:user, :admin)} + let_it_be(:user) { create(:user) } + let_it_be(:requested_user) { create(:user, name: 'John Smith') } + let_it_be(:requested_project_bot) { create(:user, :project_bot, name: 'Project bot') } + let_it_be(:project) { create(:project, :public) } + + before do + project.add_maintainer(requested_project_bot) + end + + let(:username) { requested_user.username } + + let(:query) do + %( + query { + user(username: "#{username}") { + name + } + } + ) + end + + subject { GitlabSchema.execute(query, context: { current_user: current_user }).as_json.dig('data', 'user', 'name') } + + context 'user requests' do + let(:current_user) { user } + + context 'a user' do + it 'returns name' do + expect(subject).to eq('John Smith') + end + end + + context 'a project bot' do + let(:username) { requested_project_bot.username } + + context 'when requester is nil' do + let(:current_user) { nil } + + it 'returns `****`' do + expect(subject).to eq('****') + end + end + + it 'returns `****` for a regular user' do + expect(subject).to eq('****') + end + + context 'when requester is a project maintainer' do + before do + project.add_maintainer(user) + end + + it 'returns name' do + expect(subject).to eq('Project bot') + end + end + end + end + + context 'admin requests', :enable_admin_mode do + let(:current_user) { admin } + + context 'a user' do + it 'returns name' do + expect(subject).to eq('John Smith') + end + end + + context 'a project bot' do + let(:username) { requested_project_bot.username } + + it 'returns name' do + expect(subject).to eq('Project bot') + end + end + end + end + describe 'snippets field' do subject { described_class.fields['snippets'] } diff --git a/spec/helpers/search_helper_spec.rb b/spec/helpers/search_helper_spec.rb index 9e870658870..17dcbab09bb 100644 --- a/spec/helpers/search_helper_spec.rb +++ b/spec/helpers/search_helper_spec.rb @@ -174,12 +174,26 @@ RSpec.describe SearchHelper do context "with a current project" do before do @project = create(:project, :repository) + + allow(self).to receive(:can?).and_return(true) allow(self).to receive(:can?).with(user, :read_feature_flag, @project).and_return(false) end - it "includes project-specific sections", :aggregate_failures do + it 'returns repository related labels based on users abilities', :aggregate_failures do expect(search_autocomplete_opts("Files").size).to eq(1) expect(search_autocomplete_opts("Commits").size).to eq(1) + expect(search_autocomplete_opts("Network").size).to eq(1) + expect(search_autocomplete_opts("Graph").size).to eq(1) + + allow(self).to receive(:can?).with(user, :download_code, @project).and_return(false) + + expect(search_autocomplete_opts("Files").size).to eq(0) + expect(search_autocomplete_opts("Commits").size).to eq(0) + + allow(self).to receive(:can?).with(user, :read_repository_graphs, @project).and_return(false) + + expect(search_autocomplete_opts("Network").size).to eq(0) + expect(search_autocomplete_opts("Graph").size).to eq(0) end context 'when user does not have access to project' do diff --git a/spec/lib/api/entities/user_spec.rb b/spec/lib/api/entities/user_spec.rb index 9c9a157d68a..14dc60e1a5f 100644 --- a/spec/lib/api/entities/user_spec.rb +++ b/spec/lib/api/entities/user_spec.rb @@ -12,7 +12,7 @@ RSpec.describe API::Entities::User do subject { entity.as_json } it 'exposes correct attributes' do - expect(subject).to include(:bio, :location, :public_email, :skype, :linkedin, :twitter, :website_url, :organization, :job_title, :work_information, :pronouns) + expect(subject).to include(:name, :bio, :location, :public_email, :skype, :linkedin, :twitter, :website_url, :organization, :job_title, :work_information, :pronouns) end it 'exposes created_at if the current user can read the user profile' do @@ -31,12 +31,51 @@ RSpec.describe API::Entities::User do expect(subject[:bot]).to be_falsey end - context 'with bot user' do - let(:user) { create(:user, :security_bot) } + context 'with project bot user' do + let(:project) { create(:project) } + let(:user) { create(:user, :project_bot, name: 'secret') } + + before do + project.add_maintainer(user) + end it 'exposes user as a bot' do expect(subject[:bot]).to eq(true) end + + context 'when the requester is not an admin' do + it 'does not expose project bot user name' do + expect(subject[:name]).to eq('****') + end + end + + context 'when the requester is nil' do + let(:current_user) { nil } + + it 'does not expose project bot user name' do + expect(subject[:name]).to eq('****') + end + end + + context 'when the requester is a project maintainer' do + let(:current_user) { create(:user) } + + before do + project.add_maintainer(current_user) + end + + it 'exposes project bot user name' do + expect(subject[:name]).to eq('secret') + end + end + + context 'when the requester is an admin' do + let(:current_user) { create(:user, :admin) } + + it 'exposes project bot user name', :enable_admin_mode do + expect(subject[:name]).to eq('secret') + end + end end it 'exposes local_time' do diff --git a/spec/lib/gitlab/git_access_wiki_spec.rb b/spec/lib/gitlab/git_access_wiki_spec.rb index 5ada8a6ef40..27175dc8c44 100644 --- a/spec/lib/gitlab/git_access_wiki_spec.rb +++ b/spec/lib/gitlab/git_access_wiki_spec.rb @@ -79,5 +79,30 @@ RSpec.describe Gitlab::GitAccessWiki do let(:message) { include('wiki') } end end + + context 'when the actor is a deploy token' do + let_it_be(:actor) { create(:deploy_token, projects: [project]) } + let_it_be(:user) { actor } + + before do + project.project_feature.update_attribute(:wiki_access_level, wiki_access_level) + end + + subject { access.check('git-upload-pack', changes) } + + context 'when the wiki is enabled' do + let(:wiki_access_level) { ProjectFeature::ENABLED } + + it { expect { subject }.not_to raise_error } + end + + context 'when the wiki is disabled' do + let(:wiki_access_level) { ProjectFeature::DISABLED } + + it_behaves_like 'forbidden git access' do + let(:message) { 'You are not allowed to download files from this wiki.' } + end + end + end end end diff --git a/spec/lib/sidebars/projects/menus/analytics_menu_spec.rb b/spec/lib/sidebars/projects/menus/analytics_menu_spec.rb index 9d5f029fff5..6f2ca719bc9 100644 --- a/spec/lib/sidebars/projects/menus/analytics_menu_spec.rb +++ b/spec/lib/sidebars/projects/menus/analytics_menu_spec.rb @@ -102,6 +102,12 @@ RSpec.describe Sidebars::Projects::Menus::AnalyticsMenu do specify { is_expected.to be_nil } end + describe 'when a user does not have access to repository graphs' do + let(:current_user) { guest } + + specify { is_expected.to be_nil } + end + describe 'when the user does not have access' do let(:current_user) { nil } diff --git a/spec/requests/api/graphql/user_query_spec.rb b/spec/requests/api/graphql/user_query_spec.rb index 59b805bb25b..1cba3674d25 100644 --- a/spec/requests/api/graphql/user_query_spec.rb +++ b/spec/requests/api/graphql/user_query_spec.rb @@ -488,5 +488,19 @@ RSpec.describe 'getting user information' do end end end + + context 'the user is project bot' do + let(:user) { create(:user, :project_bot) } + + before do + post_graphql(query, current_user: current_user) + end + + context 'we only request basic fields' do + let(:user_fields) { %i[id name username state web_url avatar_url] } + + it_behaves_like 'a working graphql query' + end + end end end diff --git a/spec/support/helpers/features/members_helpers.rb b/spec/support/helpers/features/members_helpers.rb index 2e86e014a1b..bdadcb8af43 100644 --- a/spec/support/helpers/features/members_helpers.rb +++ b/spec/support/helpers/features/members_helpers.rb @@ -37,6 +37,10 @@ module Spec find_row(user.name) end + def find_username_row(user) + find_row(user.username) + end + def find_invited_member_row(email) find_row(email) end |