From de7db9d75eecc16266f9331fa99aa7ab8cdf2e1e Mon Sep 17 00:00:00 2001 From: Fabio Huser Date: Tue, 21 Dec 2021 16:48:01 +0100 Subject: [PATCH] Add Group Access Token API endpoints This commit adds the group level counterpart to the existing [Project Access Token API](https://docs.gitlab.com/ee/api/resource_access_tokens.html). Group Access Tokens can already be created by instance administrators via rails console, but not by normal users. This change allows group owners to list, create and delete said tokens via GitLab REST API. Changelog: added --- app/models/group.rb | 5 + app/models/project.rb | 1 + app/policies/group_policy.rb | 9 + .../resource_access_tokens/create_service.rb | 2 +- doc/api/api_resources.md | 3 +- doc/api/group_access_tokens.md | 112 +++++++++++ lib/api/entities/resource_access_token.rb | 2 +- lib/api/resource_access_tokens.rb | 10 +- spec/models/group_spec.rb | 17 ++ spec/policies/group_policy_spec.rb | 45 ++++- .../api/resource_access_tokens_spec.rb | 187 ++++++++++-------- .../create_service_spec.rb | 45 +++-- .../revoke_service_spec.rb | 102 +++++----- 13 files changed, 386 insertions(+), 154 deletions(-) create mode 100644 doc/api/group_access_tokens.md diff --git a/app/models/group.rb b/app/models/group.rb index b18da3591e0310..fb9e6bcee79009 100644 --- a/app/models/group.rb +++ b/app/models/group.rb @@ -628,11 +628,16 @@ def group_member(user) group_members.find_by(user_id: user) end end + alias_method :resource_member, :group_member def highest_group_member(user) GroupMember.where(source_id: self_and_ancestors_ids, user_id: user.id).order(:access_level).last end + def bots + users.project_bot + end + def related_group_ids [id, *ancestors.pluck(:id), diff --git a/app/models/project.rb b/app/models/project.rb index 0b33f28f82cf94..5c4ffd083043ca 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -1667,6 +1667,7 @@ def project_member(user) project_members.find_by(user_id: user) end end + alias_method :resource_member, :project_member def membership_locked? false diff --git a/app/policies/group_policy.rb b/app/policies/group_policy.rb index 5c4990ffd9bd41..391da5b8a81b7d 100644 --- a/app/policies/group_policy.rb +++ b/app/policies/group_policy.rb @@ -23,6 +23,9 @@ class GroupPolicy < Namespaces::GroupProjectNamespaceSharedPolicy condition(:parent_share_with_group_locked, scope: :subject) { @subject.parent&.share_with_group_lock? } condition(:can_change_parent_share_with_group_lock) { can?(:change_share_with_group_lock, @subject.parent) } + desc "User is a project bot" + condition(:project_bot) { user.project_bot? && access_level >= GroupMember::GUEST } + condition(:has_projects) do group_projects_for(user: @user, group: @subject).any? end @@ -250,6 +253,8 @@ class GroupPolicy < Namespaces::GroupProjectNamespaceSharedPolicy enable :admin_dependency_proxy end + rule { project_bot }.enable :project_bot_access + rule { can?(:admin_group) & resource_access_token_feature_available }.policy do enable :read_resource_access_tokens enable :destroy_resource_access_tokens @@ -260,6 +265,10 @@ class GroupPolicy < Namespaces::GroupProjectNamespaceSharedPolicy enable :create_resource_access_tokens end + rule { can?(:project_bot_access) }.policy do + prevent :create_resource_access_tokens + end + rule { support_bot & has_project_with_service_desk_enabled }.policy do enable :read_label end diff --git a/app/services/resource_access_tokens/create_service.rb b/app/services/resource_access_tokens/create_service.rb index e0371e5d80f641..af0feef4eaa9a7 100644 --- a/app/services/resource_access_tokens/create_service.rb +++ b/app/services/resource_access_tokens/create_service.rb @@ -63,7 +63,7 @@ def default_user_params name: params[:name] || "#{resource.name.to_s.humanize} bot", email: generate_email, username: generate_username, - user_type: "#{resource_type}_bot".to_sym, + user_type: :project_bot, skip_confirmation: true # Bot users should always have their emails confirmed. } end diff --git a/doc/api/api_resources.md b/doc/api/api_resources.md index b5fac5019d9a2b..783823f80fbe24 100644 --- a/doc/api/api_resources.md +++ b/doc/api/api_resources.md @@ -25,7 +25,7 @@ The following API resources are available in the project context: | Resource | Available endpoints | |:------------------------------------------------------------------------|:--------------------| | [Access requests](access_requests.md) | `/projects/:id/access_requests` (also available for groups) | -| [Access tokens](resource_access_tokens.md) | `/projects/:id/access_tokens` | +| [Access tokens](resource_access_tokens.md) | `/projects/:id/access_tokens` (also available for groups) | | [Award emoji](award_emoji.md) | `/projects/:id/issues/.../award_emoji`, `/projects/:id/merge_requests/.../award_emoji`, `/projects/:id/snippets/.../award_emoji` | | [Branches](branches.md) | `/projects/:id/repository/branches/`, `/projects/:id/repository/merged_branches` | | [Commits](commits.md) | `/projects/:id/repository/commits`, `/projects/:id/statuses` | @@ -100,6 +100,7 @@ The following API resources are available in the group context: | Resource | Available endpoints | |:-----------------------------------------------------------------|:--------------------| | [Access requests](access_requests.md) | `/groups/:id/access_requests/` (also available for projects) | +| [Access tokens](group_access_tokens.md) | `/groups/:id/access_tokens` (also available for projects) | | [Custom attributes](custom_attributes.md) | `/groups/:id/custom_attributes` (also available for projects and users) | | [Debian distributions](packages/debian_group_distributions.md) | `/groups/:id/-/packages/debian` (also available for projects) | | [Deploy tokens](deploy_tokens.md) | `/groups/:id/deploy_tokens` (also available for projects and standalone) | diff --git a/doc/api/group_access_tokens.md b/doc/api/group_access_tokens.md new file mode 100644 index 00000000000000..71c6828de49636 --- /dev/null +++ b/doc/api/group_access_tokens.md @@ -0,0 +1,112 @@ +--- +stage: Manage +group: Authentication & Authorization +info: To determine the technical writer assigned to the Stage/Group associated with this page, see https://about.gitlab.com/handbook/engineering/ux/technical-writing/#assignments +--- + +# Group access tokens API **(FREE)** + +You can read more about [group access tokens](../user/project/settings/project_access_tokens.md#group-access-tokens). + +## List group access tokens + +> [Introduced](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/77236) in GitLab 14.7. + +Get a list of [group access tokens](../user/project/settings/project_access_tokens.md#group-access-tokens). + +```plaintext +GET groups/:id/access_tokens +``` + +| Attribute | Type | required | Description | +|-----------|---------|----------|---------------------| +| `id` | integer or string | yes | The ID or [URL-encoded path of the group](index.md#namespaced-path-encoding) | + +```shell +curl --header "PRIVATE-TOKEN: " "https://gitlab.example.com/api/v4/groups//access_tokens" +``` + +```json +[ + { + "user_id" : 141, + "scopes" : [ + "api" + ], + "name" : "token", + "expires_at" : "2021-01-31", + "id" : 42, + "active" : true, + "created_at" : "2021-01-20T22:11:48.151Z", + "revoked" : false, + "access_level": 40 + } +] +``` + +## Create a group access token + +> [Introduced](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/77236) in GitLab 14.7. + +Create a [group access token](../user/project/settings/project_access_tokens.md#group-access-tokens). + +```plaintext +POST groups/:id/access_tokens +``` + +| Attribute | Type | required | Description | +|-----------|---------|----------|---------------------| +| `id` | integer or string | yes | The ID or [URL-encoded path of the group](index.md#namespaced-path-encoding) | +| `name` | String | yes | The name of the group access token | +| `scopes` | `Array[String]` | yes | [List of scopes](../user/project/settings/project_access_tokens.md#scopes-for-a-project-access-token) | +| `access_level` | Integer | no | A valid access level. Default value is 40 (Maintainer). Other allowed values are 10 (Guest), 20 (Reporter), and 30 (Developer). | +| `expires_at` | Date | no | The token expires at midnight UTC on that date | + +```shell +curl --request POST --header "PRIVATE-TOKEN: " \ +--header "Content-Type:application/json" \ +--data '{ "name":"test_token", "scopes":["api", "read_repository"], "expires_at":"2021-01-31", "access_level": 30 }' \ +"https://gitlab.example.com/api/v4/groups//access_tokens" +``` + +```json +{ + "scopes" : [ + "api", + "read_repository" + ], + "active" : true, + "name" : "test", + "revoked" : false, + "created_at" : "2021-01-21T19:35:37.921Z", + "user_id" : 166, + "id" : 58, + "expires_at" : "2021-01-31", + "token" : "D4y...Wzr", + "access_level": 30 +} +``` + +## Revoke a group access token + +> [Introduced](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/77236) in GitLab 14.7. + +Revoke a [group access token](../user/project/settings/project_access_tokens.md#group-access-tokens). + +```plaintext +DELETE groups/:id/access_tokens/:token_id +``` + +| Attribute | Type | required | Description | +|-----------|---------|----------|---------------------| +| `id` | integer or string | yes | The ID or [URL-encoded path of the group](index.md#namespaced-path-encoding) | +| `token_id` | integer or string | yes | The ID of the group access token | + +```shell +curl --request DELETE --header "PRIVATE-TOKEN: " "https://gitlab.example.com/api/v4/groups//access_tokens/" +``` + +### Responses + +- `204: No Content` if successfully revoked. +- `400 Bad Request` or `404 Not Found` if not revoked successfully. diff --git a/lib/api/entities/resource_access_token.rb b/lib/api/entities/resource_access_token.rb index a1c7b28af45436..d16baed38f0507 100644 --- a/lib/api/entities/resource_access_token.rb +++ b/lib/api/entities/resource_access_token.rb @@ -4,7 +4,7 @@ module API module Entities class ResourceAccessToken < Entities::PersonalAccessToken expose :access_level do |token, options| - options[:project].project_member(token.user).access_level + options[:resource].resource_member(token.user).access_level end end end diff --git a/lib/api/resource_access_tokens.rb b/lib/api/resource_access_tokens.rb index f42acc6b2ebcf5..e52f8fd911166c 100644 --- a/lib/api/resource_access_tokens.rb +++ b/lib/api/resource_access_tokens.rb @@ -8,7 +8,7 @@ class ResourceAccessTokens < ::API::Base feature_category :authentication_and_authorization - %w[project].each do |source_type| + %w[project group].each do |source_type| resource source_type.pluralize, requirements: API::NAMESPACE_OR_PROJECT_REQUIREMENTS do desc 'Get list of all access tokens for the specified resource' do detail 'This feature was introduced in GitLab 13.9.' @@ -23,8 +23,8 @@ class ResourceAccessTokens < ::API::Base tokens = PersonalAccessTokensFinder.new({ user: resource.bots, impersonation: false }).execute.preload_users - resource.project_members.load - present paginate(tokens), with: Entities::ResourceAccessToken, project: resource + resource.members.load + present paginate(tokens), with: Entities::ResourceAccessToken, resource: resource end desc 'Revoke a resource access token' do @@ -58,7 +58,7 @@ class ResourceAccessTokens < ::API::Base requires :id, type: String, desc: "The #{source_type} ID" requires :name, type: String, desc: "Resource access token name" requires :scopes, type: Array[String], desc: "The permissions of the token" - optional :access_level, type: Integer, desc: "The access level of the token in the project" + optional :access_level, type: Integer, desc: "The access level of the token in the #{source_type}" optional :expires_at, type: Date, desc: "The expiration date of the token" end post ':id/access_tokens' do @@ -71,7 +71,7 @@ class ResourceAccessTokens < ::API::Base ).execute if token_response.success? - present token_response.payload[:access_token], with: Entities::ResourceAccessTokenWithToken, project: resource + present token_response.payload[:access_token], with: Entities::ResourceAccessTokenWithToken, resource: resource else bad_request!(token_response.message) end diff --git a/spec/models/group_spec.rb b/spec/models/group_spec.rb index 27aa9192579df4..f1a752fa434cfa 100644 --- a/spec/models/group_spec.rb +++ b/spec/models/group_spec.rb @@ -2066,6 +2066,23 @@ def setup_group_members(group) end end + describe '#bots' do + subject { group.bots } + + let_it_be(:group) { create(:group) } + let_it_be(:project_bot) { create(:user, :project_bot) } + let_it_be(:user) { create(:user) } + + before_all do + [project_bot, user].each do |member| + group.add_maintainer(member) + end + end + + it { is_expected.to contain_exactly(project_bot) } + it { is_expected.not_to include(user) } + end + describe '#related_group_ids' do let(:nested_group) { create(:group, parent: group) } let(:shared_with_group) { create(:group, parent: group) } diff --git a/spec/policies/group_policy_spec.rb b/spec/policies/group_policy_spec.rb index 7822ee2b92e277..7cd8d514c85d43 100644 --- a/spec/policies/group_policy_spec.rb +++ b/spec/policies/group_policy_spec.rb @@ -975,7 +975,7 @@ it { expect_disallowed(:read_label) } context 'when group hierarchy has a project with service desk enabled' do - let_it_be(:subgroup) { create(:group, :private, parent: group)} + let_it_be(:subgroup) { create(:group, :private, parent: group) } let_it_be(:project) { create(:project, group: subgroup, service_desk_enabled: true) } it { expect_allowed(:read_label) } @@ -983,6 +983,49 @@ end end + context "project bots" do + let(:project_bot) { create(:user, :project_bot) } + let(:user) { create(:user) } + + context "project_bot_access" do + context "when regular user and part of the group" do + let(:current_user) { user } + + before do + group.add_developer(user) + end + + it { is_expected.not_to be_allowed(:project_bot_access) } + end + + context "when project bot and not part of the project" do + let(:current_user) { project_bot } + + it { is_expected.not_to be_allowed(:project_bot_access) } + end + + context "when project bot and part of the project" do + let(:current_user) { project_bot } + + before do + group.add_developer(project_bot) + end + + it { is_expected.to be_allowed(:project_bot_access) } + end + end + + context 'with resource access tokens' do + let(:current_user) { project_bot } + + before do + group.add_maintainer(project_bot) + end + + it { is_expected.not_to be_allowed(:create_resource_access_tokens) } + end + end + describe 'update_runners_registration_token' do context 'admin' do let(:current_user) { admin } diff --git a/spec/requests/api/resource_access_tokens_spec.rb b/spec/requests/api/resource_access_tokens_spec.rb index 23061ab4bf0336..7e3e682767f3aa 100644 --- a/spec/requests/api/resource_access_tokens_spec.rb +++ b/spec/requests/api/resource_access_tokens_spec.rb @@ -3,25 +3,27 @@ require "spec_helper" RSpec.describe API::ResourceAccessTokens do - context "when the resource is a project" do - let_it_be(:project) { create(:project) } - let_it_be(:other_project) { create(:project) } - let_it_be(:user) { create(:user) } + let_it_be(:user) { create(:user) } + let_it_be(:user_non_priviledged) { create(:user) } - describe "GET projects/:id/access_tokens" do - subject(:get_tokens) { get api("/projects/#{project_id}/access_tokens", user) } + shared_examples 'resource access token API' do |source_type| + context "GET #{source_type}s/:id/access_tokens" do + subject(:get_tokens) { get api("/#{source_type}s/#{resource_id}/access_tokens", user) } - context "when the user has maintainer permissions" do + context "when the user has valid permissions" do let_it_be(:project_bot) { create(:user, :project_bot) } let_it_be(:access_tokens) { create_list(:personal_access_token, 3, user: project_bot) } - let_it_be(:project_id) { project.id } + let_it_be(:resource_id) { resource.id } before do - project.add_maintainer(user) - project.add_maintainer(project_bot) + if source_type == 'project' + resource.add_maintainer(project_bot) + else + resource.add_owner(project_bot) + end end - it "gets a list of access tokens for the specified project" do + it "gets a list of access tokens for the specified #{source_type}" do get_tokens token_ids = json_response.map { |token| token['id'] } @@ -38,16 +40,22 @@ expect(api_get_token["name"]).to eq(token.name) expect(api_get_token["scopes"]).to eq(token.scopes) - expect(api_get_token["access_level"]).to eq(project.team.max_member_access(token.user.id)) + + if source_type == 'project' + expect(api_get_token["access_level"]).to eq(resource.team.max_member_access(token.user.id)) + else + expect(api_get_token["access_level"]).to eq(resource.max_member_access_for_user(token.user)) + end + expect(api_get_token["expires_at"]).to eq(token.expires_at.to_date.iso8601) expect(api_get_token).not_to have_key('token') end - context "when using a project access token to GET other project access tokens" do + context "when using a #{source_type} access token to GET other #{source_type} access tokens" do let_it_be(:token) { access_tokens.first } - it "gets a list of access tokens for the specified project" do - get api("/projects/#{project_id}/access_tokens", personal_access_token: token) + it "gets a list of access tokens for the specified #{source_type}" do + get api("/#{source_type}s/#{resource_id}/access_tokens", personal_access_token: token) token_ids = json_response.map { |token| token['id'] } @@ -56,16 +64,15 @@ end end - context "when tokens belong to a different project" do + context "when tokens belong to a different #{source_type}" do let_it_be(:bot) { create(:user, :project_bot) } let_it_be(:token) { create(:personal_access_token, user: bot) } before do - other_project.add_maintainer(bot) - other_project.add_maintainer(user) + other_resource.add_maintainer(bot) end - it "does not return tokens from a different project" do + it "does not return tokens from a different #{source_type}" do get_tokens token_ids = json_response.map { |token| token['id'] } @@ -74,12 +81,8 @@ end end - context "when the project has no access tokens" do - let(:project_id) { other_project.id } - - before do - other_project.add_maintainer(user) - end + context "when the #{source_type} has no access tokens" do + let(:resource_id) { other_resource.id } it 'returns an empty array' do get_tokens @@ -89,8 +92,8 @@ end end - context "when trying to get the tokens of a different project" do - let_it_be(:project_id) { other_project.id } + context "when trying to get the tokens of a different #{source_type}" do + let_it_be(:resource_id) { unknown_resource.id } it "returns 404" do get_tokens @@ -99,8 +102,8 @@ end end - context "when the project does not exist" do - let(:project_id) { non_existing_record_id } + context "when the #{source_type} does not exist" do + let(:resource_id) { non_existing_record_id } it "returns 404" do get_tokens @@ -111,13 +114,13 @@ end context "when the user does not have valid permissions" do + let_it_be(:user) { user_non_priviledged } let_it_be(:project_bot) { create(:user, :project_bot) } let_it_be(:access_tokens) { create_list(:personal_access_token, 3, user: project_bot) } - let_it_be(:project_id) { project.id } + let_it_be(:resource_id) { resource.id } before do - project.add_developer(user) - project.add_maintainer(project_bot) + resource.add_maintainer(project_bot) end it "returns 401" do @@ -128,40 +131,36 @@ end end - describe "DELETE projects/:id/access_tokens/:token_id", :sidekiq_inline do - subject(:delete_token) { delete api("/projects/#{project_id}/access_tokens/#{token_id}", user) } + context "DELETE #{source_type}s/:id/access_tokens/:token_id", :sidekiq_inline do + subject(:delete_token) { delete api("/#{source_type}s/#{resource_id}/access_tokens/#{token_id}", user) } let_it_be(:project_bot) { create(:user, :project_bot) } let_it_be(:token) { create(:personal_access_token, user: project_bot) } - let_it_be(:project_id) { project.id } + let_it_be(:resource_id) { resource.id } let_it_be(:token_id) { token.id } before do - project.add_maintainer(project_bot) + resource.add_maintainer(project_bot) end - context "when the user has maintainer permissions" do - before do - project.add_maintainer(user) - end - - it "deletes the project access token from the project" do + context "when the user has valid permissions" do + it "deletes the #{source_type} access token from the #{source_type}" do delete_token expect(response).to have_gitlab_http_status(:no_content) expect(User.exists?(project_bot.id)).to be_falsy end - context "when using project access token to DELETE other project access token" do + context "when using #{source_type} access token to DELETE other #{source_type} access token" do let_it_be(:other_project_bot) { create(:user, :project_bot) } let_it_be(:other_token) { create(:personal_access_token, user: other_project_bot) } let_it_be(:token_id) { other_token.id } before do - project.add_maintainer(other_project_bot) + resource.add_maintainer(other_project_bot) end - it "deletes the project access token from the project" do + it "deletes the #{source_type} access token from the #{source_type}" do delete_token expect(response).to have_gitlab_http_status(:no_content) @@ -169,37 +168,31 @@ end end - context "when attempting to delete a non-existent project access token" do + context "when attempting to delete a non-existent #{source_type} access token" do let_it_be(:token_id) { non_existing_record_id } it "does not delete the token, and returns 404" do delete_token expect(response).to have_gitlab_http_status(:not_found) - expect(response.body).to include("Could not find project access token with token_id: #{token_id}") + expect(response.body).to include("Could not find #{source_type} access token with token_id: #{token_id}") end end - context "when attempting to delete a token that does not belong to the specified project" do - let_it_be(:project_id) { other_project.id } - - before do - other_project.add_maintainer(user) - end + context "when attempting to delete a token that does not belong to the specified #{source_type}" do + let_it_be(:resource_id) { other_resource.id } it "does not delete the token, and returns 404" do delete_token expect(response).to have_gitlab_http_status(:not_found) - expect(response.body).to include("Could not find project access token with token_id: #{token_id}") + expect(response.body).to include("Could not find #{source_type} access token with token_id: #{token_id}") end end end context "when the user does not have valid permissions" do - before do - project.add_developer(user) - end + let_it_be(:user) { user_non_priviledged } it "does not delete the token, and returns 400", :aggregate_failures do delete_token @@ -211,23 +204,19 @@ end end - describe "POST projects/:id/access_tokens" do + context "POST #{source_type}s/:id/access_tokens" do let(:params) { { name: "test", scopes: ["api"], expires_at: expires_at, access_level: access_level } } let(:expires_at) { 1.month.from_now } let(:access_level) { 20 } - subject(:create_token) { post api("/projects/#{project_id}/access_tokens", user), params: params } + subject(:create_token) { post api("/#{source_type}s/#{resource_id}/access_tokens", user), params: params } - context "when the user has maintainer permissions" do - let_it_be(:project_id) { project.id } - - before do - project.add_maintainer(user) - end + context "when the user has valid permissions" do + let_it_be(:resource_id) { resource.id } context "with valid params" do context "with full params" do - it "creates a project access token with the params", :aggregate_failures do + it "creates a #{source_type} access token with the params", :aggregate_failures do create_token expect(response).to have_gitlab_http_status(:created) @@ -242,7 +231,7 @@ context "when 'expires_at' is not set" do let(:expires_at) { nil } - it "creates a project access token with the params", :aggregate_failures do + it "creates a #{source_type} access token with the params", :aggregate_failures do create_token expect(response).to have_gitlab_http_status(:created) @@ -255,7 +244,7 @@ context "when 'access_level' is not set" do let(:access_level) { nil } - it 'creates a project access token with the default access level', :aggregate_failures do + it "creates a #{source_type} access token with the default access level", :aggregate_failures do create_token expect(response).to have_gitlab_http_status(:created) @@ -272,7 +261,7 @@ context "when missing the 'name' param" do let_it_be(:params) { { scopes: ["api"], expires_at: 5.days.from_now } } - it "does not create a project access token without 'name'" do + it "does not create a #{source_type} access token without 'name'" do create_token expect(response).to have_gitlab_http_status(:bad_request) @@ -283,7 +272,7 @@ context "when missing the 'scopes' param" do let_it_be(:params) { { name: "test", expires_at: 5.days.from_now } } - it "does not create a project access token without 'scopes'" do + it "does not create a #{source_type} access token without 'scopes'" do create_token expect(response).to have_gitlab_http_status(:bad_request) @@ -292,50 +281,80 @@ end end - context "when trying to create a token in a different project" do - let_it_be(:project_id) { other_project.id } + context "when trying to create a token in a different #{source_type}" do + let_it_be(:resource_id) { unknown_resource.id } - it "does not create the token, and returns the project not found error" do + it "does not create the token, and returns the #{source_type} not found error" do create_token expect(response).to have_gitlab_http_status(:not_found) - expect(response.body).to include("Project Not Found") + expect(response.body).to include("#{source_type.capitalize} Not Found") end end end context "when the user does not have valid permissions" do - let_it_be(:project_id) { project.id } + let_it_be(:resource_id) { resource.id } - context "when the user is a developer" do - before do - project.add_developer(user) - end + context "when the user role is too low" do + let_it_be(:user) { user_non_priviledged } it "does not create the token, and returns the permission error" do create_token expect(response).to have_gitlab_http_status(:bad_request) - expect(response.body).to include("User does not have permission to create project access token") + expect(response.body).to include("User does not have permission to create #{source_type} access token") end end - context "when a project access token tries to create another project access token" do + context "when a #{source_type} access token tries to create another #{source_type} access token" do let_it_be(:project_bot) { create(:user, :project_bot) } let_it_be(:user) { project_bot } before do - project.add_maintainer(user) + if source_type == 'project' + resource.add_maintainer(project_bot) + else + resource.add_owner(project_bot) + end end - it "does not allow a project access token to create another project access token" do + it "does not allow a #{source_type} access token to create another #{source_type} access token" do create_token expect(response).to have_gitlab_http_status(:bad_request) - expect(response.body).to include("User does not have permission to create project access token") + expect(response.body).to include("User does not have permission to create #{source_type} access token") end end end end end + + context 'when the resource is a project' do + let_it_be(:resource) { create(:project) } + let_it_be(:other_resource) { create(:project) } + let_it_be(:unknown_resource) { create(:project) } + + before_all do + resource.add_maintainer(user) + other_resource.add_maintainer(user) + resource.add_developer(user_non_priviledged) + end + + it_behaves_like 'resource access token API', 'project' + end + + context 'when the resource is a group' do + let_it_be(:resource) { create(:group) } + let_it_be(:other_resource) { create(:group) } + let_it_be(:unknown_resource) { create(:project) } + + before_all do + resource.add_owner(user) + other_resource.add_owner(user) + resource.add_maintainer(user_non_priviledged) + end + + it_behaves_like 'resource access token API', 'group' + end end diff --git a/spec/services/resource_access_tokens/create_service_spec.rb b/spec/services/resource_access_tokens/create_service_spec.rb index 42520ea26b2403..218bff7ed04538 100644 --- a/spec/services/resource_access_tokens/create_service_spec.rb +++ b/spec/services/resource_access_tokens/create_service_spec.rb @@ -7,10 +7,10 @@ let_it_be(:user) { create(:user) } let_it_be(:project) { create(:project, :private) } + let_it_be(:group) { create(:group, :private) } let_it_be(:params) { {} } describe '#execute' do - # Created shared_examples as it will easy to include specs for group bots in https://gitlab.com/gitlab-org/gitlab/-/issues/214046 shared_examples 'token creation fails' do let(:resource) { create(:project)} @@ -31,7 +31,7 @@ access_token = response.payload[:access_token] - expect(access_token.user.reload.user_type).to eq("#{resource_type}_bot") + expect(access_token.user.reload.user_type).to eq("project_bot") expect(access_token.user.created_by_id).to eq(user.id) end @@ -112,10 +112,8 @@ end context 'when user is external' do - let(:user) { create(:user, :external) } - before do - project.add_maintainer(user) + user.update!(external: true) end it 'creates resource bot user with external status' do @@ -162,7 +160,7 @@ access_token = response.payload[:access_token] project_bot = access_token.user - expect(project.members.find_by(user_id: project_bot.id).expires_at).to eq(nil) + expect(resource.members.find_by(user_id: project_bot.id).expires_at).to eq(nil) end end end @@ -183,7 +181,7 @@ access_token = response.payload[:access_token] project_bot = access_token.user - expect(project.members.find_by(user_id: project_bot.id).expires_at).to eq(params[:expires_at]) + expect(resource.members.find_by(user_id: project_bot.id).expires_at).to eq(params[:expires_at]) end end end @@ -234,24 +232,41 @@ end end + shared_examples 'when user does not have permission to create a resource bot' do + it_behaves_like 'token creation fails' + + it 'returns the permission error message' do + response = subject + + expect(response.error?).to be true + expect(response.errors).to include("User does not have permission to create #{resource_type} access token") + end + end + context 'when resource is a project' do let_it_be(:resource_type) { 'project' } let_it_be(:resource) { project } - context 'when user does not have permission to create a resource bot' do - it_behaves_like 'token creation fails' + it_behaves_like 'when user does not have permission to create a resource bot' - it 'returns the permission error message' do - response = subject - - expect(response.error?).to be true - expect(response.errors).to include("User does not have permission to create #{resource_type} access token") + context 'user with valid permission' do + before_all do + resource.add_maintainer(user) end + + it_behaves_like 'allows creation of bot with valid params' end + end + + context 'when resource is a project' do + let_it_be(:resource_type) { 'group' } + let_it_be(:resource) { group } + + it_behaves_like 'when user does not have permission to create a resource bot' context 'user with valid permission' do before_all do - resource.add_maintainer(user) + resource.add_owner(user) end it_behaves_like 'allows creation of bot with valid params' diff --git a/spec/services/resource_access_tokens/revoke_service_spec.rb b/spec/services/resource_access_tokens/revoke_service_spec.rb index 4f4e2ab0c9935e..3d724a79feffa8 100644 --- a/spec/services/resource_access_tokens/revoke_service_spec.rb +++ b/spec/services/resource_access_tokens/revoke_service_spec.rb @@ -6,11 +6,12 @@ subject { described_class.new(user, resource, access_token).execute } let_it_be(:user) { create(:user) } + let_it_be(:user_non_priviledged) { create(:user) } + let_it_be(:resource_bot) { create(:user, :project_bot) } let(:access_token) { create(:personal_access_token, user: resource_bot) } describe '#execute', :sidekiq_inline do - # Created shared_examples as it will easy to include specs for group bots in https://gitlab.com/gitlab-org/gitlab/-/issues/214046 shared_examples 'revokes access token' do it { expect(subject.success?).to be true } @@ -79,71 +80,80 @@ end end - context 'when resource is a project' do - let_it_be(:resource) { create(:project, :private) } + shared_examples 'revoke fails' do |resource_type| + let_it_be(:other_user) { create(:user) } - let(:resource_bot) { create(:user, :project_bot) } + context "when access token does not belong to this #{resource_type}" do + it 'does not find the bot' do + other_access_token = create(:personal_access_token, user: other_user) - before do - resource.add_maintainer(user) - resource.add_maintainer(resource_bot) - end + response = described_class.new(user, resource, other_access_token).execute - it_behaves_like 'revokes access token' + expect(response.success?).to be false + expect(response.message).to eq("Failed to find bot user") + expect(access_token.reload.revoked?).to be false + end + end - context 'revoke fails' do - let_it_be(:other_user) { create(:user) } + context 'when user does not have permission to destroy bot' do + context "when non-#{resource_type} member tries to delete project bot" do + it 'does not allow other user to delete bot' do + response = described_class.new(other_user, resource, access_token).execute - context 'when access token does not belong to this project' do - it 'does not find the bot' do - other_access_token = create(:personal_access_token, user: other_user) + expect(response.success?).to be false + expect(response.message).to eq("#{other_user.name} cannot delete #{access_token.user.name}") + expect(access_token.reload.revoked?).to be false + end + end - response = described_class.new(user, resource, other_access_token).execute + context "when non-priviledged #{resource_type} member tries to delete project bot" do + it 'does not allow developer to delete bot' do + response = described_class.new(user_non_priviledged, resource, access_token).execute expect(response.success?).to be false - expect(response.message).to eq("Failed to find bot user") + expect(response.message).to eq("#{user_non_priviledged.name} cannot delete #{access_token.user.name}") expect(access_token.reload.revoked?).to be false end end + end - context 'when user does not have permission to destroy bot' do - context 'when non-project member tries to delete project bot' do - it 'does not allow other user to delete bot' do - response = described_class.new(other_user, resource, access_token).execute - - expect(response.success?).to be false - expect(response.message).to eq("#{other_user.name} cannot delete #{access_token.user.name}") - expect(access_token.reload.revoked?).to be false - end + context 'when deletion of bot user fails' do + before do + allow_next_instance_of(::ResourceAccessTokens::RevokeService) do |service| + allow(service).to receive(:execute).and_return(false) end + end + + it_behaves_like 'rollback revoke steps' + end + end - context 'when non-maintainer project member tries to delete project bot' do - let(:developer) { create(:user) } + context 'when resource is a project' do + let_it_be(:resource) { create(:project, :private) } - before do - resource.add_developer(developer) - end + before do + resource.add_maintainer(user) + resource.add_developer(user_non_priviledged) + resource.add_maintainer(resource_bot) + end - it 'does not allow developer to delete bot' do - response = described_class.new(developer, resource, access_token).execute + it_behaves_like 'revokes access token' - expect(response.success?).to be false - expect(response.message).to eq("#{developer.name} cannot delete #{access_token.user.name}") - expect(access_token.reload.revoked?).to be false - end - end - end + it_behaves_like 'revoke fails', 'project' + end - context 'when deletion of bot user fails' do - before do - allow_next_instance_of(::ResourceAccessTokens::RevokeService) do |service| - allow(service).to receive(:execute).and_return(false) - end - end + context 'when resource is a group' do + let_it_be(:resource) { create(:group, :private) } - it_behaves_like 'rollback revoke steps' - end + before do + resource.add_owner(user) + resource.add_maintainer(user_non_priviledged) + resource.add_maintainer(resource_bot) end + + it_behaves_like 'revokes access token' + + it_behaves_like 'revoke fails', 'group' end end end -- GitLab