diff options
| -rw-r--r-- | GITLAB_SHELL_VERSION | 2 | ||||
| -rw-r--r-- | app/controllers/invites_controller.rb | 4 | ||||
| -rw-r--r-- | app/controllers/projects/pipelines_controller.rb | 2 | ||||
| -rw-r--r-- | app/policies/issue_policy.rb | 13 | ||||
| -rw-r--r-- | app/policies/personal_access_token_policy.rb | 2 | ||||
| -rw-r--r-- | app/policies/project_policy.rb | 1 | ||||
| -rw-r--r-- | app/views/invites/show.html.haml | 41 | ||||
| -rw-r--r-- | lib/api/personal_access_tokens.rb | 2 | ||||
| -rw-r--r-- | locale/gitlab.pot | 36 | ||||
| -rw-r--r-- | spec/controllers/invites_controller_spec.rb | 84 | ||||
| -rw-r--r-- | spec/controllers/projects/pipelines_controller_spec.rb | 49 | ||||
| -rw-r--r-- | spec/features/invites_spec.rb | 39 | ||||
| -rw-r--r-- | spec/features/projects/pipelines/pipeline_spec.rb | 5 | ||||
| -rw-r--r-- | spec/frontend/members/store/mutations_spec.js | 6 | ||||
| -rw-r--r-- | spec/lib/gitlab/email/handler/service_desk_handler_spec.rb | 19 | ||||
| -rw-r--r-- | spec/policies/issue_policy_spec.rb | 85 | ||||
| -rw-r--r-- | spec/policies/personal_access_token_policy_spec.rb | 7 | ||||
| -rw-r--r-- | spec/policies/project_policy_spec.rb | 4 | ||||
| -rw-r--r-- | spec/requests/api/personal_access_tokens_spec.rb | 12 |
19 files changed, 267 insertions, 146 deletions
diff --git a/GITLAB_SHELL_VERSION b/GITLAB_SHELL_VERSION index dc18eab7e57..bf4ba6520ea 100644 --- a/GITLAB_SHELL_VERSION +++ b/GITLAB_SHELL_VERSION @@ -1 +1 @@ -13.19.0 +13.19.1 diff --git a/app/controllers/invites_controller.rb b/app/controllers/invites_controller.rb index 0a9a9e03e94..cec2f0e4d8a 100644 --- a/app/controllers/invites_controller.rb +++ b/app/controllers/invites_controller.rb @@ -20,7 +20,7 @@ class InvitesController < ApplicationController end def accept - if member.accept_invite!(current_user) + if current_user_matches_invite? && member.accept_invite!(current_user) redirect_to invite_details[:path], notice: helpers.invite_accepted_notice(member) else redirect_back_or_default(options: { alert: _("The invitation could not be accepted.") }) @@ -52,7 +52,7 @@ class InvitesController < ApplicationController end def current_user_matches_invite? - @member.invite_email == current_user.email + current_user.verified_emails.include?(@member.invite_email) end def member? diff --git a/app/controllers/projects/pipelines_controller.rb b/app/controllers/projects/pipelines_controller.rb index 7779f3c3b65..415a0d66eda 100644 --- a/app/controllers/projects/pipelines_controller.rb +++ b/app/controllers/projects/pipelines_controller.rb @@ -8,7 +8,7 @@ class Projects::PipelinesController < Projects::ApplicationController before_action :pipeline, except: [:index, :new, :create, :charts, :config_variables] before_action :set_pipeline_path, only: [:show] before_action :authorize_read_pipeline! - before_action :authorize_read_build!, only: [:index] + before_action :authorize_read_build!, only: [:index, :show] before_action :authorize_read_analytics!, only: [:charts] before_action :authorize_create_pipeline!, only: [:new, :create, :config_variables] before_action :authorize_update_pipeline!, only: [:retry, :cancel] diff --git a/app/policies/issue_policy.rb b/app/policies/issue_policy.rb index e58179e320d..053243e2296 100644 --- a/app/policies/issue_policy.rb +++ b/app/policies/issue_policy.rb @@ -44,7 +44,18 @@ class IssuePolicy < IssuablePolicy enable :update_subscription end - rule { ~persisted & can?(:guest_access) }.policy do + # admin can set metadata on new issues + rule { ~persisted & admin }.policy do + enable :set_issue_metadata + end + + # support bot needs to be able to set metadata on new issues when service desk is enabled + rule { ~persisted & support_bot & can?(:guest_access) }.policy do + enable :set_issue_metadata + end + + # guest members need to be able to set issue metadata per https://gitlab.com/gitlab-org/gitlab/-/issues/300100 + rule { ~persisted & is_project_member & can?(:guest_access) }.policy do enable :set_issue_metadata end diff --git a/app/policies/personal_access_token_policy.rb b/app/policies/personal_access_token_policy.rb index 1e5404b7822..31c973f575b 100644 --- a/app/policies/personal_access_token_policy.rb +++ b/app/policies/personal_access_token_policy.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true class PersonalAccessTokenPolicy < BasePolicy - condition(:is_owner) { user && subject.user_id == user.id } + condition(:is_owner) { user && subject.user_id == user.id && !subject.impersonation } rule { (is_owner | admin) & ~blocked }.policy do enable :read_token diff --git a/app/policies/project_policy.rb b/app/policies/project_policy.rb index 3cb4644a60d..184ed44146f 100644 --- a/app/policies/project_policy.rb +++ b/app/policies/project_policy.rb @@ -661,6 +661,7 @@ class ProjectPolicy < BasePolicy rule { support_bot & ~service_desk_enabled }.policy do prevent :create_note prevent :read_project + prevent :guest_access end rule { project_bot }.enable :project_bot_access diff --git a/app/views/invites/show.html.haml b/app/views/invites/show.html.haml index ae13ef831dd..3622fc46983 100644 --- a/app/views/invites/show.html.haml +++ b/app/views/invites/show.html.haml @@ -1,29 +1,30 @@ - page_title _("Invitation") %h3.page-title= _("Invitation") -%p - = _("You have been invited") - - inviter = @member.created_by - - if inviter - = _("by") - = link_to inviter.name, user_url(inviter) - = _("to join %{source_name}") % { source_name: @invite_details[:title] } - %strong - = link_to @invite_details[:name], @invite_details[:url] - = _("as %{role}.") % { role: @member.human_access } +- if current_user_matches_invite? + - if member? + %p + = _("You are already a member of this %{member_source}.") % { member_source: @invite_details[:title] } + .actions + = link_to _("Go to %{source_name}") % { source_name: @invite_details[:title] }, @invite_details[:url], class: "btn gl-button btn-confirm" -- if member? - %p - = _("However, you are already a member of this %{member_source}. Sign in using a different account to accept the invitation.") % { member_source: @invite_details[:title] } + - else + %p + - inviter = @member.created_by + - link_to_inviter = link_to(inviter.name, user_url(inviter)) + - link_to_source = link_to(@invite_details[:name], @invite_details[:url]) + + = html_escape(_("You have been invited by %{link_to_inviter} to join %{source_name} %{strong_open}%{link_to_source}%{strong_close} as %{role}")) % { link_to_inviter: link_to_inviter, source_name: @invite_details[:title], strong_open: '<strong>'.html_safe, link_to_source: link_to_source, strong_close: '</strong>'.html_safe, role: @member.human_access } + + .actions + = link_to _("Accept invitation"), accept_invite_url(@token), method: :post, class: "btn gl-button btn-confirm" + = link_to _("Decline"), decline_invite_url(@token), method: :post, class: "btn gl-button btn-danger gl-ml-3" -- if !current_user_matches_invite? +- else %p - mail_to_invite_email = mail_to(@member.invite_email) - mail_to_current_user = mail_to(current_user.email) - link_to_current_user = link_to(current_user.to_reference, user_url(current_user)) - = _("Note that this invitation was sent to %{mail_to_invite_email}, but you are signed in as %{link_to_current_user} with email %{mail_to_current_user}.").html_safe % { mail_to_invite_email: mail_to_invite_email, mail_to_current_user: mail_to_current_user, link_to_current_user: link_to_current_user } - -- if !member? - .actions - = link_to _("Accept invitation"), accept_invite_url(@token), method: :post, class: "btn gl-button btn-confirm" - = link_to _("Decline"), decline_invite_url(@token), method: :post, class: "btn gl-button btn-danger gl-ml-3" + = _("This invitation was sent to %{mail_to_invite_email}, but you are signed in as %{link_to_current_user} with email %{mail_to_current_user}.").html_safe % { mail_to_invite_email: mail_to_invite_email, mail_to_current_user: mail_to_current_user, link_to_current_user: link_to_current_user } + %p + = _("Sign in as a user with the matching email address, add the email to this account, or sign-up for a new account using the matching email.") diff --git a/lib/api/personal_access_tokens.rb b/lib/api/personal_access_tokens.rb index 2c60938b75a..56590bb9a8f 100644 --- a/lib/api/personal_access_tokens.rb +++ b/lib/api/personal_access_tokens.rb @@ -23,7 +23,7 @@ module API helpers do def finder_params(current_user) - current_user.admin? ? { user: user(params[:user_id]) } : { user: current_user } + current_user.admin? ? { user: user(params[:user_id]) } : { user: current_user, impersonation: false } end def user(user_id) diff --git a/locale/gitlab.pot b/locale/gitlab.pot index feb3d972d2a..edbf027a005 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -15283,6 +15283,9 @@ msgstr "" msgid "Go full screen" msgstr "" +msgid "Go to %{source_name}" +msgstr "" + msgid "Go to commits" msgstr "" @@ -16418,9 +16421,6 @@ msgstr "" msgid "How many users will be evaluating the trial?" msgstr "" -msgid "However, you are already a member of this %{member_source}. Sign in using a different account to accept the invitation." -msgstr "" - msgid "I accept the %{terms_link}" msgstr "" @@ -22483,9 +22483,6 @@ msgstr "" msgid "Note that pushing to GitLab requires write access to this repository." msgstr "" -msgid "Note that this invitation was sent to %{mail_to_invite_email}, but you are signed in as %{link_to_current_user} with email %{mail_to_current_user}." -msgstr "" - msgid "Note: As an administrator you may like to configure %{github_integration_link}, which will allow login via GitHub and allow connecting repositories without generating a Personal Access Token." msgstr "" @@ -30159,6 +30156,9 @@ msgstr "" msgid "Sign in / Register" msgstr "" +msgid "Sign in as a user with the matching email address, add the email to this account, or sign-up for a new account using the matching email." +msgstr "" + msgid "Sign in preview" msgstr "" @@ -33342,6 +33342,9 @@ msgstr "" msgid "This group, its subgroups and projects will be removed on %{date} since its parent group '%{parent_group_name}'' has been scheduled for removal." msgstr "" +msgid "This invitation was sent to %{mail_to_invite_email}, but you are signed in as %{link_to_current_user} with email %{mail_to_current_user}." +msgstr "" + msgid "This is a \"Ghost User\", created to hold all issues authored by users that have since been deleted. This user cannot be removed." msgstr "" @@ -37140,6 +37143,9 @@ msgstr "" msgid "You are about to transfer the control of your account to %{group_name} group. This action is NOT reversible, you won't be able to access any of your groups and projects outside of %{group_name} once this transfer is complete." msgstr "" +msgid "You are already a member of this %{member_source}." +msgstr "" + msgid "You are an admin, which means granting access to %{client_name} will allow them to interact with GitLab as an admin as well. Proceed with caution." msgstr "" @@ -37470,7 +37476,7 @@ msgstr "" msgid "You have been granted %{member_human_access} access to project %{name}." msgstr "" -msgid "You have been invited" +msgid "You have been invited by %{link_to_inviter} to join %{source_name} %{strong_open}%{link_to_source}%{strong_close} as %{role}" msgstr "" msgid "You have been redirected to the only result; see the %{a_start}search results%{a_end} instead." @@ -38066,9 +38072,6 @@ msgstr "" msgid "archived:" msgstr "" -msgid "as %{role}." -msgstr "" - msgid "assign yourself" msgstr "" @@ -38528,14 +38531,14 @@ msgstr "" msgid "element is not a hierarchy" msgstr "" -msgid "email '%{email}' does not match the allowed domain of %{email_domains}" -msgid_plural "email '%{email}' does not match the allowed domains: %{email_domains}" -msgstr[0] "" -msgstr[1] "" - msgid "email '%{email}' is not a verified email." msgstr "" +msgid "email does not match the allowed domain of %{email_domains}" +msgid_plural "email does not match the allowed domains: %{email_domains}" +msgstr[0] "" +msgstr[1] "" + msgid "enabled" msgstr "" @@ -39568,9 +39571,6 @@ msgstr "" msgid "to help your contributors communicate effectively!" msgstr "" -msgid "to join %{source_name}" -msgstr "" - msgid "toggle collapse" msgstr "" diff --git a/spec/controllers/invites_controller_spec.rb b/spec/controllers/invites_controller_spec.rb index 6b94d186d5f..bca59813a23 100644 --- a/spec/controllers/invites_controller_spec.rb +++ b/spec/controllers/invites_controller_spec.rb @@ -24,9 +24,64 @@ RSpec.describe InvitesController do end end + shared_examples 'invite email match enforcement' do |error_status:, flash_alert: nil| + it 'accepts user if invite email matches signed in user' do + expect do + request + end.to change { project_members.include?(user) }.from(false).to(true) + + expect(response).to have_gitlab_http_status(:found) + expect(flash[:notice]).to include 'You have been granted' + end + + it 'accepts invite if invite email matches confirmed secondary email' do + secondary_email = create(:email, :confirmed, user: user) + member.update!(invite_email: secondary_email.email) + + expect do + request + end.to change { project_members.include?(user) }.from(false).to(true) + + expect(response).to have_gitlab_http_status(:found) + expect(flash[:notice]).to include 'You have been granted' + end + + it 'does not accept if invite email matches unconfirmed secondary email' do + secondary_email = create(:email, user: user) + member.update!(invite_email: secondary_email.email) + + expect do + request + end.not_to change { project_members.include?(user) } + + expect(response).to have_gitlab_http_status(error_status) + expect(flash[:alert]).to eq(flash_alert) + end + + it 'does not accept if invite email does not match signed in user' do + member.update!(invite_email: 'bogus@email.com') + + expect do + request + end.not_to change { project_members.include?(user) } + + expect(response).to have_gitlab_http_status(error_status) + expect(flash[:alert]).to eq(flash_alert) + end + end + describe 'GET #show' do subject(:request) { get :show, params: params } + context 'when logged in' do + before do + sign_in(user) + end + + it_behaves_like 'invite email match enforcement', error_status: :ok + it_behaves_like 'invalid token' + end + context 'when it is part of our invite email experiment' do let(:extra_params) { { invite_type: 'initial_email' } } @@ -58,34 +113,6 @@ RSpec.describe InvitesController do end end - context 'when logged in' do - before do - sign_in(user) - end - - it 'accepts user if invite email matches signed in user' do - expect do - request - end.to change { project_members.include?(user) }.from(false).to(true) - - expect(response).to have_gitlab_http_status(:found) - expect(flash[:notice]).to include 'You have been granted' - end - - it 'forces re-confirmation if email does not match signed in user' do - member.update!(invite_email: 'bogus@email.com') - - expect do - request - end.not_to change { project_members.include?(user) } - - expect(response).to have_gitlab_http_status(:ok) - expect(flash[:notice]).to be_nil - end - - it_behaves_like 'invalid token' - end - context 'when not logged in' do context 'when invite token belongs to a valid member' do context 'when instance allows sign up' do @@ -239,6 +266,7 @@ RSpec.describe InvitesController do subject(:request) { post :accept, params: params } + it_behaves_like 'invite email match enforcement', error_status: :redirect, flash_alert: 'The invitation could not be accepted.' it_behaves_like 'invalid token' end diff --git a/spec/controllers/projects/pipelines_controller_spec.rb b/spec/controllers/projects/pipelines_controller_spec.rb index a80c5fa82f6..5c64bdba2bc 100644 --- a/spec/controllers/projects/pipelines_controller_spec.rb +++ b/spec/controllers/projects/pipelines_controller_spec.rb @@ -298,35 +298,46 @@ RSpec.describe Projects::PipelinesController do end describe 'GET #show' do - render_views - - let_it_be(:pipeline) { create(:ci_pipeline, project: project) } - - subject { get_pipeline_html } - def get_pipeline_html get :show, params: { namespace_id: project.namespace, project_id: project, id: pipeline }, format: :html end - def create_build_with_artifacts(stage, stage_idx, name) - create(:ci_build, :artifacts, :tags, pipeline: pipeline, stage: stage, stage_idx: stage_idx, name: name) - end + context 'when the project is public' do + render_views - before do - create_build_with_artifacts('build', 0, 'job1') - create_build_with_artifacts('build', 0, 'job2') + let_it_be(:pipeline) { create(:ci_pipeline, project: project) } + + def create_build_with_artifacts(stage, stage_idx, name) + create(:ci_build, :artifacts, :tags, pipeline: pipeline, stage: stage, stage_idx: stage_idx, name: name) + end + + before do + create_build_with_artifacts('build', 0, 'job1') + create_build_with_artifacts('build', 0, 'job2') + end + + it 'avoids N+1 database queries', :request_store do + control_count = ActiveRecord::QueryRecorder.new { get_pipeline_html }.count + expect(response).to have_gitlab_http_status(:ok) + + create_build_with_artifacts('build', 0, 'job3') + + expect { get_pipeline_html }.not_to exceed_query_limit(control_count) + expect(response).to have_gitlab_http_status(:ok) + end end - it 'avoids N+1 database queries', :request_store do - get_pipeline_html + context 'when the project is private' do + let(:project) { create(:project, :private, :repository) } + let(:pipeline) { create(:ci_pipeline, project: project) } - control_count = ActiveRecord::QueryRecorder.new { get_pipeline_html }.count - expect(response).to have_gitlab_http_status(:ok) + it 'returns `not_found` when the user does not have access' do + sign_in(create(:user)) - create_build_with_artifacts('build', 0, 'job3') + get_pipeline_html - expect { get_pipeline_html }.not_to exceed_query_limit(control_count) - expect(response).to have_gitlab_http_status(:ok) + expect(response).to have_gitlab_http_status(:not_found) + end end end diff --git a/spec/features/invites_spec.rb b/spec/features/invites_spec.rb index a72cf033d61..be01488e3f2 100644 --- a/spec/features/invites_spec.rb +++ b/spec/features/invites_spec.rb @@ -90,48 +90,17 @@ RSpec.describe 'Group or Project invitations', :aggregate_failures do end context 'when signed in and an invite link is clicked' do - context 'when an invite email is a secondary email for the user' do - let(:invite_email) { 'user_secondary@example.com' } - - before do - sign_in(user) - visit invite_path(group_invite.raw_invite_token) - end - - it 'sends user to the invite url and allows them to decline' do - expect(current_path).to eq(invite_path(group_invite.raw_invite_token)) - expect(page).to have_content("Note that this invitation was sent to #{invite_email}") - expect(page).to have_content("but you are signed in as #{user.to_reference} with email #{user.email}") - - click_link('Decline') - - expect(page).to have_content('You have declined the invitation') - expect(current_path).to eq(dashboard_projects_path) - expect { group_invite.reload }.to raise_error ActiveRecord::RecordNotFound - end - - it 'sends uer to the invite url and allows them to accept' do - expect(current_path).to eq(invite_path(group_invite.raw_invite_token)) - expect(page).to have_content("Note that this invitation was sent to #{invite_email}") - expect(page).to have_content("but you are signed in as #{user.to_reference} with email #{user.email}") - - click_link('Accept invitation') - - expect(page).to have_content('You have been granted') - expect(current_path).to eq(activity_group_path(group)) - end - end - context 'when user is an existing member' do before do - sign_in(owner) + group.add_developer(user) + sign_in(user) visit invite_path(group_invite.raw_invite_token) end it 'shows message user already a member' do expect(current_path).to eq(invite_path(group_invite.raw_invite_token)) - expect(page).to have_link(owner.name, href: user_url(owner)) - expect(page).to have_content('However, you are already a member of this group.') + expect(page).to have_link(user.name, href: user_path(user)) + expect(page).to have_content('You are already a member of this group.') end end end diff --git a/spec/features/projects/pipelines/pipeline_spec.rb b/spec/features/projects/pipelines/pipeline_spec.rb index 70dc0bd04e8..b93cbddf553 100644 --- a/spec/features/projects/pipelines/pipeline_spec.rb +++ b/spec/features/projects/pipelines/pipeline_spec.rb @@ -361,9 +361,8 @@ RSpec.describe 'Pipeline', :js do let(:project) { create(:project, :public, :repository, public_builds: false) } let(:role) { :guest } - it 'does not show failed jobs tab pane' do - expect(page).to have_link('Pipeline') - expect(page).not_to have_content('Failed Jobs') + it 'does not show the pipeline details page' do + expect(page).to have_content('Not Found') end end end diff --git a/spec/frontend/members/store/mutations_spec.js b/spec/frontend/members/store/mutations_spec.js index 7ad7034eb6d..78bbad394a0 100644 --- a/spec/frontend/members/store/mutations_spec.js +++ b/spec/frontend/members/store/mutations_spec.js @@ -44,8 +44,7 @@ describe('Vuex members mutations', () => { describe('when error has a message', () => { it('shows error message', () => { const error = new Error('Request failed with status code 422'); - const message = - 'User email "john.smith@gmail.com" does not match the allowed domain of example.com'; + const message = 'User email does not match the allowed domain of example.com'; error.response = { data: { message }, @@ -88,8 +87,7 @@ describe('Vuex members mutations', () => { describe('when error has a message', () => { it('shows error message', () => { const error = new Error('Request failed with status code 422'); - const message = - 'User email "john.smith@gmail.com" does not match the allowed domain of example.com'; + const message = 'User email does not match the allowed domain of example.com'; error.response = { data: { message }, diff --git a/spec/lib/gitlab/email/handler/service_desk_handler_spec.rb b/spec/lib/gitlab/email/handler/service_desk_handler_spec.rb index 3a60564d8d2..bea49e8abc3 100644 --- a/spec/lib/gitlab/email/handler/service_desk_handler_spec.rb +++ b/spec/lib/gitlab/email/handler/service_desk_handler_spec.rb @@ -128,6 +128,25 @@ RSpec.describe Gitlab::Email::Handler::ServiceDeskHandler do expect(issue.assignees).to be_empty expect(issue.milestone).to be_nil end + + context 'when issues are set to private' do + before do + project.project_feature.update!(issues_access_level: ProjectFeature::PRIVATE) + end + + it 'applies quick action commands present on templates' do + file_content = %(Text from service_desk2 template \n/label ~#{label.title} \n/milestone %"#{milestone.name}") + set_template_file('service_desk2', file_content) + + receiver.execute + + issue = Issue.last + expect(issue.description).to include('Text from service_desk2 template') + expect(issue.label_ids).to include(label.id) + expect(issue.author_id).to eq(User.support_bot.id) + expect(issue.milestone).to eq(milestone) + end + end end end diff --git a/spec/policies/issue_policy_spec.rb b/spec/policies/issue_policy_spec.rb index bc09191f6ec..8ff936d5a35 100644 --- a/spec/policies/issue_policy_spec.rb +++ b/spec/policies/issue_policy_spec.rb @@ -11,13 +11,37 @@ RSpec.describe IssuePolicy do let(:reporter) { create(:user) } let(:group) { create(:group, :public) } let(:reporter_from_group_link) { create(:user) } + let(:non_member) { create(:user) } + let(:support_bot) { User.support_bot } def permissions(user, issue) described_class.new(user, issue) end + shared_examples 'support bot with service desk enabled' do + before do + allow(::Gitlab::IncomingEmail).to receive(:enabled?) { true } + allow(::Gitlab::IncomingEmail).to receive(:supports_wildcard?) { true } + + project.update!(service_desk_enabled: true) + end + + it 'allows support_bot to read issues, create and set metadata on new issues' do + expect(permissions(support_bot, issue)).to be_allowed(:read_issue, :read_issue_iid, :update_issue, :admin_issue, :set_issue_metadata) + expect(permissions(support_bot, issue_no_assignee)).to be_allowed(:read_issue, :read_issue_iid, :update_issue, :admin_issue, :set_issue_metadata) + expect(permissions(support_bot, new_issue)).to be_allowed(:create_issue, :set_issue_metadata) + end + end + + shared_examples 'support bot with service desk disabled' do + it 'allows support_bot to read issues, create and set metadata on new issues' do + expect(permissions(support_bot, issue)).to be_disallowed(:read_issue, :read_issue_iid, :update_issue, :admin_issue, :set_issue_metadata) + expect(permissions(support_bot, issue_no_assignee)).to be_disallowed(:read_issue, :read_issue_iid, :update_issue, :admin_issue, :set_issue_metadata) + expect(permissions(support_bot, new_issue)).to be_disallowed(:create_issue, :set_issue_metadata) + end + end + context 'a private project' do - let(:non_member) { create(:user) } let(:project) { create(:project, :private) } let(:issue) { create(:issue, project: project, assignees: [assignee], author: author) } let(:issue_no_assignee) { create(:issue, project: project) } @@ -34,12 +58,6 @@ RSpec.describe IssuePolicy do create(:project_group_link, group: group, project: project) end - it 'does not allow non-members to read issues' do - expect(permissions(non_member, issue)).to be_disallowed(:read_issue, :read_issue_iid, :update_issue, :admin_issue, :set_issue_metadata) - expect(permissions(non_member, issue_no_assignee)).to be_disallowed(:read_issue, :read_issue_iid, :update_issue, :admin_issue, :set_issue_metadata) - expect(permissions(non_member, new_issue)).to be_disallowed(:create_issue, :set_issue_metadata) - end - it 'allows guests to read issues' do expect(permissions(guest, issue)).to be_allowed(:read_issue, :read_issue_iid) expect(permissions(guest, issue)).to be_disallowed(:update_issue, :admin_issue, :set_issue_metadata) @@ -82,6 +100,15 @@ RSpec.describe IssuePolicy do expect(permissions(assignee, new_issue)).to be_allowed(:create_issue, :set_issue_metadata) end + it 'does not allow non-members to read, update or create issues' do + expect(permissions(non_member, issue)).to be_disallowed(:read_issue, :read_issue_iid, :update_issue, :admin_issue, :set_issue_metadata) + expect(permissions(non_member, issue_no_assignee)).to be_disallowed(:read_issue, :read_issue_iid, :update_issue, :admin_issue, :set_issue_metadata) + expect(permissions(non_member, new_issue)).to be_disallowed(:create_issue, :set_issue_metadata) + end + + it_behaves_like 'support bot with service desk disabled' + it_behaves_like 'support bot with service desk enabled' + context 'with confidential issues' do let(:confidential_issue) { create(:issue, :confidential, project: project, assignees: [assignee], author: author) } let(:confidential_issue_no_assignee) { create(:issue, :confidential, project: project) } @@ -196,7 +223,8 @@ RSpec.describe IssuePolicy do expect(permissions(author, issue_locked)).to be_allowed(:read_issue, :read_issue_iid, :update_issue) expect(permissions(author, issue_locked)).to be_disallowed(:admin_issue, :reopen_issue, :set_issue_metadata) - expect(permissions(author, new_issue)).to be_allowed(:create_issue, :set_issue_metadata) + expect(permissions(author, new_issue)).to be_allowed(:create_issue) + expect(permissions(author, new_issue)).to be_disallowed(:set_issue_metadata) end it 'allows issue assignees to read, reopen and update their issues' do @@ -208,14 +236,44 @@ RSpec.describe IssuePolicy do expect(permissions(assignee, issue_locked)).to be_allowed(:read_issue, :read_issue_iid, :update_issue) expect(permissions(assignee, issue_locked)).to be_disallowed(:admin_issue, :reopen_issue, :set_issue_metadata) + end - expect(permissions(author, new_issue)).to be_allowed(:create_issue, :set_issue_metadata) + it 'allows non-members to read and create issues' do + expect(permissions(non_member, issue)).to be_allowed(:read_issue, :read_issue_iid) + expect(permissions(non_member, issue_no_assignee)).to be_allowed(:read_issue, :read_issue_iid) + expect(permissions(non_member, new_issue)).to be_allowed(:create_issue) + end + + it 'allows non-members to read issues' do + expect(permissions(non_member, issue)).to be_allowed(:read_issue, :read_issue_iid) + expect(permissions(non_member, issue_no_assignee)).to be_allowed(:read_issue, :read_issue_iid) + end + + it 'does not allow non-members to update, admin or set metadata' do + expect(permissions(non_member, issue)).to be_disallowed(:update_issue, :admin_issue, :set_issue_metadata) + expect(permissions(non_member, issue_no_assignee)).to be_disallowed(:update_issue, :admin_issue, :set_issue_metadata) + expect(permissions(non_member, new_issue)).to be_disallowed(:set_issue_metadata) end + it 'allows support_bot to read issues' do + # support_bot is still allowed read access in public projects through :public_access permission, + # see project_policy public_access rules policy (rule { can?(:public_access) }.policy {...}) + expect(permissions(support_bot, issue)).to be_allowed(:read_issue, :read_issue_iid) + expect(permissions(support_bot, issue)).to be_disallowed(:update_issue, :admin_issue, :set_issue_metadata) + + expect(permissions(support_bot, issue_no_assignee)).to be_allowed(:read_issue, :read_issue_iid) + expect(permissions(support_bot, issue_no_assignee)).to be_disallowed(:update_issue, :admin_issue, :set_issue_metadata) + + expect(permissions(support_bot, new_issue)).to be_disallowed(:create_issue, :set_issue_metadata) + end + + it_behaves_like 'support bot with service desk enabled' + context 'when issues are private' do before do project.project_feature.update!(issues_access_level: ProjectFeature::PRIVATE) end + let(:issue) { create(:issue, project: project, author: author) } let(:visitor) { create(:user) } let(:admin) { create(:user, :admin) } @@ -258,6 +316,15 @@ RSpec.describe IssuePolicy do expect(permissions(admin, issue)).to be_disallowed(:create_note) end end + + it 'does not allow non-members to update or create issues' do + expect(permissions(non_member, issue)).to be_disallowed(:read_issue, :read_issue_iid, :update_issue, :admin_issue, :set_issue_metadata) + expect(permissions(non_member, issue_no_assignee)).to be_disallowed(:update_issue, :admin_issue, :set_issue_metadata) + expect(permissions(non_member, new_issue)).to be_disallowed(:create_issue, :set_issue_metadata) + end + + it_behaves_like 'support bot with service desk disabled' + it_behaves_like 'support bot with service desk enabled' end context 'with confidential issues' do diff --git a/spec/policies/personal_access_token_policy_spec.rb b/spec/policies/personal_access_token_policy_spec.rb index b5e8d40b133..e146133429b 100644 --- a/spec/policies/personal_access_token_policy_spec.rb +++ b/spec/policies/personal_access_token_policy_spec.rb @@ -41,6 +41,13 @@ RSpec.describe PersonalAccessTokenPolicy do it { is_expected.to be_allowed(:read_token) } it { is_expected.to be_allowed(:revoke_token) } end + + context 'subject of the impersonated token' do + let_it_be(:token) { build_stubbed(:personal_access_token, user: current_user, impersonation: true) } + + it { is_expected.to be_disallowed(:read_token) } + it { is_expected.to be_disallowed(:revoke_token) } + end end context 'current_user is a blocked administrator', :enable_admin_mode do diff --git a/spec/policies/project_policy_spec.rb b/spec/policies/project_policy_spec.rb index f3c92751d06..8f3cac205be 100644 --- a/spec/policies/project_policy_spec.rb +++ b/spec/policies/project_policy_spec.rb @@ -479,8 +479,8 @@ RSpec.describe ProjectPolicy do let(:current_user) { User.support_bot } context 'with service desk disabled' do - it { expect_allowed(:guest_access) } - it { expect_disallowed(:create_note, :read_project) } + it { expect_allowed(:public_access) } + it { expect_disallowed(:guest_access, :create_note, :read_project) } end context 'with service desk enabled' do diff --git a/spec/requests/api/personal_access_tokens_spec.rb b/spec/requests/api/personal_access_tokens_spec.rb index ccc5f322ff9..0ff2c46e693 100644 --- a/spec/requests/api/personal_access_tokens_spec.rb +++ b/spec/requests/api/personal_access_tokens_spec.rb @@ -6,6 +6,7 @@ RSpec.describe API::PersonalAccessTokens do let_it_be(:path) { '/personal_access_tokens' } let_it_be(:token1) { create(:personal_access_token) } let_it_be(:token2) { create(:personal_access_token) } + let_it_be(:token_impersonated) { create(:personal_access_token, impersonation: true, user: token1.user) } let_it_be(:current_user) { create(:user) } describe 'GET /personal_access_tokens' do @@ -24,8 +25,9 @@ RSpec.describe API::PersonalAccessTokens do get api(path, current_user), params: { user_id: token1.user.id } expect(response).to have_gitlab_http_status(:ok) - expect(json_response.count).to eq(1) + expect(json_response.count).to eq(2) expect(json_response.first['user_id']).to eq(token1.user.id) + expect(json_response.last['id']).to eq(token_impersonated.id) end end @@ -34,6 +36,7 @@ RSpec.describe API::PersonalAccessTokens do let_it_be(:user) { create(:user) } let_it_be(:token) { create(:personal_access_token, user: current_user)} let_it_be(:other_token) { create(:personal_access_token, user: user) } + let_it_be(:token_impersonated) { create(:personal_access_token, impersonation: true, user: current_user) } it 'returns all PATs belonging to the signed-in user' do get api(path, current_user, personal_access_token: token) @@ -95,6 +98,7 @@ RSpec.describe API::PersonalAccessTokens do context 'when current_user is not an administrator' do let_it_be(:user_token) { create(:personal_access_token, user: current_user) } let_it_be(:user_token_path) { "/personal_access_tokens/#{user_token.id}" } + let_it_be(:token_impersonated) { create(:personal_access_token, impersonation: true, user: current_user) } it 'fails revokes a different users token' do delete api(path, current_user) @@ -107,6 +111,12 @@ RSpec.describe API::PersonalAccessTokens do expect(response).to have_gitlab_http_status(:no_content) end + + it 'cannot revoke impersonation token' do + delete api("/personal_access_tokens/#{token_impersonated.id}", current_user) + + expect(response).to have_gitlab_http_status(:bad_request) + end end end end |