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

fix: Actor is required to get user repositories #20443

Merged
merged 5 commits into from
Jul 21, 2022

Conversation

dhruvmanila
Copy link
Contributor

fixes: #20442

@GiteaBot GiteaBot added the lgtm/need 1 This PR needs approval from one additional maintainer to be merged. label Jul 21, 2022
Copy link
Member

@silverwind silverwind left a comment

Choose a reason for hiding this comment

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

redacting review for now.

@wxiaoguang wxiaoguang removed the lgtm/need 1 This PR needs approval from one additional maintainer to be merged. label Jul 21, 2022
@GiteaBot GiteaBot added the lgtm/need 1 This PR needs approval from one additional maintainer to be merged. label Jul 21, 2022
@6543
Copy link
Member

6543 commented Jul 21, 2022

what we could do to not have a panic but an error:

check for actor == nil and if so return an error.New("Actor is needed but not given")

@dhruvmanila
Copy link
Contributor Author

So, the panic was thrown because instead of passing in the user model directly through the Actor field, it's passing the id through the OwnerId field:

repos, _, err := repo_model.GetUserRepositories(&repo_model.SearchRepoOptions{
ListOptions: db.ListOptions{
PageSize: repo_model.RepositoryListDefaultPageSize,
Page: 1,
},
Private: true,
OwnerID: u.ID,
})

As per the reviews, I think 2 changes need to be done:

  1. Pass in the user model directly through the Actor option
  2. Return an error if Actor is not given (as suggested by @6543)

@dhruvmanila dhruvmanila changed the title fix: check if field is not nil to avoid nil pointer dereference fix: Actor is required to get user repositories Jul 21, 2022
@dhruvmanila dhruvmanila force-pushed the fix/admin-purge-user branch from 492aa2b to 645f58b Compare July 21, 2022 19:10
@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 Jul 21, 2022
@6543 6543 added the type/bug label Jul 21, 2022
@6543 6543 added this to the 1.18.0 milestone Jul 21, 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.

I have a feeling we should change repo_model.GetUserRepositories to use OwnerID and not Actor to list the UserRepositories. However, this does fix the bug.

I apologise for merging the PR with this bug - it is due to a mismerge.

@zeripath
Copy link
Contributor

I've added a test case too

@zeripath zeripath added skip-changelog This PR is irrelevant for the (next) changelog, for example bug fixes for unreleased features. and removed backport/v1.17 labels Jul 21, 2022
@zeripath
Copy link
Contributor

Doesn't need backporting as far as I'm aware as purging hasn't been backported

@6543
Copy link
Member

6543 commented Jul 21, 2022

🚀

@6543 6543 merged commit 3df3379 into go-gitea:main Jul 21, 2022
@dhruvmanila dhruvmanila deleted the fix/admin-purge-user branch July 22, 2022 03:08
zjjhot added a commit to zjjhot/gitea that referenced this pull request Jul 25, 2022
* giteaofficial/main:
  Fix Ruby package parsing by removed unused email field (go-gitea#20470)
  [skip ci] Updated translations via Crowdin
  Add repository condition for issue count (go-gitea#20454)
  Prepend commit message to template content (go-gitea#20429)
  Improve pprof doc (go-gitea#20463)
  Improve code diff highlight, fix incorrect rendered diff result (go-gitea#19958)
  Add Cache-Control header to html and api responses, add no-transform (go-gitea#20432)
  [skip ci] Updated translations via Crowdin
  Allow non-semver packages in the Conan package registry (go-gitea#20412)
  Use body text color in repository files table links (go-gitea#20386)
  Correct code block in installation docs for Snap (go-gitea#20440)
  Downgrade golangci-lint to 1.47.0 (go-gitea#20445)
  Add eslint-plugin-sonarjs (go-gitea#20431)
  Fix: Actor is required to get user repositories (go-gitea#20443)
  Add "X-Gitea-Object-Type" header for GET `/raw/` & `/media/` API (go-gitea#20438)
  Simplify visibility checks (go-gitea#20406)
vsysoev pushed a commit to IntegraSDL/gitea that referenced this pull request Aug 10, 2022
Co-authored-by: 6543 <6543@obermui.de>
Co-authored-by: Andrew Thornton <art27@cantab.net>
@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. skip-changelog This PR is irrelevant for the (next) changelog, for example bug fixes for unreleased features. type/bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Panic when admin tries to delete a user with purge=true
6 participants