Skip to content

fix github migration error when using multiple tokens #34144

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 4 commits into from
Apr 13, 2025

Conversation

TheFox0x7
Copy link
Contributor

@TheFox0x7 TheFox0x7 commented Apr 7, 2025

Git authorization was not taking into account multiple token feature, leading to auth failures

Closes: #34141


I made it default to first token. I'm honestly not sure if it wouldn't be better to scrap the multi token feature instead. The UI hides the tokens so it's hard to tell if no mistake was made in the input and it's the only provider "supporting" this.
I guess the intention was to somewhat speed up migration from github for a single repository which is nice too.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Apr 7, 2025
@github-actions github-actions bot added the modifies/go Pull requests that update Go code label Apr 7, 2025
@TheFox0x7
Copy link
Contributor Author

Is the basicAuth setup in client creation even working?

@pull-request-size pull-request-size bot added size/M and removed size/S labels Apr 8, 2025
@TheFox0x7 TheFox0x7 marked this pull request as ready for review April 8, 2025 21:22
@wxiaoguang
Copy link
Contributor

IIRC "multiple tokens" are used to benefit more "access rate limit quota"? If only the first token is used, would it be more likely reach the rate limit?

@TheFox0x7
Copy link
Contributor Author

Honestly, I'm not sure if git clones count for rate limits. That's not rest api.

Thread I found on this suggests they don't: https://github.com/orgs/community/discussions/44515

@TheFox0x7
Copy link
Contributor Author

I'm fairly sure that basic auth can be removed but gitbucket might depend on it, which I know nothing about.

@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 Apr 13, 2025
@wxiaoguang wxiaoguang added this to the 1.24.0 milestone Apr 13, 2025
@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 13, 2025
@wxiaoguang wxiaoguang enabled auto-merge (squash) April 13, 2025 23:09
@wxiaoguang wxiaoguang merged commit 8a6df00 into go-gitea:main Apr 13, 2025
26 checks passed
@wxiaoguang
Copy link
Contributor

@TheFox0x7

#34141 (comment)

the entire GitHub migration was failing after this fix

zjjhot added a commit to zjjhot/gitea that referenced this pull request Apr 14, 2025
* giteaofficial/main: (27 commits)
  fix github migration error when using multiple tokens (go-gitea#34144)
  Add package version api endpoints (go-gitea#34173)
  Fix incorrect file links (go-gitea#34189)
  Add cache for common package queries (go-gitea#22491)
  Allow admins and org owners to change org member public status (go-gitea#28294)
  Fix span svg layout (go-gitea#34185)
  fix webhook url (go-gitea#34186)
  Optimize overflow-menu (go-gitea#34183)
  Move and rename UpdateRepository (go-gitea#34136)
  Update milestones.tmpl (go-gitea#34184)
  [skip ci] Updated translations via Crowdin
  Refactor Git Attribute & performance optimization (go-gitea#34154)
  [skip ci] Updated translations via Crowdin
  fix(go-gitea#33711): cross-publish docker images to ghcr.io (go-gitea#34148)
  refactor organization menu (go-gitea#33928)
  feat: Add sorting by exclusive labels (issue priority) (go-gitea#33206)
  Fix vertical centering of file tree icons and use entryIcon for submodules/symlinks (go-gitea#34137)
  bugfix check for alternate ssh host certificate location (go-gitea#34146)
  Cache GPG keys, emails and users when list commits (go-gitea#34086)
  Set MERMAID_MAX_SOURCE_CHARACTERS to 50000 (go-gitea#34152)
  ...
@TheFox0x7
Copy link
Contributor Author

TheFox0x7 commented Apr 14, 2025

I'll send a followup when I'll find the cause. I have no clues currently. Since CI passed it I doubt it's a single token issue (unless for some reason that test doesn't run in CI) so shouldn't be a regression at the very least.

Without the fix the test passes on single token input.

@wxiaoguang
Copy link
Contributor

wxiaoguang commented Apr 14, 2025

CI passes, but does it really work with GitHub oauth2?

By reading the "oauth2" code, it does token.SetAuthHeader(req2), then:

image

Is it the same as your new code u.User = url.UserPassword(...) ?

@TheFox0x7
Copy link
Contributor Author

TheFox0x7 commented Apr 14, 2025

It worked before and the single test outcome is the same with and without the fix - both pass. So unless the token workflow stopped working there's no changes there.

Logic is taken from NullDownloader just without support for user/pass system.
https://github.com/TheFox0x7/gitea/blob/164a382bcdcf70b67983bb5e5ff8ef310606d340/modules/migration/null_downloader.go#L67-L80

Let's wait for more details from the reporter.

@wxiaoguang
Copy link
Contributor

If I understand correctly, the "oauth2:token" works with GitHub migration on your side, and at the moment it's unclear about how to reproduce the user's problem, right?

@TheFox0x7
Copy link
Contributor Author

oauth2:token is the default system in general when token is passed (from NullDownloader), it's just our Github migration supports multiple tokens which encoded oauth2:token1,token2 and ended up treating it as basic auth.
The single token case has not changed and still is oauth2:token - it worked before and I don't see why it should stop now.

Yes, at the moment is unclear what is the users issue after the patch. It could be any of:

  • a regression in single token system
  • the fix was not sufficient to make multitoken work
  • the issue is unrelated
  • version tested does not have the patch and it's a false positive

Currently I suspect the last case, but I can manually test it later to be extra sure.

project-mirrors-bot-tu bot pushed a commit to project-mirrors/forgejo-as-gitea-fork that referenced this pull request Apr 19, 2025
Git authorization was not taking into account multiple token feature,
leading to auth failures

Closes: go-gitea#34141

---------

Co-authored-by: wxiaoguang <[email protected]>
(cherry picked from commit 8a6df00)
@lunny lunny added the backport/v1.23 This PR should be backported to Gitea 1.23 label Apr 28, 2025
GiteaBot pushed a commit to GiteaBot/gitea that referenced this pull request Apr 28, 2025
Git authorization was not taking into account multiple token feature,
leading to auth failures

Closes: go-gitea#34141

---------

Co-authored-by: wxiaoguang <[email protected]>
@GiteaBot GiteaBot added the backport/done All backports for this PR have been created label Apr 28, 2025
lunny added a commit that referenced this pull request Apr 28, 2025
Backport #34144 by @TheFox0x7

Git authorization was not taking into account multiple token feature,
leading to auth failures

Closes: #34141

---------

Co-authored-by: TheFox0x7 <[email protected]>
Co-authored-by: wxiaoguang <[email protected]>
Co-authored-by: Lunny Xiao <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport/done All backports for this PR have been created backport/v1.23 This PR should be backported to Gitea 1.23 lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. modifies/go Pull requests that update Go code type/bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Error while migrating with multiple github token
6 participants