Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Don't require team membership for team search results #21174

Closed
wants to merge 4 commits into from

Conversation

noerw
Copy link
Member

@noerw noerw commented Sep 15, 2022

Another regression from #18518.

Note that tests for other routes expect non-team-members not to have access to it:

But I'd argue this assumption breaks the quite valid use-case of searching for teams you are not a member of.
Also the /api/v1/orgs/{org}/teams endpoint (available to org members) will happily list all existing teams, so there is some inconsistency here already. I'd say you don't need to be team member to see details, as the information gained is not exactly sensitive.

related discussion for permissions in the frontend:

@noerw noerw added type/bug modifies/api This PR adds API routes or modifies them backport/v1.17 labels Sep 15, 2022
@noerw noerw added this to the 1.18.0 milestone Sep 15, 2022
@GiteaBot GiteaBot added the lgtm/need 1 This PR needs approval from one additional maintainer to be merged. label Sep 16, 2022
@lunny
Copy link
Member

lunny commented Sep 16, 2022

CI failure is related.

@noerw noerw requested a review from Gusted September 16, 2022 09:38
@Gusted
Copy link
Contributor

Gusted commented Sep 17, 2022

as the information gained is not exactly sensitive.

		apiTeams[i] = &api.Team{
			ID:                      teams[i].ID,
			Name:                    teams[i].Name,
			Description:             teams[i].Description,
			IncludesAllRepositories: teams[i].IncludesAllRepositories,
			CanCreateOrgRepo:        teams[i].CanCreateOrgRepo,
			Permission:              teams[i].AccessMode.String(),
			Units:                   teams[i].GetUnitNames(),
			UnitsMap:                teams[i].GetUnitsMap(),
		}

Which information is exactly needed, maybe we could limit it to just the necessary fields? I do find this to be quite "senstive" as it laids out the whole team structure in a organization.

@noerw
Copy link
Member Author

noerw commented Sep 22, 2022

Which information is exactly needed, maybe we could limit it to just the necessary fields?

I want to know which teams exist with what purpose, so I know what access to request. So .Name and .Description would be enough for the transparency & fast communication use case I have in mind.

I do find this to be quite "senstive" as it laids out the whole team structure in a organization.

Under what premise is this a bad thing? I can see it as a social engineering attack vector, but that's about it.
If you need to wall information off coworkers for some reason, gitea provides you with tools already: just create another private gitea org.

My main goal here is to make the API permissions consistent, I don't see a strictly better option.
Philosophically I strongly prefer to make existing teams visible to all org members (in the spirit of transparency, accountability, flat hierarchies, and short communication paths).
That said, this decision is of course not about my preferences, but should accommodate the needs of most gitea users. I'm arguing that gitea already provides tools to wall information off others (private orgs), so gitea would be enhanced by also enabling users that prefer less secrecy.

@Gusted
Copy link
Contributor

Gusted commented Oct 9, 2022

I'm sorry @noerw for the late reaction, I'm pretty sure I was misunderstanding this code and situation. Limiting the return results to Name + Description is fine IMO.

@lunny
Copy link
Member

lunny commented Oct 23, 2022

replaced by #20844

@lunny lunny closed this Oct 23, 2022
@lunny lunny removed this from the 1.18.0 milestone Oct 23, 2022
@go-gitea go-gitea locked and limited conversation to collaborators May 3, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/need 1 This PR needs approval from one additional maintainer to be merged. modifies/api This PR adds API routes or modifies them type/bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants