Use PipelinesFinder as a single source of Pipelines in MergeRequestsController

Everyone can contribute. Help move this issue forward while earning points, leveling up and collecting rewards.

• Close this issue

Summary

The MergeRequestsController has multiple ways to fetch Pipelines to be rendered alongside merge requests, and we've had to spend non-trivial time patching up the permissions here to make sure Pipeline data isn't accidentally leaked via merge requests with different permissions.

We should always query pipelines through PipelinesFinder, even for something as simple as @merge_request.last_pipeline. Because the permissions can be disparate, We make a properly permission-scoped search for a Pipeline and ass the merge request as search criteria.

Improvements

• Lower complexity both cognitively (permission design) and technical (fewer permissions checks)
• Fewer before_action callbacks in the controller.

Risks

• If we're not careful or don't fully understand how PipelinesFinder works, we might block access to some Pipelines that should be accessible.

I've considered the possibility that people are using incorrectly implemented permissions checks and "fixing" it will break their workflow, but the user-facing fixes have already been shipped, and this change shouldn't change any behavior.

Involved components

• Replace all non-finder Pipeline queries in app/controllers/projects/merge_requests_controller.rb
• Remove #set_pipeline_variables from app/controllers/projects/merge_requests/application_controller.rb

Optional: Intended side effects

None visible

Optional: Missing test coverage

We've recently added tests along with security fixes, so I think those should cover this refactoring.