diff --git a/server/api/login_test.go b/server/api/login_test.go index 18fb77ed0..5474f0b9c 100644 --- a/server/api/login_test.go +++ b/server/api/login_test.go @@ -250,6 +250,9 @@ func TestHandleAuth(t *testing.T) { assert.Equal(t, http.StatusSeeOther, c.Writer.Status()) assert.Equal(t, "/login?error=registration_closed", c.Writer.Header().Get("Location")) + // a rejected login must not persist any (broken) user/org row, see #6769 + _store.AssertNotCalled(t, "CreateUser", mock.Anything) + _store.AssertNotCalled(t, "OrgCreate", mock.Anything) }) t.Run("should deny a user with missing org access", func(t *testing.T) { diff --git a/server/store/datastore/migration/029_fix_zero_forge_id_user_ref.go b/server/store/datastore/migration/029_fix_zero_forge_id_user_ref.go new file mode 100644 index 000000000..f0f580566 --- /dev/null +++ b/server/store/datastore/migration/029_fix_zero_forge_id_user_ref.go @@ -0,0 +1,43 @@ +// Copyright 2026 Woodpecker Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package migration + +import ( + "src.techknowlogick.com/xormigrate" + "xorm.io/xorm" +) + +// Users provisioned through the admin web UI or CLI (`admin user add`) used to +// be stored with forge_id=0 when no forge was given, leaving them unresolvable +// for OAuth login (they never match the real forge id, so login is rejected with +// "registration closed"). This heals those rows by clamping them to the default +// forge, mirroring the runtime default (api.defaultForgeID) now enforced on user +// creation and the sibling `replaceZeroForgeIDsInOrgs` migration for orgs. +// +// The orgs update is repeated because a broken user's personal org is created +// with the same forge_id=0 and could have been added after that earlier orgs +// migration already ran. +// +// See https://github.com/woodpecker-ci/woodpecker/issues/6769. +var replaceZeroForgeIDsInUsers = xormigrate.Migration{ + ID: "replace-zero-forge-ids-in-users", + MigrateSession: func(sess *xorm.Session) (err error) { + if _, err = sess.Exec("UPDATE users SET forge_id=1 WHERE forge_id=0;"); err != nil { + return err + } + _, err = sess.Exec("UPDATE orgs SET forge_id=1 WHERE forge_id=0;") + return err + }, +} diff --git a/server/store/datastore/migration/029_fix_zero_forge_id_user_ref_test.go b/server/store/datastore/migration/029_fix_zero_forge_id_user_ref_test.go new file mode 100644 index 000000000..8f6fcea8c --- /dev/null +++ b/server/store/datastore/migration/029_fix_zero_forge_id_user_ref_test.go @@ -0,0 +1,89 @@ +// Copyright 2026 Woodpecker Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package migration + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +type userV029 struct { + ID int64 `xorm:"pk autoincr 'id'"` + ForgeID int64 `xorm:"forge_id"` + Login string `xorm:"'login'"` + Hash string `xorm:"'hash'"` +} + +func (userV029) TableName() string { return "users" } + +type orgV029 struct { + ID int64 `xorm:"pk autoincr 'id'"` + ForgeID int64 `xorm:"forge_id"` + Name string `xorm:"'name'"` + IsUser bool `xorm:"is_user"` +} + +func (orgV029) TableName() string { return "orgs" } + +func TestReplaceZeroForgeIDsInUsers(t *testing.T) { + engine, closeDB := testDB(t, true) + defer closeDB() + + require.NoError(t, engine.Sync(new(userV029), new(orgV029))) + + // user + personal org left with forge_id=0 by legacy CLI/web-ui provisioning + _, err := engine.Insert(&userV029{ForgeID: 0, Login: "broken", Hash: "h1"}) + require.NoError(t, err) + _, err = engine.Insert(&orgV029{ForgeID: 0, Name: "broken", IsUser: true}) + require.NoError(t, err) + // a healthy user/org already on the real forge must stay untouched + _, err = engine.Insert(&userV029{ForgeID: 1, Login: "healthy", Hash: "h2"}) + require.NoError(t, err) + _, err = engine.Insert(&orgV029{ForgeID: 2, Name: "other-forge", IsUser: false}) + require.NoError(t, err) + + sess := engine.NewSession() + defer sess.Close() + require.NoError(t, replaceZeroForgeIDsInUsers.MigrateSession(sess)) + require.NoError(t, sess.Commit()) + + zeroUsers, err := engine.Where("forge_id = 0").Count(new(userV029)) + require.NoError(t, err) + assert.EqualValues(t, 0, zeroUsers, "no user should be left on forge_id=0") + + zeroOrgs, err := engine.Where("forge_id = 0").Count(new(orgV029)) + require.NoError(t, err) + assert.EqualValues(t, 0, zeroOrgs, "no org should be left on forge_id=0") + + // the healed user is now resolvable on the default forge + healed := new(userV029) + found, err := engine.Where("login = ?", "broken").Get(healed) + require.NoError(t, err) + require.True(t, found) + assert.EqualValues(t, 1, healed.ForgeID) + + // unrelated forge ids are untouched + healthy := new(userV029) + _, err = engine.Where("login = ?", "healthy").Get(healthy) + require.NoError(t, err) + assert.EqualValues(t, 1, healthy.ForgeID) + + otherOrg := new(orgV029) + _, err = engine.Where("name = ?", "other-forge").Get(otherOrg) + require.NoError(t, err) + assert.EqualValues(t, 2, otherOrg.ForgeID) +} diff --git a/server/store/datastore/migration/migration.go b/server/store/datastore/migration/migration.go index c34055489..0037cf5f8 100644 --- a/server/store/datastore/migration/migration.go +++ b/server/store/datastore/migration/migration.go @@ -57,6 +57,7 @@ var migrationTasks = []*xormigrate.Migration{ &fixForgeColumns, &addCronField, &updatePipelineStructureTagsReleases, + &replaceZeroForgeIDsInUsers, } var allBeans = []any{