Remove redundant permission checks for GraphQL job type

What does this MR do and why?

Related to #330707 (closed)

Reading a build requires a project-level permission, so we don't need to check read permissions for each build.

In this MR we remove the read_commit_status authorization from Ci::JobType because its callers already do the check.

Side info

Ci::JobType is used in;

• Types::Terraform::StateVersionType; the authorization was just added in this MR.
• Types::Ci::Config::GroupType; the authorization is provided through Resolvers::Ci::ConfigResolver -> Types::Ci::Config::ConfigType -> Types::Ci::Config::StageType.
• Types::Ci::StageType; the authorization is provided in itself with authorize :read_commit_status.
• Types::Ci::PipelineType; the authorization is provided in itself with authorize :read_pipeline.
• Types::Ci::GroupType; the authorization is provided through Types::Ci::StageType.
• Types::ProjectType; the authorization is provided in the field declaration with authorize: :read_commit_status.
• Mutations::Ci::Job::Unschedule; the authorization is provided in itself with authorize :update_build.
• Mutations::Ci::Job::Retry; the authorization is provided in itself with authorize :update_build.
• Mutations::Ci::Job::Play; the authorization is provided in itself with authorize :update_build.
• Mutations::Ci::Job::Cancel; the authorization is provided in itself with authorize :update_build.

Screenshots or screen recordings

Before this MR:

After this MR:

How to set up and validate locally

1. Run the development env with RAILS_PROFILE=true.
2. Create a pipeline with some jobs.
3. Open GraphiQL http://gdk.test:3000/-/graphql-explorer.
4. Trace the outputs via Sherlock.
5. Run the query with variables;

query getPipelineDetails($projectPath: ID!, $iid: ID!) {
  project(fullPath: $projectPath) {
    pipeline(iid: $iid) {
      stages {
        nodes {
          groups {
            nodes {
              jobs {
                nodes {
                  status: detailedStatus {
                    icon
                    hasDetails
                  }
                }
              }
            }
          }
        }
      }
    }
  }
}

MR acceptance checklist

These checklists encourage us to confirm any changes have been analyzed to reduce risks in quality, performance, reliability, security, and maintainability.

Quality

• Quality checklist confirmed

1. I have self-reviewed this MR per code review guidelines.
2. For the code that that this change impacts, I believe that the automated tests (Testing Guide) validate functionality that is highly important to users (including consideration of all test levels). If the existing automated tests do not cover this functionality, I have added the necessary additional tests or I have added an issue to describe the automation testing gap and linked it to this MR.
3. I have considered the technical aspects of the impact of this change on both gitlab.com hosted customers and self-hosted customers.
4. I have considered the impact of this change on the front-end, back-end, and database portions of the system where appropriate and applied frontend, backend and database labels accordingly.
5. I have tested this MR in all supported browsers, or determiend that this testing is not needed.
6. I have confirmed that this change is backwards compatible across updates, or I have decided that this does not apply.
7. I have properly separated EE content from FOSS, or this MR is FOSS only. (Where should EE code go?)
8. If I am introducing a new expectation for existing data, I have confirmed that existing data meets this expectation or I have made this expectation optional rather than required.

Performance, reliability, and availability

• Performance, reliability, and availability checklist confirmed

1. I am confident that this MR does not harm performance, or I have asked a reviewer to help assess the performance impact. (Merge request performance guidelines)
2. I have added information for database reviewers in the MR description, or I have decided that it is unnecessary. (Does this MR have database-related changes?)
3. I have considered the availability and reliability risks of this change. I have also considered the scalability risk based on future predicted growth
4. I have considered the performance, reliability and availability impacts of this change on large customers who may have significantly more data than the average customer.

Documentation

• Documentation checklist confirmed

1. I have included changelog trailers, or I have decided that they are not needed. (Does this MR need a changelog?)
2. I have added/updated documentation, or I have decided that documentation changes are not needed for this MR. (Is documentation required?)

Security

• Security checklist confirmed

1. I have confirmed that if this MR contains changes to processing or storing of credentials or tokens, authorization, and authentication methods, or other items described in the security review guidelines, I have added the label security and I have @-mentioned @gitlab-com/gl-security/appsec.

Deployment

• Deployment checklist confirmed

1. I have considered using a feature flag for this change because the change may be high risk. If I decided to use a feature flag, I plan to test the change in staging before I test it in production, and I have considered rolling it out to a subset of production customers before doing rolling it out to all customers. When to use a feature flag
2. I have informed the Infrastructure department of a default setting or new setting change per definition of done, or decided that this is not needed.

changed milestone to %14.3

added All SaaS Category:Continuous Integration FY22Q3 OKR-FY24Q2 SLOMissed SLONear Miss automation:ml backend bugperformance candidate14.3 customer devopsverify grouppipeline authoring priority2 quad-planningcomplete-action sectionops severity2 typebug workflowin dev + 1 deleted label

assigned to @furkanayhan

added typemaintenance label

added typefeature label

Reviewer roulette

Changes that require review have been detected!

Please refer to the table below for assigning reviewers and maintainers suggested by Danger in the specified category:

Category	Reviewer	Maintainer
backend	Pedro Pombeiro (@pedropombeiro) (UTC+2, 1 hour behind @furkanayhan)	Mark Chao (@lulalala) (UTC+8, 5 hours ahead of @furkanayhan)

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.

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

Generated by Danger

changed the description

removed typefeature label

marked the checklist item Quality checklist confirmed as completed

marked the checklist item Performance, reliability, and availability checklist confirmed as completed

added 1 commit

• edf8a6dd - Remove redundant permission checks for GraphQL job type

Compare with previous version

changed the description

added 283 commits

• edf8a6dd...a1d583e1 - 282 commits from branch master
• f2d536ed - Remove redundant permission checks for GraphQL job type

Compare with previous version

marked the checklist item Security checklist confirmed as completed

added security label

@gitlab-com/gl-security/appsec I've made small changes on the read_build permission of jobs. Can you please review the changes? security

@matteeyah Can you please review this? backend

requested review from @matteeyah

Nice job @furkanayhan! I left two minor comments.

removed review request for @matteeyah

changed the description

added 2 commits

• 436a795f - Remove redundant permission checks for GraphQL job type
• 7c1ef0f3 - Add tests for the new permission

Compare with previous version

requested review from @matteeyah

mentioned in issue #330707 (closed)

approved this merge request

removed review request for @matteeyah

@matteeyah, thanks for approving this merge request.

This is the first time the merge request is approved. To ensure full test coverage, a new pipeline has been started.

For more info, please refer to the following links:

• rspec minimal jobs
• jest minimal jobs
• merging a merge request.

@alexkalderimis I see you have lots of commits in app/graphql/types/ci/job_type.rb so I thought it would be great if you do the final review of this. Can you, please?

requested review from @alexkalderimis

approved this merge request

Thanks @furkanayhan - this is a clear and straightforward change. It makes sense to use field authorization when object authorization is too expensive.

However, since there are multiple ways to access jobs (sadly it isn't just a one path field), I'd like to see the list of all places, we need to guard, so we can verify that all access is checke correctly.

Back to you for discussion and notes @furkanayhan

removed review request for @alexkalderimis

changed the description

requested review from @alexkalderimis

approved this merge request

@furkanayhan - I have approved this MR with caveats (see comments above).

I think the more permanent solution is to fix the underlying performance issues with the current authorization checks, but we can iterate to that goal.

resolved all threads

enabled an automatic merge when the pipeline for 7818fd52 succeeds

merged

mentioned in commit b783aac0

added workflowstaging-canary label and removed workflowin dev label

added workflowstaging label and removed workflowstaging-canary label

added workflowcanary label and removed workflowstaging label

added workflowproduction label and removed workflowcanary label

This will help us to check later:

75th and 99th percentiles of duration and db count: https://log.gprd.gitlab.net/goto/b08207d25d2259903d5140b938f612e7

Last 7 days:

(https://log.gprd.gitlab.net/goto/7be1d7260b643a9d7ab97e6a6a2b9c99)

added releasedcandidate label

added releasedpublished label and removed releasedcandidate label

mentioned in merge request kubitus-project/kubitus-installer!236 (merged)

added Category:Pipeline Composition label

mentioned in issue #410474 (closed)