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

Never add labels not from this repository or organisation and remove org labels on transfer #14928

Merged

Conversation

zeripath
Copy link
Contributor

@zeripath zeripath commented Mar 8, 2021

Prevent the addition of labels from outside of the repository or
organisation and remove organisation labels on transfer.

Related #14908
Related #14912

Signed-off-by: Andrew Thornton art27@cantab.net

…org labels on transfer

Prevent the addition of labels from outside of the repository or
organisation and remove organisation labels on transfer.

Related go-gitea#14908

Signed-off-by: Andrew Thornton <art27@cantab.net>
@zeripath zeripath added this to the 1.14.0 milestone Mar 8, 2021
@zeripath
Copy link
Contributor Author

zeripath commented Mar 8, 2021

This is an extract from #14912 without the broken migration and consistency check - which should be fixed in that or another PR.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Mar 8, 2021
@zeripath
Copy link
Contributor Author

zeripath commented Mar 8, 2021

Damn! This still has the broken query.

zeripath and others added 4 commits March 8, 2021 17:29
Signed-off-by: Andrew Thornton <art27@cantab.net>
Signed-off-by: Andrew Thornton <art27@cantab.net>
Signed-off-by: Andrew Thornton <art27@cantab.net>
@6543

This comment has been minimized.

Signed-off-by: Andrew Thornton <art27@cantab.net>
…m:zeripath/gitea into fix-14908-labels-not-in-repository-part-1
Signed-off-by: Andrew Thornton <art27@cantab.net>
Signed-off-by: Andrew Thornton <art27@cantab.net>
Signed-off-by: Andrew Thornton <art27@cantab.net>
@zeripath
Copy link
Contributor Author

zeripath commented Mar 8, 2021

finally fixed the damned SQL!

@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 Mar 8, 2021
@lunny
Copy link
Member

lunny commented Mar 9, 2021

How about to create a new function named

(i *Issue) CanAddLabel(l *Label) bool {
     switch {
        case l.RepoID > 0:
           return l.RepoID == issue.RepoID
        case l.OrgID > 0:
           return l.OrgID == issue.Owner.ID
        default:
           return false
     }
}

@zeripath
Copy link
Contributor Author

zeripath commented Mar 9, 2021

That's not going to improve the complexity of repo_transfer.go.

It would also have to be:

func (issue *Issue) isValidLabel(e Engine, label *Label) (bool, error) {
	if err := issue.loadRepo(e); err != nil {
		return false, err
	}

	return (label.RepoID == issue.RepoID || label.OrgID == issue.Repo.OwnerID), nil
}

// IsValidLabel returns true if the label is a valid label for this issue.
func (issue *Issue) IsValidLabel(label *Label) (bool, error) {
	return issue.isValidLabel(x, label)
}

i.e. it would have to return an error which would significantly increase the complexity of ReplaceLabels and newIssueLabels

@zeripath
Copy link
Contributor Author

zeripath commented Mar 9, 2021

We can't check hasLabel in there because it would still require duplication in the code for replace labels.

@zeripath
Copy link
Contributor Author

Aha the test failure was a real failure - the others were passing in error!

@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 Mar 12, 2021
@6543
Copy link
Member

6543 commented Mar 12, 2021

🚀

@6543 6543 merged commit 42b9b46 into go-gitea:master Mar 12, 2021
@zeripath zeripath deleted the fix-14908-labels-not-in-repository-part-1 branch March 12, 2021 20:42
@go-gitea go-gitea locked and limited conversation to collaborators May 13, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. type/bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants