Unprotecting a branch returning 403

Summary

On various occasions, users are unable to unprotect a branch from both UI and API. They are getting a 403 when they try to do that. In UI the unprotect branch button is disabled (greyed out) and on clicking it triggers a 403 page. From our initial investigation, it looks like the problem is related to the unprotect_access_levels setting. In both cases, it is set to 0 (means “no one can change it”). So probably we enforce this rule literally. As a result, users can lock this setting but cannot unlock it back.

Steps to reproduce

• These steps are just assumptions. I wasn't able to reproduce this but based on the tickets we have received created this:
• Import a project from GitHub with a single branch and a user that doesn't exist within GitLab
• Try to protect and unprotect a branch created by that user.

What is the current bug behavior?

In UI the unprotect branch button is disabled (greyed out) and on clicking it triggers a 403 page

What is the expected correct behavior?

An owner or maintainer should be able to unprotect a branch under all circumstances.

Relevant logs and/or screenshots

Tickets

• https://gitlab.zendesk.com/agent/tickets/244241
• https://gitlab.zendesk.com/agent/tickets/249478

cc// @sean_carroll

added typebug label

added Category:Source Code Management customer sectiondev labels

added groupsource code label

changed the description

Setting label(s) devopscreate based on groupsource code.

added devopscreate label

• These steps are just assumptions. I wasn't able to reproduce this but based on the tickets we have received created this:

@arihantar thanks for this issue. I'm marking it a priority3 severity3 as it will need some investigation to validate if it indeed a problem. Once it is validated, it will be a P2/S2.

changed milestone to %Backlog

added priority3 severity3 labels

added [deprecated] Accepting merge requests label

@sean_carroll Can you please increase priority /severity?
We have same issue on several projects after a migration of our local GitLab instance.
This is impacting, as we cannot unprotect, or modify the branch settings on who is allowed to merge or to push, and we are unable to configure code owner approval. Any change is reporting the error "Failed to update branch!"
Thanks

Thank you for the feedback @nchampci

Upgrading to a P2.

@tlinz I would say this is a priority1, except that there are probably a limited number of users who are affected by this due to their configuration.

Added to %15.2 planning

Hi @nchampci, thanks for bringing this to our attention again. However, I am sorry , the groupsource code cannot prioritize this in %15.2 as our capacity is used up with other high priority issues. I'll have to push this to %15.3 I see someone marked this as ~"Accepting merge requests". Would this be something that you would consider contributing?

Again, I am sorry I don't have better news for you.

changed milestone to %15.2

added priority2 severity2 labels and removed priority3 severity3 labels

added backend label

changed milestone to %15.3

mentioned in issue create-stage#13017 (closed)

mentioned in issue create-stage#13025 (closed)

mentioned in epic &6592 (closed)

@sean_carroll and @tlinz,

This groupsource code bug has at most 25% of the SLO duration remaining and is ~"approaching-SLO" breach. Please consider taking action before this becomes a ~"missed-SLO" in 14 days (2022-07-24).

added SLONear Miss label

added SLOMissed label

changed milestone to %15.4

added missed:15.3 label

mentioned in issue create-stage#13039 (closed)

changed milestone to %15.5

@a_mcdonald @tlinz

I don't think this issue is ready to be assigned to an engineer as it cannot be reproduced.

These steps are just assumptions. I wasn't able to reproduce this but based on the tickets we have received created this:

Moving to %Backlog and adding workflowneeds issue review until it can be reproduced. It could be scheduled as a spike if there is some context / evidence of the problem or multiple customer reports.

Adding this as a spike for %15.8 (capacity permitting) due to recent customer reports

changed milestone to %Backlog

added workflowneeds issue review label

We have another report from a customer internal ZD link

• Through the UI, project owners and admins cannot unprotect a branch. They receive 403 error stating "You don't have the permission to access this page. Please contact your GitLab administrator to get permission."

• GitLab admins have no permission to unprotect the branch either.

• They have to force-reset the unprotect_access_level for a branch through direct console access to GitLab instance.

This can be replicated by using the API to set the unprotect_access_level to 0. In fact, we found out that it was accidentally set via API to 0 in some of our users projects.

For example:

https://gitlab.example.com/api/v4/projects/5/protected_branches?name=*-stable&push_access_level=30&merge_access_level=30&unprotect_access_level=0

However, we still believe that admins shouldn't have the option be locked out of unprotecting a branch via UI or API and this may be a personal opinion, but owners shouldn't be locked out of unprotecting a branch of a project that they own either.

Premium Self-managed customer wants this bug to be fixed. "We use API to set protected branches, we have automation around it. (gitlabform ) Because of that bug we are not able to manage that particular part(protected branches) of repositories. Workaround : disable that particular repo where it cause problems from automation and click it manually from UI"

changed milestone to %15.8

added spike label

changed the description

set weight to 2

Moving to %15.9

changed milestone to %15.9

mentioned in epic &9567 (closed)

mentioned in issue create-stage#13111 (closed)

added Deliverable label

assigned to @ghinfey

@ghinfey this is assigned to you as a %15.9 Deliverable

Hi @sean_carroll / @igor.drozdov / @a_mcdonald,

Do I understand well that fixing this bug would require a breaking change? After all if people have set unprotect_access_levels = 0 they may have done this intentionally, so that no one can unprotect it.

@ghinfey would this be a breaking change ?

@tlinz. Yes that would be my understanding, tho it would seem like an unlikely scenario.

Maybe you can check in the data if anyone has set this, @ghinfey.

Thanks @tlinz I'll look into that. However it wont tell us if the intention was to have no one able to unprotect a branch. And we do know that at least some users are using unprotect_access_levels = 0.

true

added workflowready for development label and removed workflowneeds issue review label

Setting health status to on track as the milestone has just begun.

Issue participants are welcome to override this by setting the health status to another value.

changed health status to on track

removed workflowready for development label

added workflowin dev label

mentioned in merge request !110256 (closed)

I've been able to replicate this locally and I've added Draft: Add condition to always owner to remove ... (!110256 - closed) which would specify that the project owner could always unprotect the branch.

@tlinz would that be suitable behaviour from a product perspective?

Hi @ghinfey, I think specifying on the application side "that the project owner could always unprotect the branch" may be a breaking change, depending on how you mean "always".

Since you have replicated this, how does it actionally happen that "In both cases, it is set to 0". I would assume that when I don't specify the unprotect_access_levels when protecting a branch via the API or when I simply use the UI where it can't be specified it should default to unprotect_access_levels = 40, see https://docs.gitlab.com/ee/api/protected_branches.html#protect-repository-branches.

The default (Maintainer access) is set by this before callback, which ensures that any user accessing the api is a maintainer. So in the event that unprotect_access_level is not specified, a default of 40 is not created, rather the API will forbid access to any user with a role below Maintainer.

When a unprotect_access_level = 0 (No Access) is defined for a protected_branch, no user regardless of their role, is allowed to make a change to the protection level of the branch. Its evaluated here.

Interestingly, it also means it is pointless to have unprotect_access_levels = 30(Developer access), as a developer is not authorised to use this API or view project settings.

@tlinz I think the question is what we would consider to be ideal behaviour here. Currently No Access means exactly that (as the description above states, we enforce this rule literally).

Similar to what is mentioned in the description, my proposal is to add an exception to 'No Access', whereby a project owner can always unprotect a branch. Let me revert to the team to get more opinions on whether that would be a breaking change.

Hey @igor.drozdov, I'm wondering if I can ask for your advice on the above?

The protected_branches API allows users to set unprotect_access_level to no access. Then as no access suggests, no user is able to remove it.

Do you think it would be a breaking change to allow the project owner to do it?

@ghinfey, I think rather that chaning the meaning of No Access, we should change that:

So in the event that unprotect_access_level is not specified, a default of 40 is not created, rather the API will forbid access to any user with a role below Maintainer.

I am curios what @igor.drozdov thinks.

@ghinfey @tlinz sorry, I haven't managed to have a look today, will do it tomorrow

@ghinfey @tlinz some thinking out loud for you to consider but I don't have a proposal yet:

Do you think it would be a breaking change to allow the project owner to do it?

@ghinfey I believe @tlinz wonders whether there are users that actually rely on this bug behavior and whether fixing it has any security implications I think it's unlikely because as far as I know this setting can be set/changed via UI only (but correct me if I'm wrong) and if it's actually set to No access it can be changed only via direct access to rails-console/database which is not an intuitive UX

For push/merge rules it makes sense to enforce this rule literally because it restricts accidental push to the default branch for example, even for admins and project owners. But for the unprotect rule, I can't think of any application of No access policy.

Interestingly, it also means it is pointless to have unprotect_access_levels = 30(Developer access), as a developer is not authorised to use this API or view project settings.

Yes, that's an interesting observation indeed. Only a maintainer+ can change a protected branch, so I wonder if we can narrow this setting to two options [maintainer and owner]. Hm, but at the same time, maintainers will be able to lock themselves out if they set this option to the owner.

Thanks @igor.drozdov

I wonder if we can narrow this setting to two options [maintainer and owner]

I think this could be a good option. However, would a setting of Maintainer also be redundant? As that is the default.

Could we fix this issue by removing the No access option? Then in another issue, consider the intention of this feature? In that case could we avoid making a breaking change because as you point out:

But for the unprotect rule, I can't think of any application of No access policy.

Could we fix this issue by removing the No access option? Then in another issue, consider the intention of this feature? In that case could we avoid making a breaking change because as you point out:

It would be good to avoid a breaking change, especially as this issue does not have heavy user engagement. @ghinfey can we close this spike and create a new linked issue as a featureenhancement ?

CC @tlinz

@ghinfey @tlinz @sean_carroll maybe unrelated, but I have also noticed a suggestion to add Instance Admins role: #12776 (closed)

Could we fix this issue by removing the No access option?

I think yes. We can forbid setting No access to unprotect-access-level, but do not update the existing projects that have it already set. That won't be a breaking change in this case

whether there are users that actually rely on this bug behavior and whether fixing it has any security implication

@tlinz @sean_carroll Was it the reason why it's considered a breaking change? Or is there another one?

Was it the reason why it's considered a breaking change? Or is there another one?

@ghinfey do you recall why we marked this a breaking change?

It looks like @igor.drozdov has identified a good way forward.

Thanks @sean_carroll and @igor.drozdov,

I made the judgement that it would be a breaking change if we decided to modify 'NO ACCESS' to allow the owner or maintainer to unprotect the branch (as suggested in the issue description under expected correct behavior). My thinking was that it was feasible that some users may have intended that 'NO ACCESS' be interpreted literally.

I think yes. We can forbid setting No access to unprotect-access-level, but do not update the existing projects that have it already set. That won't be a breaking change in this case

This seems like the clearest path forward.

maybe unrelated, but I have also noticed a suggestion to add Instance Admins role:

Alternatively I think this might be a good solution. We could allow an admin to unprotect the branch when unprotect_access_levels = 'NO ACCESS'. WDYT @igor.drozdov?

My thinking was that it was feasible that some users may have intended that 'NO ACCESS' be interpreted literally.

Fair enough, thanks for the context. In light of the new proposed solution we can ignore it as a breaking change now, which is good news! CC @tlinz

added missed-deliverable label

mentioned in issue gitlab-org/quality/quality-engineering/team-tasks#1641 (closed)

mentioned in issue gitlab-org/quality/quality-engineering/team-tasks#1644 (closed)

This issue is scheduled for completion in this milestone but is in an early development stage. Changing health status to 'at risk'.

Issue participants are welcome to override this by setting the health status to another value.

changed health status to at risk

@ghinfey this was scheduled as a spike in %15.9.

What is the outcome? Should we schedule this issue for development work in %15.10 and if so what weight would be needed?

Hi @sean_carroll

After discussion above the best way forward is to forbid setting No access to unprotect-access-level, but not update the existing projects that have it already set. This will not be a breaking change.

I will create a new issue for this. Also I have done some development toward this but it wont be finished by this milestone.

I will create a new issue for this. Also I have done some development toward this but it wont be finished by this milestone.

Thank you @ghinfey, that's fine. You can link any draft MR to the new issue. Once it's created you can also link the new issue to this one and close it.

New issue for removal of NO_ACCESS: #391693 (closed)

mentioned in issue #391693 (closed)

closed

marked this issue as related to #391693 (closed)

Outcome of this spike is Forbid setting NO_ACCESS to unprotect-access-le... (#391693 - closed)

@ghinfey I have marked this workflowcomplete

added workflowcomplete label and removed workflowin dev label