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

Add API to manage issue dependencies #17935

Merged
merged 89 commits into from
Mar 28, 2023
Merged

Add API to manage issue dependencies #17935

merged 89 commits into from
Mar 28, 2023

Conversation

qwerty287
Copy link
Contributor

@qwerty287 qwerty287 commented Dec 8, 2021

Adds API endpoints to manage issue/PR dependencies

  • GET /repos/{owner}/{repo}/issues/{index}/blocks List issues that are blocked by this issue
  • POST /repos/{owner}/{repo}/issues/{index}/blocks Block the issue given in the body by the issue in path
  • DELETE /repos/{owner}/{repo}/issues/{index}/blocks Unblock the issue given in the body by the issue in path
  • GET /repos/{owner}/{repo}/issues/{index}/dependencies List an issue's dependencies
  • POST /repos/{owner}/{repo}/issues/{index}/dependencies Create a new issue dependencies
  • DELETE /repos/{owner}/{repo}/issues/{index}/dependencies Remove an issue dependency

Closes #15393
Closes #22115

@lunny lunny added the modifies/api This PR adds API routes or modifies them label Dec 10, 2021
@lunny lunny added this to the 1.17.0 milestone Dec 10, 2021
@noerw noerw added the type/feature Completely new functionality. Can only be merged if feature freeze is not active. label Dec 23, 2021
@codecov-commenter
Copy link

codecov-commenter commented Jan 7, 2022

Codecov Report

Merging #17935 (56a916b) into main (f521e88) will decrease coverage by 0.15%.
The diff coverage is 37.79%.

@@            Coverage Diff             @@
##             main   #17935      +/-   ##
==========================================
- Coverage   47.14%   46.99%   -0.15%     
==========================================
  Files        1149     1156       +7     
  Lines      151446   153078    +1632     
==========================================
+ Hits        71397    71942     +545     
- Misses      71611    72644    +1033     
- Partials     8438     8492      +54     
Impacted Files Coverage Δ
cmd/dump.go 0.66% <0.00%> (-0.01%) ⬇️
cmd/migrate_storage.go 5.76% <0.00%> (-0.12%) ⬇️
cmd/web.go 0.00% <0.00%> (ø)
models/actions/run.go 1.64% <0.00%> (-0.08%) ⬇️
models/actions/runner.go 1.44% <ø> (ø)
models/packages/package.go 45.45% <0.00%> (-1.13%) ⬇️
models/user/search.go 77.50% <0.00%> (-6.29%) ⬇️
modules/actions/workflows.go 0.00% <0.00%> (ø)
modules/context/context.go 64.54% <0.00%> (-3.53%) ⬇️
modules/doctor/storage.go 30.65% <0.00%> (-1.29%) ⬇️
... and 44 more

... and 54 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Jan 7, 2022
Copy link
Contributor

@zeripath zeripath left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a serious security problem in this code. Not only can I use these endpoints to check if any repository exists but I can read any issue on any repository using this.

routers/api/v1/repo/issue.go Outdated Show resolved Hide resolved
routers/api/v1/repo/issue.go Outdated Show resolved Hide resolved
routers/api/v1/repo/issue.go Outdated Show resolved Hide resolved
routers/api/v1/repo/issue.go Outdated Show resolved Hide resolved
routers/api/v1/repo/issue.go Outdated Show resolved Hide resolved
routers/api/v1/repo/issue.go Outdated Show resolved Hide resolved
@qwerty287
Copy link
Contributor Author

@zeripath I added permission checks in dde1c05. Could you please review it?

@qwerty287
Copy link
Contributor Author

qwerty287 commented Mar 26, 2023

@zeripath The API returns a nil pointer if there's a hidden issue (if you can't write the repo of the original issue) because there's no repo and it tries to generate the API URL of the issue. I think it's better to just ignore these issues. I changed this in 37081b4, if you have a better solution, I can revert it. Everything else worked as expected in my tests.

@zeripath
Copy link
Contributor

@zeripath The API returns a nil pointer if there's a hidden issue (if you can't write the repo of the original issue) because there's no repo and it tries to generate the API URL of the issue. I think it's better to just ignore these issues. I changed this in 37081b4, if you have a better solution, I can revert it. Everything else worked as expected in my tests.

The issue with ignoring the issues is that it breaks the paging.

The solution would be to look again at the logs/error? I think it would be very easy to fix convert.ToAPIIssue to prevent the error.

For example (based on 44a120e):

diff --git a/services/convert/issue.go b/services/convert/issue.go
index 6651fb1d8..6d31a123b 100644
--- a/services/convert/issue.go
+++ b/services/convert/issue.go
@@ -35,8 +35,6 @@ func ToAPIIssue(ctx context.Context, issue *issues_model.Issue) *api.Issue {
 
 	apiIssue := &api.Issue{
 		ID:          issue.ID,
-		URL:         issue.APIURL(),
-		HTMLURL:     issue.HTMLURL(),
 		Index:       issue.Index,
 		Poster:      ToUser(ctx, issue.Poster, nil),
 		Title:       issue.Title,
@@ -54,6 +52,8 @@ func ToAPIIssue(ctx context.Context, issue *issues_model.Issue) *api.Issue {
 		if err := issue.Repo.LoadOwner(ctx); err != nil {
 			return &api.Issue{}
 		}
+		apiIssue.URL = issue.APIURL()
+		apiIssue.HTMLURL = issue.HTMLURL()
 		apiIssue.Labels = ToLabelList(issue.Labels, issue.Repo, issue.Repo.Owner)
 		apiIssue.Repo = &api.RepositoryMeta{
 			ID:       issue.Repo.ID,

@qwerty287
Copy link
Contributor Author

Done, thanks @zeripath

@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 26, 2023
@zeripath
Copy link
Contributor

@qwerty287 do we think the HIDDEN thing is the correct thing to do? It seems like the simplest solution.

@qwerty287
Copy link
Contributor Author

Yes, I think that's the best solution for this case.

@lunny lunny added the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Mar 27, 2023
FullName: issue.Repo.FullName(),
if issue.Repo != nil {
if err := issue.Repo.LoadOwner(ctx); err != nil {
return &api.Issue{}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we hide the error or record it into log?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does not do so for LoadLabels and LoadPoster as well.

@jolheiser jolheiser enabled auto-merge (squash) March 28, 2023 16:55
@jolheiser
Copy link
Member

🎺 🤖

@jolheiser jolheiser merged commit 3cab9c6 into go-gitea:main Mar 28, 2023
@jolheiser jolheiser removed the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Mar 28, 2023
zjjhot added a commit to zjjhot/gitea that referenced this pull request Mar 29, 2023
* upstream/main:
  Refactor internal API for git commands, use meaningful messages instead of "Internal Server Error" (go-gitea#23687)
  Add CSS rules for basic colored labels (go-gitea#23774)
  Add meilisearch support (go-gitea#23136)
  Add missing translation for `actions.runners.reset_registration_token_success` (go-gitea#23732)
  [skip ci] Updated translations via Crowdin
  Implement Issue Config (go-gitea#20956)
  Set repository link based on the url in package.json for npm packages (go-gitea#20379)
  Add API to manage issue dependencies (go-gitea#17935)
  Add creation time in tag list page (go-gitea#23693)
  Make minio package support legacy MD5 checksum (go-gitea#23768)
  Yarden Shoham has a new email address (go-gitea#23767)
  fix br display for packages curls (go-gitea#23737)
@qwerty287 qwerty287 deleted the deps-api branch March 29, 2023 16:06
@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/done This PR has enough approvals to get merged. There are no important open reservations anymore. modifies/api This PR adds API routes or modifies them type/feature Completely new functionality. Can only be merged if feature freeze is not active.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add issue's dependencies informations in issue API endpoint [API] Manage Issue/Pull Dependencies
10 participants