Skip to content

Fix delete nonexist oauth application 500 and prevent deadlock #15384

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

Merged
merged 7 commits into from
Apr 10, 2021

Conversation

lunny
Copy link
Member

@lunny lunny commented Apr 10, 2021

Return 404 when asked to delete OAuth2 Application and ensure that the session is closed too.

Fix #15356

@GiteaBot GiteaBot added the lgtm/need 1 This PR needs approval from one additional maintainer to be merged. label Apr 10, 2021
@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 Apr 10, 2021
@6543 6543 added the modifies/api This PR adds API routes or modifies them label Apr 10, 2021
@zeripath
Copy link
Contributor

Somehow this appears to be killing CI.

@6543 6543 added this to the 1.15.0 milestone Apr 10, 2021
@lunny
Copy link
Member Author

lunny commented Apr 10, 2021

I guess MakeRequest could not be invoke twice because req has been consumed.

@lunny lunny force-pushed the lunny/delete_oauth2 branch from f063aa5 to 44b893f Compare April 10, 2021 13:48
@zeripath zeripath changed the title Fix delete nonexist oauth application 500 Fix delete nonexist oauth application 500 and prevent deadlock Apr 10, 2021
@codecov-io
Copy link

Codecov Report

Merging #15384 (b3ebe86) into master (487f2ee) will increase coverage by 1.41%.
The diff coverage is 55.56%.

❗ Current head b3ebe86 differs from pull request most recent head 409226e. Consider uploading reports for the commit 409226e to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master   #15384      +/-   ##
==========================================
+ Coverage   42.21%   43.63%   +1.41%     
==========================================
  Files         767      677      -90     
  Lines       81624    81338     -286     
==========================================
+ Hits        34458    35488    +1030     
+ Misses      41531    40058    -1473     
- Partials     5635     5792     +157     
Impacted Files Coverage Δ
cmd/admin.go 0.00% <0.00%> (ø)
cmd/dump.go 0.93% <0.00%> (-0.01%) ⬇️
cmd/hook.go 0.00% <0.00%> (ø)
cmd/serv.go 2.61% <0.00%> (-0.02%) ⬇️
cmd/web.go 0.00% <0.00%> (ø)
cmd/web_graceful.go 0.00% <0.00%> (ø)
cmd/web_letsencrypt.go 0.00% <0.00%> (ø)
models/admin.go 60.31% <ø> (ø)
models/branches.go 42.61% <ø> (+1.51%) ⬆️
models/commit_status.go 61.74% <0.00%> (ø)
... and 400 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c680eb2...409226e. Read the comment docs.

@techknowlogick techknowlogick merged commit 1fc1d60 into go-gitea:master Apr 10, 2021
@techknowlogick
Copy link
Member

@lunny please send backports :)

@lunny lunny deleted the lunny/delete_oauth2 branch April 11, 2021 01:18
lunny added a commit to lunny/gitea that referenced this pull request Apr 11, 2021
…tea#15384)

* Fix delete nonexist oauth application 500

* Fix test

* Close the session

Signed-off-by: Andrew Thornton <[email protected]>

* Update integrations/api_oauth2_apps_test.go

* Fix more missed sess.Close

* Remove unnecessary blank line

Co-authored-by: Andrew Thornton <[email protected]>
Co-authored-by: 6543 <[email protected]>
lunny added a commit to lunny/gitea that referenced this pull request Apr 11, 2021
…tea#15384)

* Fix delete nonexist oauth application 500

* Fix test

* Close the session

Signed-off-by: Andrew Thornton <[email protected]>

* Update integrations/api_oauth2_apps_test.go

* Fix more missed sess.Close

* Remove unnecessary blank line

Co-authored-by: Andrew Thornton <[email protected]>
Co-authored-by: 6543 <[email protected]>
@lunny lunny added the backport/done All backports for this PR have been created label Apr 11, 2021
6543 added a commit that referenced this pull request Apr 11, 2021
… (#15397)

* Fix delete nonexist oauth application 500

* Fix test

* Close the session

* Fix more missed sess.Close

* Remove unnecessary blank line

Signed-off-by: Andrew Thornton <[email protected]>
Co-authored-by: Andrew Thornton <[email protected]>
Co-authored-by: 6543 <[email protected]>
6543 added a commit that referenced this pull request Apr 11, 2021
… (#15396)

* Fix delete nonexist oauth application 500

* Fix test

* Close the session

* Fix more missed sess.Close

* Remove unnecessary blank line

Signed-off-by: Andrew Thornton <[email protected]>
Co-authored-by: Andrew Thornton <[email protected]>
Co-authored-by: 6543 <[email protected]>
@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
backport/done All backports for this PR have been created 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/bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Deleting Oauth application via api with nonexistent id causes internal server error
6 participants