Skip to content

Support multiple users with same login name but different forges#5612

Merged
xoxys merged 16 commits into
mainfrom
fix-user-lookup
Oct 21, 2025
Merged

Support multiple users with same login name but different forges#5612
xoxys merged 16 commits into
mainfrom
fix-user-lookup

Conversation

@anbraten
Copy link
Copy Markdown
Member

@anbraten anbraten commented Oct 7, 2025

Currently we just lookup the user by it's login name.
If you have the same name on multible forges connected to Woodpecker, on login the repos randomly change the user of one forge to another witch then potentially sets the user of a different forge as the repo as repo owner ...
... other strange things can happen too ...

this changes it to have the forgeRemoteID be the primary way to lookup users inside the database...

@anbraten anbraten added the bug Something isn't working label Oct 7, 2025
@woodpecker-bot
Copy link
Copy Markdown
Contributor

woodpecker-bot commented Oct 7, 2025

@6543 6543 changed the title fix: find users with forge-id Support multible users with same login name but different forges Oct 14, 2025
@6543 6543 added the server label Oct 14, 2025
@6543 6543 marked this pull request as ready for review October 14, 2025 01:13
Comment thread server/store/datastore/user.go Outdated
Copy link
Copy Markdown
Member

@6543 6543 left a comment

Choose a reason for hiding this comment

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

beside that nit, like it ...

@6543
Copy link
Copy Markdown
Member

6543 commented Oct 14, 2025

PS: we have a simmilar problem if a organisation is renamed ...

@6543
Copy link
Copy Markdown
Member

6543 commented Oct 14, 2025

PS: PS: I'll address the test errors tomorow ... but the constrain should be here now ... as we use it as key for users now

@6543 6543 mentioned this pull request Oct 14, 2025
21 tasks
@anbraten
Copy link
Copy Markdown
Member Author

code lgtm

@codecov
Copy link
Copy Markdown

codecov Bot commented Oct 14, 2025

Codecov Report

❌ Patch coverage is 9.38967% with 193 lines in your changes missing coverage. Please review.
✅ Project coverage is 21.30%. Comparing base (fed0ced) to head (8a659dd).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
server/store/mocks/mock_Store.go 0.00% 92 Missing ⚠️
woodpecker-go/woodpecker/mocks/mock_Client.go 0.00% 48 Missing ⚠️
server/api/users.go 0.00% 41 Missing ⚠️
server/api/login.go 61.53% 4 Missing and 1 partial ⚠️
woodpecker-go/woodpecker/user.go 64.28% 5 Missing ⚠️
server/store/datastore/user.go 60.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5612      +/-   ##
==========================================
- Coverage   21.31%   21.30%   -0.01%     
==========================================
  Files         420      420              
  Lines       38080    38163      +83     
==========================================
+ Hits         8116     8130      +14     
- Misses      29211    29280      +69     
  Partials      753      753              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@6543
Copy link
Copy Markdown
Member

6543 commented Oct 14, 2025

@anbraten I fixed more multi-user forge stuff you might review again :)

Comment thread server/api/users.go
@6543
Copy link
Copy Markdown
Member

6543 commented Oct 14, 2025

@anbraten could you write lgtm or deny again :)

I know review dont work on own pulls 😓

@6543 6543 added enhancement improve existing features bug Something isn't working and removed bug Something isn't working labels Oct 14, 2025
@xoxys
Copy link
Copy Markdown
Member

xoxys commented Oct 14, 2025

I dont like to introduce new cli flags using id's. Thats weird to use not human friendly and should be avoided.

@6543
Copy link
Copy Markdown
Member

6543 commented Oct 14, 2025

then I'll remove that cli change part for now

@6543 6543 requested a review from xoxys October 14, 2025 17:56
@6543
Copy link
Copy Markdown
Member

6543 commented Oct 15, 2025

@xoxys did remove the cli patch

@qwerty287 qwerty287 changed the title Support multible users with same login name but different forges Support multiple users with same login name but different forges Oct 15, 2025
@xoxys
Copy link
Copy Markdown
Member

xoxys commented Oct 15, 2025

See my comments in the chat.

Copy link
Copy Markdown
Member

@xoxys xoxys left a comment

Choose a reason for hiding this comment

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

Code LGMT, but hard to test locally. My personal preference would be an rc-release first, or release 3.11 without this change, and merge it after the release to get it battle-tested using the next tag.

Comment thread server/model/repo.go
Copy link
Copy Markdown
Member Author

@anbraten anbraten left a comment

Choose a reason for hiding this comment

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

would keep the changes to the actual fix and a minimum as those areas apis are quite part of the core and therefore critical

Comment thread server/model/repo.go
Comment thread server/api/users.go
@6543
Copy link
Copy Markdown
Member

6543 commented Oct 16, 2025

ugh ... I can strip it out 😓

@xoxys xoxys enabled auto-merge (squash) October 21, 2025 06:07
@xoxys xoxys merged commit 045a222 into main Oct 21, 2025
7 checks passed
@xoxys xoxys deleted the fix-user-lookup branch October 21, 2025 06:19
@woodpecker-bot woodpecker-bot mentioned this pull request Oct 21, 2025
1 task
@6543
Copy link
Copy Markdown
Member

6543 commented Oct 21, 2025

well thanks 😅 I still was going to split it out but that makes it way easyer for me :)

@xoxys
Copy link
Copy Markdown
Member

xoxys commented Oct 21, 2025

As we have done the release without this PR, need to overcomplicate things IMO. Just keep in mind if we have to revert partially this needs to be done manually. If I misunderstood out discussion in the chat and overruled you @anbraten, sorry for that, this was not on purpose.

@6543
Copy link
Copy Markdown
Member

6543 commented Oct 21, 2025

no problem :) just ping me if there is an issue with this exact patch

@6543
Copy link
Copy Markdown
Member

6543 commented Oct 21, 2025

created snapshot and deployed on our own ci ...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working enhancement improve existing features server

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants