Skip to content

Conversation

lynxplay
Copy link
Contributor

Commit 6a97ab0 reworked team permission application. The introduced logic overrode the unitModes for every team a user is in, max(...) the current value and the team value together.

The logic completely fails in case the team does not have a unit for the specific unit type defined, in which case the logic inserted the minimumVisibility, overriding any previous aggregation of access modes for the unit.

This is resolved by simply always merging the unit access mode of the team as it will simply default to None in case the team does not have a permission defined for the unit, which will be swallowed by the max(..) call in favour of the previous aggregated permission.

Commit 6a97ab0 reworked team permission
application. The introduced logic overrode the unitModes for *every*
team a user is in, max(...) the current value and the team value
together.

The logic completely fails in case the team does not have a unit for the
specific unit type defined, in which case the logic inserted the
minimumVisibility, overriding any previous aggregation of access modes
for the unit.

This is resolved by simply always merging the unit access mode of the
team as it will simply default to None in case the team does not have a
permission defined for the unit, which will be swallowed by the max(..)
call in favour of the previous aggregated permission.
@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Sep 16, 2025
@github-actions github-actions bot added the modifies/go Pull requests that update Go code label Sep 16, 2025
@lynxplay
Copy link
Contributor Author

lynxplay commented Sep 16, 2025

For further description of the faulty code, imagine the following run of a collaborator through that method

// Collaborators on organization
// EXAMPLE
// The user is a collaborator, e.g. with write permissions.
// We hence give them write permissions for every unit in the repo.
// The map is now filled with AccessWrite.
if isCollaborator {
	for _, u := range repo.Units {
		perm.unitsMode[u.Type] = perm.AccessMode
	}
}

// if user in an owner team
// EXAMPLE
// This is a complete noop in this case, the user does not have admin access on a team.
for _, team := range teams {
	if team.HasAdminAccess() {
		perm.AccessMode = perm_model.AccessModeOwner
		perm.unitsMode = nil
		return perm, nil
	}
}

for _, u := range repo.Units {
	for _, team := range teams {
              // EXAMPLE
	       // We iterate the users team now. We find a team and query it for e.g. the Code unit.
	       // The team we are about to query however does not have access modes configured for the code unit
		unitAccessMode := minAccessMode
		// THIS if now does not run. unitAccessMode is minAccessMode which itself is None or Read at best. Definitely not Write like the collaborator should have
	        if teamMode, exist := team.UnitAccessModeEx(ctx, u.Type); exist {
		        unitAccessMode = max(perm.unitsMode[u.Type], unitAccessMode, teamMode)
	        }
	        perm.unitsMode[u.Type] = unitAccessMode // We completely override the previous unitMode without any regard for its value. We set it to minAccessMode and move on. This is makes this logic faulty in general but also dependent on the order the teams are iterated in as later teams might again bump the unit mode or fall it back down to min access level.
	}
}

@lunny
Copy link
Member

lunny commented Sep 16, 2025

Could you add some tests for that?

A small set of unit tests to cover merging behaviour between
collaborator and teams.
@lynxplay
Copy link
Contributor Author

I pushed some tests, I am very inexperienced with gitea's unittest layout, specifically the split into fixtures and in-code definitions. Would appreciate feedback.

@lunny lunny requested a review from wxiaoguang September 17, 2025 14:43
@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Sep 17, 2025
@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Sep 17, 2025
@lunny lunny added type/bug skip-changelog This PR is irrelevant for the (next) changelog, for example bug fixes for unreleased features. labels Sep 17, 2025
@lunny lunny added this to the 1.25.0 milestone Sep 17, 2025
@lunny lunny added the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Sep 17, 2025
@lunny lunny merged commit 2f3da6d into go-gitea:main Sep 17, 2025
26 checks passed
@GiteaBot GiteaBot removed the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Sep 17, 2025
zjjhot added a commit to zjjhot/gitea that referenced this pull request Sep 18, 2025
* giteaofficial/main:
  Clean up npm dependencies (go-gitea#35508)
  Correctly override user unitmodes (go-gitea#35501)
  [skip ci] Updated translations via Crowdin
  Enable more markdown paste features in textarea editor (go-gitea#35494)
  Move git command to git/gitcmd (go-gitea#35483)
  Exposing TimeEstimate field  in the API (go-gitea#35475)
  Clean up npm dependencies (go-gitea#35484)
@lynxplay lynxplay deleted the bugfix/team-invalidation-of-unit-modes branch September 19, 2025 09:35
@wxiaoguang wxiaoguang added the backport/v1.24 This PR should be backported to Gitea 1.24 label Sep 22, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport/v1.24 This PR should be backported to Gitea 1.24 lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. modifies/go Pull requests that update Go code skip-changelog This PR is irrelevant for the (next) changelog, for example bug fixes for unreleased features. type/bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants