Block pipelines on protected branches

@ayufan @dimitrieh I'm not sure what the right thing to do here is. Blocking manual pipeline runs on protected branches makes sense, but is there any reason to look at one of the existing exceptions like "allow to merge" or "allow to push" and see what roles/people are allowed to do those things? Should we add a third thing, "allow to run manual pipelines"? That seems excessive.

If/when we implement #24196 (closed), we might need to reconsider this. That issue assumes pipelines are only run if you already have proper permissions for protected branches, but if someone can manually trigger a pipeline on a protected branch, but the variables and runners aren't available, it will probably end badly (but at least securely).

mentioned in issue #24196 (closed)

I think we can get away without the need for an "allow to run manual pipelines" configuration, and just say that protected branches also limit manual pipeline runs to Masters. There are times when a Developer might want to run a pipeline, and the project isn't doing CD, so it's not a security issue, but it seems like such an edge case that's not worth making an option or even a checkbox.

added feature proposal label

changed milestone to %Backlog

Also related, I think we currently let people retry pipeline jobs that run on protected branches. So even if we block running a new pipeline, someone could still re-deploy old code (accidentally) and bork production. So we'd have to block all manual pipeline triggers; for entire pipelines, for job retries, etc.

mentioned in issue #21911 (moved)

changed milestone to %9.3

Maybe if "Allowed to merge" is set to Developers + Masters, then Developers should be allowed to trigger Pipelines on the protected branch too.

@grzesiek Can you describe an engineering side of that story?

added Deliverable label

assigned to @grzesiek

@zj Is there a reason why you down-voted this issue?

@grzesiek I don't see what limiting this abilities makes sense, and will limit any developer in his work. This would mean I can't restart any transient failures on master? Cascading effects seem not really thought through.

This is an interesting point, I wonder what @markpundsack thinks about it?

@grzesiek @zj If we follow the suggestion in https://gitlab.com/gitlab-org/gitlab-ce/issues/30634#note_28290464, then you'd be run pipelines as a developer.

Also, this issue is about manually running a pipeline from scratch (possibly including custom variables), not about retrying failed jobs. The latter could have different permissions.

mentioned in issue #32618 (closed)

Based on https://gitlab.com/gitlab-org/gitlab-ce/issues/32618#note_30478102, let's use the "allowed to merge" permission check rather than "allowed to push" or the blanked "master" check as originally proposed.

changed the description

mentioned in issue #32768 (closed)

added pipeline label

@grzesiek Could you describe to @godfat what needs to be done in that story? Maybe @godfat would be able to finish that?

I think that it would in simplest case just be extension for CreatePipelineService and also create additional checks in a few places.

We already have protected_ref?. So this should be easily useable already.

assigned to @godfat

@godfat If there is something I could help you with, just let me know!

Quick thought on this. Functionality-wise should be simple, the toughest part would be giving feedback to the user. Why I no longer have the privilege? We might also need to hide some UI.

AFAIK,

run manual pipelines

has already done by @grzesiek recently.

@bikebilly I am not completely sure if this is a security issue, provided that it's been like this from the beginning and people should be aware of that. And if this is a breaking feature change, I would prefer to think more thoroughly and make it more friendly rather than start failing silently.

A good example is #32618 (closed), which we broke and changed in a patch release.

I am not saying that we shouldn't just implement this, just want to mention that any breaking change should be carefully considered.

@dosuken123 I think you're referring to manual actions, not manual pipelines. Manual pipelines are in the project pipeline page, and click on "Run pipeline"

I agree with @godfat that "failing silently" is not a good candidate for MVP. This would add a significant amount of work for our support team.

@godfat Sorry, it's already described at

and perform manual actions (covered by #20261 (closed), but listed here for completeness)

Please ignore my comment.

Yes. And it seems that we don't have currently a way to visualise the reason of the failure.

So as part of this change, we should add ci_pipelines.failure_reason.

My idea is to reject during creation, create pipeline, but probably mark it as failed.

@bikebilly @ayufan I would think upon creation would be good enough, but well, we should also block from retrying failed jobs, for examples. Also cancelling, I think.

Pipeline schedule is a good question. I would feel we should only check upon creation, otherwise removing a person from the project might effectively remove/stop all the schedules, which is probably unexpected, and people would need to change the owner before removing previous owner.

mentioned in issue #32948 (moved)

While I am still thinking if there's an easy way to do this, I feel it's hard to merge this before 7th.

I think it should be handled in Controller/API level (To visualize and catch ealier), thought, alternatively creating a new guard in CreatePipelineService may work. What if target branch is protected after scheduled pipeline created. We need to add the check on PipelineScheduleWorker or so.

@dosuken123

A new guard in CreatePipelineService it is already prepared to raise an error. This error is surfaced, but not in every place.

Here are some notes before I jump into implementation.

Places we need to check

@ayufan's idea about creating the pipeline anyway but give it a reason to fail, could probably cover a lot of feedback. This could be a step forward, and we might want to set the reason why it's failing in the future anyway. However this might require UI/UX changes too, and it doesn't cover for retrying/cancelling. We're not going to fail the pipeline if someone failed to retry/cancel, and they still need to know why this is failing. We don't want to create a failed job in this case either.

Permissions we need to check

create/update_commit_status Umm... right we should think about this too, because this could potentially create a pipeline. It might not hurt, but it could be confusing
create/update_build Note that not only ProjectPolicy has this, BuildPolicy has this, too
create/update_pipeline We need to do similar stuffs as we did in BuildPolicy, duplicating the permission check in PipelinePolicy
create/update_pipeline_schedule Updating the schedule could be fine! Actually, following the same pattern as for now, we could ignore anything here, just deactivate the schedule whenever a pipeline cannot be created

@dimitrieh @bikebilly @ayufan What do you think about making protected branch/tag information more accessible?

What I am thinking here is that, we could just hide several buttons (e.g. retry, cancel) if the user has insufficient permission, but then it would not be clear why the user suddenly no longer has sufficient permission after this change.

It's also not clear a branch/tag is protected or not. If we make it clear, this pipeline/job is a protected pipeline/job, running for a protected branch/tag, then people might be more aware that they might not have sufficient permission to do this anymore.

This is not directly answering "why", but it could still be beneficial that they would be more careful for what's happening here.

We should also show this information reading a particular branch, like https://gitlab.com/gitlab-org/gitlab-ee/commits/master or https://gitlab.com/gitlab-org/gitlab-ee/tree/master

What do you think?

I think that we are going to far. If we would focus on "fixing creation first" it would already solve a bunch of problems. I do understand a point of improving retry too, but this seems to me out of scope.

But I still agree that if we start blocking pipeline creation it should be clearly visible.

Given that I don't think that it is feasible to do for %9.3, as we don't have a way to surface that information easily, as most of our views are written in Vue, and for that reason frontend'er has to be involved.

mentioned in merge request !11910 (merged)

For now I would do minimal for giving error feedback.

However I hit into a question. Could anyone answer this? @grzesiek @bikebilly @ayufan I realized that triggers might not have a corresponding owner, if they're pretty old for example, and the API is not passing the current user which makes the API call. So now I am confused which user we should be checking against.

Owner of the trigger?
Whoever uses the trigger to make the API call?

For now I implemented it as:

If there's a current user, check with it
If there's a trigger owner, check with it
If there's no users at all, only allow it if the ref is not protected (so this only breaks the old calls against protected ref)

Also, pipeline schedule would deactivate itself if the owner doesn't have the permission.

@godfat I would say that the most simple solution is just to reject pipelines triggered by ownerless trigger if a branch is protected. Relevant feedback should be presented to user so that he can take ownership over a trigger.

mentioned in commit 9984f07a

I'm not sure if schedule should deactivate, as over time user can regain permissions.

@grzesiek We decided to not reject it. See https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/11910#note_31632911

@ayufan It would already deactivate for now, so we would consider remove it entirely then. I think as long as we have proper error report, this won't be an issue. I want to deactivate it because (a) we're already doing so (b) as a way to inform people that this schedule doesn't work, instead of failing silently.

@godfat are you referring to have a protected branch/tag ...tag ? similar to "latest" ? As I think that would be good, and we do that already

@dimitrieh Yes, to be more specifically, in the below two screenshots:

^^^ 80 jobs from master (protected)

^^^ from master (protected)

So that if people cannot see Retry or Cancel, they would be more aware that they might not have sufficient permission to do so, not that the buttons are just missing.

changed milestone to %9.4

added Stretch and removed Deliverable labels

As discussed I changed this to Stretch.

@godfat so i think you are referring to, as in answer to my proposal at https://gitlab.com/gitlab-org/gitlab-ce/issues/30634#note_31637583

for pipeline list:

for pipeline view:

for job view (with new jobs sidebar from: https://gitlab.com/gitlab-org/gitlab-ce/issues/24339 ):

@dimitrieh Yes! Exactly. Thank you. This should make it less surprising that an action can't be made because the branch is protected.

@bikebilly I see that you pinged me but I don't see any mockups on the issue description. Does this have Frontend and UX?

@bikebilly @dimitrieh I think that also works for me, and my only concern is that it should be consistent everywhere we're showing the branch name. We're showing branches in a lot of places, so better to make it small, indeed. I think we should even show this in dropdown menu, for example, when we're trying to run a manual pipeline, it should also tell me which branch is protected there.

@filipa Previously we don't plan to have any frontend for this, but I would like to avoid #32618 (closed) from happening again, because I think this would certainly also break a lot of people's workflow. So if we're going forward, we should at least give users feedback on why something is not working now. However, only some of the places could report this information, and some of the places would just silently fail, without giving any information. In those cases, we would like to have some proper way to report the error. I don't think there's a mockup yet though.

I think the main places we're missing reports are:

Retry and cancel
Pipeline schedule

For retry and cancel, we could just go ahead and hide the buttons. But that would also surprise people, that's why I am thinking that perhaps showing that the branch is protected would be an easier step. At least they would know the pipeline is a bit different if the buttons are missing.

As for pipeline schedule, I don't have a good idea yet. Ideally we should have a log for people to see why a schedule is not happening, I think.

@godfat @dimitrieh I think we need both: if we keep the buttons but they don't do anything it will look like a bug, if we only remove the buttons it will look like a bug.

Cancel button is rendered if cancel_path is provided. Retry button is rendered if retry_path is provided.

I would like to keep this logic as simple as possible :), @godfat is it possible that backend does not send theses keys if the user can't retry or cancel?

As for user info, a protected tag seams the best approach to me, main reason being:

Screen readers will be able to read it better than a labeled icon

There should be minimal frontend work here. @godfat can you please ping me on slack if you need me?

@filipa Yes it should be simple to not send cancel_path or retry_path if they can't be done, but is there a way to tell the user that why they can't be done? I thought not sending them would simply look like removing the buttons?

I would ping you when there's a clear action we could take here. It's already moved to %9.4 so I think we have time to think over it.

@godfat Is there anything you need help with here? Please let me know, in the meantime I'm going to unassign myself.

unassigned @grzesiek

@grzesiek I think we're waiting for product decision for now. If we don't care about giving the error feedback, current implementation should be fine, but I strongly encourage us to have some kind of feedback before enabling this.

hi,

we have one problem with the pipelines and protected branches. In a project only masters have the right to add trigger-token's. And only masters can "take ownership" of a token. So: A trigger token will always belong to a master account (https://gitlab.com/help/ci/triggers/README.md#adding-a-new-trigger).

If i'm right your MR will limit the right to trigger a pipeline if the user is "allowed to merge". But with the current implementation of gitlab, every trigger-token belongs to a master and is allowed to merge.

So it would be great to do a "change owner" of a trigger token to some non-master account and not only a "take ownership". We currently do this with a workaround:

promote a user to master
impersonate this user
take ownership of the trigger token
demote the user to developer

But this is only a workaouround, because if you forget this ... the pipeline will always run as master.

@ulrichSchreiner If I read you correctly, if we make all developers could add their own triggers, this could be solved?

I don't know why we made that triggers are master only, but I guess after this is implemented, it makes sense to relax the permission to allow developers to make their own triggers?

@bikebilly What do you think?

@godfat Well the whole Settings are not available for a developer, and i think this is ok. I would prefer to change the owner of a trigger by a master. We have some technical accounts for such triggers, so it would be great to allow a master to set the owner of a token.

A trigger-token is not master-only but the UI-workflow leads to a situation where only master's can create and take ownership

@ulrichSchreiner Good point, so we only need to add an extra section to setup who would be the owner, is this correct? I am wondering if this would cause some weird edge cases, for example that the owner now cannot see their own stuffs, or the owner suddenly could see something they aren't supposed to see.

do you think that a mix of your proposal (when the branch name has space to get labels) and mine (padlock icon when the branch has no space) could be acceptable as a minimal change for this kind of feedback?

@bikebilly I'd rather have a consistent way of showing this information.. so let's choose visual cue to make this clear

As for what to choose. I don't think this information should perse be displayed on the branch indicator in the pipeline list. I think if we are clear on our tooltip statement of the tags presented in https://gitlab.com/gitlab-org/gitlab-ce/issues/30634#note_31836405 we should be clear.

what about for pipeline list: Pipeline runs on a protected branch

and other places: Branch is protected.

Is it possible for someone to run a pipeline on a protected branch, but which will not run (thus immediately fail) because they have insufficient rights?

Thus we need to show some sort of a failed error message?

added UX label

@dimitrieh

Is it possible for someone to run a pipeline on a protected branch, but which will not run (thus immediately fail) because they have insufficient rights?

Yes, via "Run pipeline" and API. However we could provide error feedback in both cases. What we cannot show feedback would be retrying or cancelling, which we could just hide the buttons whenever they don't have permission.

I wrote a list at https://gitlab.com/gitlab-org/gitlab-ce/issues/30634#note_31524840

What we cannot show feedback would be retrying or cancelling, which we could just hide the buttons whenever they don't have permission.

@godfat agreed here!

assigned to @dimitrieh

assigning myself for UX matters on this issue

@bikebilly

about retry and cancel actions, what about disabling the buttons and add a tooltip for them that explains why they're disabled?

That would certainly work better, just that it needs frontend then :P

If you can run the pipeline, you don't need to see the badge at all. If yo cannot, the pipeline will be marked as failed and probably you're more attracted by that instead of a badge saying protected.

Do you mean that we mark the pipeline being failed if they don't have sufficient permission? I am unsure why we should do that, if we could just avoid creating the pipeline, and saying that they don't have sufficient permission? I think the current merge request already did so.

Things we currently can't give proper feedback:

retry
cancel
play
pipeline schedule (maybe no longer an issue, I am not sure)

The reset should be fine.

@bikebilly I think it's still nice to have it, but yes if we have frontend and just tweak the button to give feedback, I don't think showing that the pipeline is protected would be too important here.

@filipa Do you still have capacity for this release? Or is there anyone else could help here? So what we're going to do is, we don't want to just hide the buttons for retrying or cancelling for a pipeline/job, we want to still show them, but in a disabled manner, and provide tooltip when mouseover it, telling them why the button is disabled.

How could we do this? If it's easy, maybe I could do it by myself :P

@godfat I can help!

I don't see any design, so to make sure what I need to do:

Disable buttons when the pipeline is protected and show a tooltip with that information.

What do I know this? Which key should I look for?

Also, do we want to show the tag protected as well or did we drop that?

@filipa Great, thanks!

We ignore protected tag in this issue/merge request, since we decided to just show disabled buttons, and that should be clear enough for this feature.

I remembered that you said that if retry_path doesn't exist, then the button won't show. What about if we also provide disabled_retry_tooltip as the message on the tooltip, then we still show the button, but in a disabled manner with that tooltip?

Or do you have any other recommendation?

@godfat

Wondering if we could keep the logic of only rendering the button if retry_path is provided instead of checking for 2 things. But I am not sure if I have a more elegant solution, to be honest.

Current implementation:

If retry_path is provided we render the button.

(In manual action, we have a playable flag for each. If this flag is false we rendered it as disabled.)

Suggestion:

I am wondering if we could follow the same pattern:

Send retry_path (it can be with the real one or an empty string)
And send also a "flag" - it is not really a flag because we need a message, it can be retry_path_disabled_message.

This would allow us to know whether to render it as disabled or not and have the message.

I suggest leaving the tooltip part out of it since it is too specific, but I guess it also works, what you prefer :)

TL;DR: If it would be possible to also send retry_path it would make the logic in the frontend a lot cleaner

@filipa Sure let's follow the same pattern, but I am not sure about the current status. Looking at the code, play_path would only be set if the job is playable. Are you saying that it's possible that play_path would be set, however playable would be false?

I could see this makes sense, and backend would have all the flexibility to decide whether we should show or disable the button.

I like retry_path_disabled_message.

Backend implementation note. Given the current implementation, we could maybe do this:

expose :retry_path, if: -> (*) { has_retry_path? }  do |pipeline|
  retry_namespace_project_pipeline_path(pipeline.project.namespace,
                                        pipeline.project,
                                        pipeline.id)
end

expose :retry_path_disabled_message, if: -> (*) { disable_retry? } do |pipeline|
  # ...
end

def has_retry_path?
  can_retry? || allowed_to_create_pipeline?
end

def disable_retry?
  !can_retry? && allowed_to_create_pipeline?
end

def allowed_to_create_pipeline?
  Ci::Pipeline.allowed_to_create?(
    request.current_user, pipeline.project, pipeline.ref)
end

This somehow looks weird because:

We don't show the button if they cannot retry
However we still show the button if at the same time they cannot create the pipeline for the given ref as well

I'll think more about how we could express the idea better, and try to remove allowed_to_create? and move the functionality into simple can?.

In order to render it and show a tooltip I would rather have the retry_path being sent with retry_path_disabled_message:

retry_path is present -> render a button
retry_path is not present -> don't render a button
retry_path_disabled_message is present & retry_path is present -> render a disabled button with the given meesage

Does it make sense?

Do you have a MR?

@filipa Yes, let's try that in https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/11910 please? Backend is not ready for this change though :P

assigned to @filipa

about retry and cancel actions, what about disabling the buttons and add a tooltip for them that explains why they're disabled?

@bikebilly we are currently not doing that throughout the interface. if the action is not available its hidden. Let's do it here similarly to create a consistent experience...

if issues arise this is a simple fix afterwards

@bikebilly @dimitrieh Here's the summary of current possible approach:

Ignore on blocking retry/cancel. This means that only pipeline creation would be protected, and developers could retry/cancel regardlessly (I think this is the current implementation)
Just hide the retry/cancel buttons. They won't know why they can no longer do this (I think this would be confusing, because this is a breaking change)
Provide some "protected" labels as a hint that they might not have the permission (in progress?)
Show disabled buttons (I think this works better but it's inconsistent UX)

@filipa Let's wait a bit for consensus. Sorry!

@bikebilly

I suppose we're only talking about feedback to users here, and that the 'protection' is enforced in any cases (it is the scope of this issue). Can you confirm?

Sorry I guess I am unclear about the scope here, because @ayufan once said: https://gitlab.com/gitlab-org/gitlab-ce/issues/30634#note_31527235

I think that we are going to far. If we would focus on "fixing creation first" it would already solve a bunch of problems. I do understand a point of improving retry too, but this seems to me out of scope.

That's why I mentioned that we could ignore retry/cancel first. However I do agree that we should also block on retry/cancel otherwise it just seems weird (or broken).

So after all, let's decide in between:

Hide the buttons for retry/cancel
Show disabled buttons for retry/cancel
- If we decided to disable buttons, we'll also need to think about when we should do this. Do we do this for all the cases? I don't actually think so. We don't have to show guests that they can't retry. So the only case I think we should show disabled buttons, would be that they usually should have the permission, but the branch/tag is protected. This logic is actually somehow weird and inconsistent, I agree with this. I might think this is just a patch to the problem, not really the answer to it.

@godfat no problem! Please ping me on slack once we reach one!

For the sanity of code I think the simpler the best. The one that we have reached seemed good to me -> https://gitlab.com/gitlab-org/gitlab-ce/issues/30634#note_33805580

@bikebilly anything wrong with that?

unassigned @filipa

assigned to @filipa

unassigned @dimitrieh and @filipa

I was not aware that we also have race with assignees!

@bikebilly Do you need a design added to the description for this? I see that @dimitrieh has provided input and feedback but that this issue is still in the UX status.

Thanks @bikebilly, much appreciated!

changed milestone to %9.5

added Deliverable and removed Stretch labels

@bikebilly Thanks for checking in. The current implementation is that if it failed to create a pipeline, we would just disable the schedule, therefore the next interval won't be reached at all.

This is the same behaviour as before, just that now it's more likely to be deactivated.

@bikebilly It's already documented. I added that this could also happen on protected branches or tags: https://gitlab.com/gitlab-org/gitlab-ce/commit/de01b862254be634a0602c6a8875cdda0538354f

closed via merge request !11910 (merged)

mentioned in commit ac948684

mentioned in commit 22e54c97

mentioned in issue #36406

mentioned in issue gitlab#6127

Block pipelines on protected branches

Description

Proposal

Links / references

Documentation blurb

Designs

Child items ...

Activity

Places we need to check

Permissions we need to check

Admin message

Admin message

Block pipelines on protected branches

Description

Proposal

Links / references

Documentation blurb

Activity

Places we need to check

Permissions we need to check