Create new policies for read, destroy, and create tokens

What does this MR do?

There are a few places in our codebase where can?(:admin_resource_access_tokens) is too broad, for example in the project access token API GET method where the user only needs token read permissions or when group admins want to (WIP) disable the creation of project access tokens but should still be able to read and destroy existing tokens. This MR breaks down the can?(:admin_resource_access_tokens) permission into create_*, read_*, and destroy_*.

It also introduces a column to namespace_settings called resource_access_token_creation_allowed, and introduces a policy called resource_access_token_creation_allowed? to add meaningful specs for the new create_*, read_*, and destroy_* policies, and to facilitate future work on disabling project access tokens.

Migration output:

Up:

% rails db:migrate
== 20210312193532 AddResourceAccessTokenCreationAllowedToNamespaceSettings: migrating 
-- add_column(:namespace_settings, :resource_access_token_creation_allowed, :boolean, {:default=>true, :null=>false})
   -> 0.0030s
== 20210312193532 AddResourceAccessTokenCreationAllowedToNamespaceSettings: migrated (0.0178s) 

Down:

% rails db:rollback STEP=1
== 20210312193532 AddResourceAccessTokenCreationAllowedToNamespaceSettings: reverting 
-- remove_column(:namespace_settings, :resource_access_token_creation_allowed)
   -> 0.0013s
== 20210312193532 AddResourceAccessTokenCreationAllowedToNamespaceSettings: reverted (0.0084s) 

Screenshots (strongly suggested)

Does this MR meet the acceptance criteria?

Conformity

• Does this MR need a changelog?
  • I have included a changelog entry.
  • I have not included a changelog entry because _____.
• Documentation (if required)
• Code review guidelines
• Merge request performance guidelines
• Style guides
• Database guides
• Separation of EE specific content

Availability and Testing

• Review and add/update tests for this feature/bug. Consider all test levels. See the Test Planning Process.
• Tested in all supported browsers
• Informed Infrastructure department of a default or new setting change, if applicable per definition of done

Security

If this MR contains changes to processing or storing of credentials or tokens, authorization and authentication methods and other items described in the security review guidelines:

• Label as security and @ mention @gitlab-com/gl-security/appsec
• The MR includes necessary changes to maintain consistency between UI, API, email, or other methods
• Security reports checked/validated by a reviewer from the AppSec team

added groupauthentication and authorization [DEPRECATED] label

marked this merge request as draft

added backend label

	2 Warnings
	This merge request includes more than 10 commits. Each commit should meet the following criteria: Have a well-written commit message. Has all tests passing when used on its own (e.g. when using git checkout SHA). Can be reverted on its own without also requiring the revert of commit that came before it. Is small enough that it can be reviewed in isolation in under 30 minutes or so. If this merge request contains commits that do not meet this criteria and/or contains intermediate work, please rebase these commits into a smaller number of commits or split this merge request into multiple smaller merge requests.
	This merge request does not refer to an existing milestone.

Reviewer roulette

Changes that require review have been detected! A merge request is normally reviewed by both a reviewer and a maintainer in its primary category (e.g. frontend or backend), and by a maintainer in all other categories.

To spread load more evenly across eligible reviewers, Danger has picked a candidate for each review slot, based on their timezone. Feel free to override these selections if you think someone else would be better-suited or use the GitLab Review Workload Dashboard to find other available reviewers.

To read more on how to use the reviewer roulette, please take a look at the Engineering workflow and code review guidelines. Please consider assigning a reviewer or maintainer who is a domain expert in the area of the merge request.

Once you've decided who will review this merge request, assign them as a reviewer! Danger does not automatically notify them for you.

Category	Reviewer	Maintainer
backend	Matt Kasa (@mattkasa) (UTC-7, 2 hours behind @serenafang)	Igor Drozdov (@igor.drozdov) (UTC+3, 8 hours ahead of @serenafang)
database	Andy Soiron (@Andysoiron) (UTC+1, 6 hours ahead of @serenafang)	Toon Claes (@toon) (UTC+1, 6 hours ahead of @serenafang)
frontend	Roman Kuba (@rkuba) (UTC+1, 6 hours ahead of @serenafang)	Nicolò Maria Mezzopera (@nmezzopera) (UTC+1, 6 hours ahead of @serenafang)
~migration	No reviewer available	No maintainer available
test Quality for spec/features/*	Matt Kasa (@mattkasa) (UTC-7, 2 hours behind @serenafang)	Maintainer review is optional for test Quality for spec/features/*

If needed, you can retry the danger-review job that generated this comment.

Generated by Danger

/cc @manojmj

added 1 commit

• b3f2f93f - Add specs for read and destroy tokens

Compare with previous version

changed the description

added 1 commit

• db8180d3 - Add resource access token creation enabled to table

Compare with previous version

added databasereview pending label

added database label

added 1 commit

• dea0e572 - Fix read token usage

Compare with previous version

added 1 commit

• 0b7c0a00 - Add changelog entry

Compare with previous version

Setting label(s) devopsmanage sectiondev based on ~"group::access".

added devopsmanage sectiondev labels

changed the description

added 1 commit

• 3340ed84 - Render create token form if can create token

Compare with previous version

added frontend label

added 1 commit

• 6f52bf8b - Modify project policy

Compare with previous version

added 1 commit

• 697a7af0 - Remove project helper

Compare with previous version

added 1 commit

• 17232069 - Update group policy

Compare with previous version

added 1 commit

• 42d26585 - Update group policy and revoke service

Compare with previous version

added 1 commit

• a265c674 - Fix sidebar permission

Compare with previous version

removed frontend label

changed the description

added 1 commit

• e2cdd468 - Change check can destroy token name

Compare with previous version

added frontend label

marked this merge request as ready

@manojmj Hi Manoj, could you backend review this please? Thanks

@alinamihaila Could you do the database review? Thanks!

@aturinske Could you do the frontend review? Thanks!

@sliaquat Hi Sanad, could you do the test/Quality review? Thanks

requested review from @manojmj, @alinamihaila, @aturinske, and @sliaquat

approved this merge request

added databasereviewed label and removed databasereview pending label

assigned to @toon

requested review from @toon

approved this merge request

requested review from @mrincon

added 1 commit

• e4d52ad8 - Address MR review comments

Compare with previous version

added 1 commit

• 402b1a1e - Rename resource access token available

Compare with previous version

added 3 commits

• c6af9043 - Revert "Address MR review comments"
• 6a9a9ad2 - Revert "Rename resource access token available"
• f3801164 - Add before checks to controller

Compare with previous version

added 1 commit

• b8601b56 - Render create token form if can create token

Compare with previous version

approved this merge request

marked the checklist item Label as security and @ mention @gitlab-com/gl-security/appsec as completed

I am labelling this with a security label and tagging @gitlab-com/gl-security/appsec

added security label

Question: @serenafang should we add extra checks when displaying the "revoke" token buttons?

I suspect that we previously assumed that as long as the form was displayed the user could revoke them, so I think we have to add a permissions check at this point:

• https://gitlab.com/gitlab-org/gitlab/blob/3dcc435c3ac0584625f25045486cd92136d10593/app/views/shared/access_tokens/_table.html.haml#L46

I also noticed that this form is reused, so we may have to consider different cases:

approved this merge request

@serenafang Looks good from database side. I still see some discussions open, and did you assign a backend maintainer already?

added databaseapproved label and removed databasereviewed label

unassigned @toon

added 1 commit

• 4331ffa0 - Add case for check permissions

Compare with previous version

changed title from Create new policies for read and destroy tokens to Create new policies for read, destroy, and create tokens

added 1 commit

• f5f5a101 - Modify changelog entry

Compare with previous version

@ebaque Would you mind backend maintainer reviewing this please? Thank you!

requested review from @ebaque

added 1468 commits

• f5f5a101...78c22ade - 1460 commits from branch master
• 19feb4d9 - Create new policies for read and destroy tokens
• 62bab8a5 - Add specs for read and destroy tokens
• c505e3e6 - Add resource access token creation enabled to table
• 1a2a81e4 - Fix read token usage
• be46d649 - Add changelog entry
• aba9635d - Render create token form if can create token
• ccbbd7c4 - Add case for check permissions
• 918e746c - Modify changelog entry

Compare with previous version

Bundle size analysis [beta]

This compares changes in bundle size for entry points between the commits b4bf4e7a and 22b99a52

Special assets

Entrypoint / Name	Size before	Size after	Diff	Diff in percent
average	3.04 MB	3.04 MB	-	0.0 %
mainChunk	1.87 MB	1.87 MB	-	0.0 %

---

Note: We do not have exact data for b4bf4e7a. So we have used data from: c73795c6.
The intended commit has no webpack pipeline, so we chose the last commit with one before it.

Please look at the full report for more details

---

Read more about how this report works.

Generated by Danger

added 12 commits

• 918e746c...761a8b63 - 4 commits from branch master
• 907cc6b0 - Create new policies for read and destroy tokens
• 915c1588 - Add specs for read and destroy tokens
• 754f3c40 - Add resource access token creation enabled to table
• 851c6c35 - Fix read token usage
• efdaeb7e - Add changelog entry
• 4e1e0514 - Render create token form if can create token
• b53f605e - Add case for check permissions
• be41d8f7 - Modify changelog entry

Compare with previous version

mentioned in issue gitlab-org/quality/triage-reports#2435 (closed)

added 1 commit

• 76c501e0 - Simplify check permissions method

Compare with previous version

requested review from @ankelly

approved this merge request

approved this merge request

approved this merge request

removed review request for @sliaquat

added 1 commit

• 79f7d5ce - Change return super to return true

Compare with previous version

added 487 commits

• 79f7d5ce...82da9ec1 - 477 commits from branch master
• e0426202 - Create new policies for read and destroy tokens
• 3d336c75 - Add specs for read and destroy tokens
• 7ad926ff - Add resource access token creation enabled to table
• 8f4591ca - Fix read token usage
• 88b2fe69 - Add changelog entry
• 7ceec68d - Render create token form if can create token
• 8df9d4ed - Add case for check permissions
• 4ca6508f - Modify changelog entry
• be8586b4 - Simplify check permissions method
• ef9e97a3 - Change return super to return true

Compare with previous version

added 1 commit

• 00b62cf9 - Change group and policy to return super

Compare with previous version

added 246 commits

• 00b62cf9...bb2bcbd3 - 235 commits from branch master
• 11d2b257 - Create new policies for read and destroy tokens
• c70a6991 - Add specs for read and destroy tokens
• 2ec1bf43 - Add resource access token creation enabled to table
• e5b48982 - Fix read token usage
• 8e4db7c6 - Add changelog entry
• 0bab2291 - Render create token form if can create token
• 2d5788db - Add case for check permissions
• 2826c64f - Modify changelog entry
• e418a0d0 - Simplify check permissions method
• 9b563145 - Change return super to return true
• 22b99a52 - Change group and policy to return super

Compare with previous version

resolved all threads

approved this merge request

enabled an automatic merge when the pipeline for cf25a5ce succeeds

merged

mentioned in commit 8d8c1e2e

mentioned in merge request !53783 (closed)

added workflowstaging label

mentioned in merge request !57756 (merged)

added security-awardsnomination label

added workflowproduction label and removed workflowstaging label

added security-awardsawarded label and removed security-awardsnomination label

Congratulations @serenafang, your Issue/Merge Request has been awarded! (Learn more about the Security Awards Program)

added releasedcandidate label

mentioned in merge request !62731 (merged)

added typemaintenance label