Version Control for Snippets: Access Rights Check for project snippet

changed milestone to %12.8

added Deliverable backend devopscreate direction groupeditor [DEPRECATED] typefeature workflowin dev + 1 deleted label

changed title from WIP: Resolve "Version Control for Snippets: Implement access rights checking for git requests (Step 3)" to WIP: Version Control for Snippets: Access Rights Check

changed the description

assigned to @lulalala

added feature flag label

added 4 commits

c3fab0ef - Create snippet repository models
e9831719 - Renamed subject to repositorable
4cbd8f49 - Fixing rubocop offenses
d3640072 - Remove unrelated spec setup

Compare with previous version

	1 Warning
	This merge request is quite big (more than 530 lines changed), please consider splitting it into multiple merge requests.

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 randomly picked a candidate for each review slot. Feel free to override this selection if you think someone else would be better-suited, or the chosen person is unavailable.

To read more on how to use the reviewer roulette, please take a look at the Engineering workflow and code review guidelines.

Once you've decided who will review this merge request, mention them as you normally would! Danger does not (yet?) automatically notify them for you.

Category	Reviewer	Maintainer
backend	Sean Arnold (`@seanarnold`)	Sean McGivern (`@smcgivern`)

Generated by Danger

added 141 commits

d3640072...06119f68 - 138 commits from branch fj-39176-create-project-snippet-repository
67e05762 - Remove unrelated spec setup
d2145682 - Implement geo access check
0d276a49 - Check all accesss

Compare with previous version

changed the description

added 2 commits

525e2b31 - Extract snippet related permission table
03c16870 - Handle project snippet in different class

Compare with previous version

added backstage [DEPRECATED] label

added 1 commit

466cd6ca - Handle project snippet git access check

Compare with previous version

changed the description

added security label

unmarked as a Work In Progress

changed title from WIP: Version Control for Snippets: Access Rights Check to Version Control for Snippets: Access Rights Check for project snippet

Hi @fjsanpedro this is the initial implementation, for project snippet, focusing only on user as actor. Could you review please? Thanks!

assigned to @fjsanpedro

@lulalala I think we can reuse most of the logic by inheriting from GitAccessSnippet instead of GitAccess. Also notice that snippet permissions have changed recently. We no longer have read_project_snippet or read_personal_snippet but read_snippet. With these new permissions names, we can also reuse the logic in both checkers (in case we need both).

unassigned @fjsanpedro

@lulalala ah, sorry, I forgot to say that for the MVP we need to limit the number of files to 1. After the MVP we will raise that limit.

This means that, when we receive a git operation we have to check the repository, check if there is a file, and if it exists then we need to check the git command. If it's an update or move then ok but if it's a create operation we have to reject it.

There might other edge cases for this operation.

added 1773 commits

466cd6ca...65ca3467 - 1771 commits from branch fj-39176-create-project-snippet-repository
3352dc2e - Extract snippet related permission table
0befd3b4 - Handle project snippet git access check

Compare with previous version

added 1 commit

5ea508b1 - Handle project snippet git access check

Compare with previous version

Hi @fjsanpedro I've made some changes for you to review.

The spec failure is due to master. Once we rebase it will go away.

I've decided to leave the single file edit checks for the next MR, in which I plan to add a UserAccessForSnippet.

assigned to @fjsanpedro

Nice @lulalala! I've made some comments.

unassigned @fjsanpedro

added 2 commits

a00bef64 - Move shared snippet message to parent class
6a4cca7c - Add UserAccessSnippet

Compare with previous version

Hi @fjsanpedro I've updated the MR with a new UserAccessSnippet class. Could you review? Thanks!

assigned to @fjsanpedro

changed the description

@lulalala nice work. I left some comments.

unassigned @fjsanpedro

added 7 commits

52af1932 - Remove caching method calls
7adf473d - Consolidate common checks
1abdcb80 - Refactor project spec
9fcb55d0 - Remove unused spec
a5341688 - Override check for complete freedom
7d15e2f8 - Mark personal as to be implemented
135fda7f - Fix more spec

Compare with previous version

Hi @fjsanpedro I've made the updates. Can you recheck? Thanks.

assigned to @fjsanpedro

added 1 commit

572201c4 - Simplify UserAccess project assignment

Compare with previous version

@lulalala some more feedback, nothing major though.

unassigned @fjsanpedro

added 1442 commits

572201c4...1ea6cbbb - 1285 commits from branch fj-39176-create-project-snippet-repository
9fa88a3b - Add mention of closing issues after error resolve
9cd9f288 - Increase paid signup experimentation ratio
066f3d39 - Change flag to docker-services
531b3ad6 - Use new spinner in admin/application_settings
991b2a5e - Fix vertical alignment of spinner inside buttons
9e9b5e69 - Only show Progress Bar for new users
ae70b705 - Allow Boards::ListService to find a single board
c43156cd - Add tooltip when dates are too long
126e4bdd - Fix ProjectAuthorization calculation for shared groups
3a67004c - Merge branch 'security-fix_project_auth_calculation_for_group_shares-master' into 'master'
ddf4c84d - Switched Auto-DevOps Test from only/except to rules
ebf086bb - Fix hot module reload for error tracking pages
f4406d8f - Clear host memoization for feature test
f210da52 - Better rendering of version text
cab55bf1 - Add documentation for logs times ranges
e504302b - Hide duplicate company/individual question
f13fb32a - Un-skip web terminal spec
cb650598 - Stop caching WebIDE terminal pipeline status
28c8bd3f - Add changelog for MR 24443
bfb09b56 - Refactor Ci::PipelineEnums for readability
6f1a6640 - Add environment auto stop worker
33b842f4 - docs/ci: fix typesetting of "Node.js"
790891fc - API: Support list repository commits with order
9f543675 - Add spec for order parameter
d87e813c - Fix autocomplete limitation bug
6f738552 - Update naming validation for Conan recipes
bb3c3843 - Updated file for jest testing
574aed28 - Cleanup mock_data file after rebase with master
5b5de2c7 - Fix a typo
103e3dfa - Merge branch 'leipert-fix-user-label-bug' into 'master'
91236217 - Update CHANGELOG-EE.md for 12.6.7-ee
e29f7fe0 - Update CHANGELOG.md for 12.6.7
db800a16 - Merge branch 'tr-fix-error-tracking-hmr' into 'master'
6cb9bca8 - Introduce new service to block user
9785b85e - Merge branch '251-record-user-block-audit-event' into 'master'
f209cd65 - ContainerExpirationPolicies run without user
49b9944c - Merge branch '15398-admin-user-for-policies' into 'master'
ad1cda55 - Merge branch 'bw-board-list-by-id' into 'master'
3dd6f36a - Speed up expanding/collapsing diff files
a644e7d0 - Merge branch 'psi-diff-dedupe' into 'master'
8b84a926 - Compare diff against HEAD if diff_head is passed
c58c97cc - Merge branch...
baf2eb66 - Merge branch '202095-net-new-saas-trial-sign-ups-are-asked-duplicate-questions' into 'master'
6f9e1118 - Add break clause to log cursor event processing
4110e5bd - Refactor CiBadgeLink and project show page
66a374df - Merge branch 'qa-fix-create-and-process-pipeline-spec' into 'master'
0ba60888 - Merge branch '37335-conan-name-validation-fixes' into 'master'
4ad55296 - Merge branch '199984-document-new-time-picker-in-logs-page-changes-and-dashboard' into 'master'
48a05150 - Added section about targeting elements in FE tests
f46fc221 - Merge branch 'docs-add-how-to-target-elements-to-fe-testing-doc' into 'master'
a1660302 - Migrate underscore in the contributors folder
4d93319f - Store mentions in after_save callback
e523b67a - Treat registry as SSOT for LFS
8b5f1fc2 - Merge branch 'mk/registry-is-ssot-for-lfs' into 'master'
83821cba - Revert "Merge branch 'leipert-fix-user-label-bug' into 'master'"
04d8f2f0 - Merge branch 'qa-fix-transient-geo-log-cursor-error' into 'master'
2e6120dc - Update ajax_variable_list.js to use lodash escape
0ee1313d - Load 10mb of a diff instead of 100kb
af9f589f - Avoid double encoding of import url credentials
01f8766c - Separate access classes into own class files
7f4e4d28 - Merge branch 'refactoring-entities-file-15' into 'master'
e65daf20 - Separate JobRequest entities into own class files
d657d06c - Merge branch 'refactoring-entities-file-26' into 'master'
d40eaa58 - Separate page domain entities into own class files
417747c9 - Merge branch 'refactoring-entities-file-28' into 'master'
b4e16cda - Separate environment entities in class files
2e110b2f - Merge branch 'refactoring-entities-file-23' into 'master'
422c7fa6 - Merge branch 'revert-103e3dfa' into 'master'
35ac1474 - Separate badge entities into own class files
d887cda0 - Merge branch 'refactoring-entities-file-30' into 'master'
f519ddbf - Separate cluster entities into own class files
883c82d9 - Merge branch 'refactoring-entities-file-32' into 'master'
e2eff013 - Quarantine create snippet test
c8bf1bc0 - Refactor WebIDE error message to use GlAlert
0357dce1 - Merge branch '36854-webide-use-gl-alert' into 'master'
14990b7b - Merge branch 'growth-60-hide-progress-bar-for-existing-users' into 'master'
1cc0d426 - Merge branch 'id-fix-viewing-diff-of-a-large-blob' into 'master'
2ba01b6a - Replace underscore with lodash for ./app/assets/javascripts/serverless
c8e227ec - Merge branch 'resolve_gitlab_issue_196651' into 'master'
6963a1d7 - Merge remote-tracking branch 'security/master'
9c918f02 - Merge branch '197371-docs-for-resolve-sentry-issue-close' into 'master'
af6762f7 - Refactor merge requests api requests specs
70d04e11 - Merge branch '200018-refactor-api-merge-reques-specsts' into 'master'
8e95b4dc - Merge remote-tracking branch 'dev/master'
505c18c8 - Replace underscore with lodash in app/assets/javascripts/pipeline
57acb8eb - Merge branch...
30f3844f - Merge branch 'kborges/avoid-double-credential-encoding' into 'master'
3d471c0d - Adding a basic fix and a resulting spec
0daeab64 - Making the spec look for the correct case instead
ec6ed9f3 - Updating the string methods used
e67589a5 - Adding a changelog that I forgot
d719c3cf - Merge branch...
a3f1bde4 - Merge branch '35671-api-repository-commits-with-order' into 'master'
6e99b19e - Merge branch '205511-quarantine-create-snippet-test' into 'master'
7c115957 - Update broken links to Cloud Run for Anthos documentation
409a8a00 - Merge branch 'sy-fix-memoization-leaks' into 'master'
e8d50ea9 - Merge branch 'eread/fix-tier-rendering' into 'master'
33e38761 - Fix: [Geo]Replicating objects in object storage schedules removal
048eaa02 - Merge branch...
afde97a1 - Merge branch...
d8a00988 - Merge branch 'id-master-head-diff' into 'master'
98436d99 - Create operations_strategies and operations_scopes tables
470e298a - Merge branch 'feature-flag-strategy-scope-tables' into 'master'
b8829383 - Add feature gate filters for users
b8bb871f - Merge branch 'aa-feature-gate-filter' into 'master'
956d85c5 - Add docs on the self-service framework
2e34b70f - Merge branch 'tc-geo-dsl-docs' into 'master'
3e6e8a55 - Merge branch 'add-cleanup-worker-for-auto-stopped-environment' into 'master'
43c4f6c9 - Merge branch 'trigger-store-mentions-in-after-save-callback' into 'master'
f3687d1d - Add migration and spec
70eeffd1 - Merge branch 'migration-save-instance-administrators-id' into 'master'
36e5ea05 - Merge branch 'ali/fix-cloud-run-doc-links' into 'master'
2f507e78 - Merge branch 'increase-experimentation-ratio' into 'master'
6baefb5d - Fix broken note block
ada96883 - Merge branch 'aqualls-fix-note-prometheus-docs' into 'master'
028c520d - Fix typos in Contributor and Development Docs
ea4052ab - Fix typos in AutoDevOps Docs
86333738 - Merge branch 'docs-typo-node' into 'master'
d30b0802 - Fix typos in Web Application Firewall Docs
8301bed3 - Merge branch 'docs-runner-services-flag' into 'master'
3691ff93 - Fix typo in Migrating from Jenkins docs
b4afe0f9 - Remove design management spec from quarantine
57a9d8ec - Merge branch 'auto_devops_test_rules' into 'master'
54648d43 - Merge branch 'doc-autodevops' into 'master'
389f8c96 - Merge branch 'doc-jenkins' into 'master'
6ade65a3 - Add group identification headers to epic emails
dadf933e - Merge branch 'doc-development-handbook-typos' into 'master'
ddcf6f85 - Merge branch 'doc-waf' into 'master'
a5aec080 - Remove unnecessary milestone join tables
6eef8900 - Merge branch 'remove-multiple-milestone-mess' into 'master'
2a1cee28 - Merge branch 'issue_12786' into 'master'
53294dd8 - Merge branch 'scanner-doc-typo' into 'master'
b4ee92f2 - Record audit event when user is created
09adce61 - Merge branch '251-record-user-add-audit-event' into 'master'
60f63573 - Merge branch 'ml-remove-design-management-spec-from-quarantine' into 'master'
eb7dee5f - Update dependency @gitlab/ui to ^9.8.0
3e8a6aba - Update Jest snapshots
59d6603e - Docs update note for external id and kube-ctl
33816707 - Merge branch 'Docs-update-note-for-external-id-kube-ctl' into 'master'
df4c29d0 - Add vuln projs widget to instance sec dashboard
dc6f1ca9 - Merge branch '17969-isd-vulnerable-projects' into 'master'
09b7dd8f - Merge branch...
e193b928 - Merge branch '194276-move-environments_store_spec-to-jest' into 'master'
0b8a2aa7 - Merge branch 'jivanvl-replace-underscore-lodash-contributors' into 'master'
b0bd8910 - Merge branch '196663-underscore-to-lodash-app-assets-javascripts-ci_variable_list' into 'master'
02f20276 - Capture design note movements from design_overlay
23ae95ad - Merge branch 'moveable-new-comment-pin' into 'master'
a073d48a - [UPDATE] updated underscore to lodash in spec/javascripts/badges
8d62a7ae - Merge branch '196678-replace-_-with-lodash' into 'master'
8b3c1eff - Merge branch 'renovate/gitlab-packages' into 'master'
ce527675 - Change description of minutes quota
d3ee57b1 - Merge branch '198013-update-windows-shared-12.8' into 'master'
2577961c - Handle project snippet git access check
249da1dd - Update initialize method
f0f3d2b6 - Fix repository checks
2f629c26 - Revert reposiotry check
a0bcd1e7 - Add message for todo

Compare with previous version

changed target branch from fj-39176-create-project-snippet-repository to master

Hi @fjsanpedro I've made some changes. Could you review please? Thanks!

assigned to @fjsanpedro

@lulalala LGTM! Since I'm very involved in the development I might not be too objective. Let's make it go for the next round of review. I recommend @nick.thomas because we need somebody from groupsource code to take a look.

unassigned @fjsanpedro

added 1 commit

1c438887 - Remove override check

Compare with previous version

added 12 commits

1c438887...31ad7992 - 11 commits from branch master
ea40b800 - Handle project snippet git access check

Compare with previous version

Hi @gitlab-com/gl-security/appsec, please see if this should be checked by you. This is related to access check when using git to interact with snippet repositories. It is build on top of existing project repository checks. Thanks