Skip to content

Prevent DeleteUser API abuse#10125

Merged
lafriks merged 2 commits into
go-gitea:masterfrom
6543-forks:prefent-api-missuse
Feb 3, 2020
Merged

Prevent DeleteUser API abuse#10125
lafriks merged 2 commits into
go-gitea:masterfrom
6543-forks:prefent-api-missuse

Conversation

@6543
Copy link
Copy Markdown
Member

@6543 6543 commented Feb 3, 2020

close #10119

@6543
Copy link
Copy Markdown
Member Author

6543 commented Feb 3, 2020

I think we should backport this ...

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Feb 3, 2020
@techknowlogick techknowlogick changed the title Prefent API missuse Prevent API misuse Feb 3, 2020
@techknowlogick techknowlogick added this to the 1.12.0 milestone Feb 3, 2020
@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 Feb 3, 2020
@6543
Copy link
Copy Markdown
Member Author

6543 commented Feb 3, 2020

I run in this trap while writing tests for the sdk ...

@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 Feb 3, 2020
@6543 6543 changed the title Prevent API misuse Prevent DeleteUser API abuse Feb 3, 2020
Copy link
Copy Markdown
Member

@jolheiser jolheiser left a comment

Choose a reason for hiding this comment

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

Small nits otherwise LGTM

Comment thread routers/api/v1/admin/user.go Outdated
Comment thread routers/org/setting.go Outdated
@codecov-io
Copy link
Copy Markdown

Codecov Report

❗ No coverage uploaded for pull request base (master@29151b9). Click here to learn what that means.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master   #10125   +/-   ##
=========================================
  Coverage          ?    43.4%           
=========================================
  Files             ?      576           
  Lines             ?    79636           
  Branches          ?        0           
=========================================
  Hits              ?    34565           
  Misses            ?    40791           
  Partials          ?     4280
Impacted Files Coverage Δ
routers/org/setting.go 0% <ø> (ø)
routers/api/v1/admin/user.go 30.19% <0%> (ø)

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 29151b9...fbc34ad. Read the comment docs.

@lafriks lafriks merged commit ea50f60 into go-gitea:master Feb 3, 2020
@lafriks
Copy link
Copy Markdown
Member

lafriks commented Feb 3, 2020

Please send backport

@6543 6543 deleted the prefent-api-missuse branch February 3, 2020 16:46
6543 added a commit to 6543-forks/gitea that referenced this pull request Feb 3, 2020
* fix & co

* word suggestions from @jolheiser
@6543
Copy link
Copy Markdown
Member Author

6543 commented Feb 3, 2020

done -> #10128

@lafriks lafriks added the backport/done All backports for this PR have been created label Feb 3, 2020
lafriks pushed a commit that referenced this pull request Feb 3, 2020
@lunny
Copy link
Copy Markdown
Member

lunny commented Feb 4, 2020

I think it's better to move the check into models.DeleteUser.

@6543
Copy link
Copy Markdown
Member Author

6543 commented Feb 4, 2020

should I send a refactor PR?

@guillep2k
Copy link
Copy Markdown
Member

I think it's better to move the check into models.DeleteUser.

So orgs can't be deleted ever? 🤔

@6543
Copy link
Copy Markdown
Member Author

6543 commented Feb 4, 2020

@guillep2k Orgs have there own delete function:

gitea/models/org.go

Lines 257 to 275 in b3c72a7

// DeleteOrganization completely and permanently deletes everything of organization.
func DeleteOrganization(org *User) (err error) {
sess := x.NewSession()
defer sess.Close()
if err = sess.Begin(); err != nil {
return err
}
if err = deleteOrg(sess, org); err != nil {
if IsErrUserOwnRepos(err) {
return err
} else if err != nil {
return fmt.Errorf("deleteOrg: %v", err)
}
}
return sess.Commit()
}

to be exact it is here:

gitea/models/org.go

Lines 299 to 301 in b3c72a7

if _, err = e.ID(u.ID).Delete(new(User)); err != nil {
return fmt.Errorf("Delete: %v", err)
}

@6543
Copy link
Copy Markdown
Member Author

6543 commented Feb 4, 2020

@lunny -> #10134

@go-gitea go-gitea locked and limited conversation to collaborators Nov 24, 2020
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.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[API] [Bug] get 500 on Dashborard when delete org

9 participants