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

Unsanitize user and org names in DB #4762

Open
wants to merge 26 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
26 commits
Select commit Hold shift + click to select a range
ecc7541
fix: add user migration
pat-s Jan 23, 2025
dce8939
chore: update years
pat-s Jan 23, 2025
1aa34c1
chore: update migration to unsanitize
pat-s Jan 23, 2025
19ac5be
chore: remove all toLower calls
pat-s Jan 23, 2025
291b1be
adjust tests to new migration
pat-s Jan 23, 2025
6a71341
chore: spell
pat-s Jan 23, 2025
c0264f8
Update server/store/datastore/migration/024_unsanitize_org_and_user_n…
pat-s Jan 23, 2025
fd289fa
Update server/store/datastore/migration/024_unsanitize_org_and_user_n…
pat-s Jan 23, 2025
d05055b
Update server/store/datastore/migration/024_unsanitize_org_and_user_n…
pat-s Jan 23, 2025
0df49fa
Update server/store/datastore/migration/024_unsanitize_org_and_user_n…
pat-s Jan 23, 2025
1c3f31e
chore: remove org test
pat-s Jan 23, 2025
66da958
chore: update test
pat-s Jan 23, 2025
f2df9a2
chore: format
pat-s Jan 23, 2025
3f297ad
chore: adjust more tests
pat-s Jan 23, 2025
16f2469
Update server/store/datastore/org.go
pat-s Jan 23, 2025
a2d5946
chore: review
pat-s Jan 23, 2025
8e0fc7a
import
pat-s Jan 24, 2025
1771ce8
Merge branch 'main' into fix/sanitize-usernames-db
xoxys Jan 24, 2025
e638c62
clean header
pat-s Jan 28, 2025
69c7d75
add tests checking case-sensitive DB duplicates
pat-s Feb 1, 2025
d354425
Merge branch 'main' into fix/sanitize-usernames-db
xoxys Feb 1, 2025
73d4f19
restore toLower
pat-s Feb 2, 2025
aa726dc
improve user org handling
anbraten Feb 2, 2025
31f31c4
fix test
anbraten Feb 2, 2025
90df661
Update server/api/login.go
pat-s Feb 6, 2025
30a2c83
chore: remove sanitation when getting orgName
pat-s Feb 6, 2025
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .cspell.json
Original file line number Diff line number Diff line change
Expand Up @@ -201,6 +201,7 @@
"typecheck",
"Typeflag",
"unplugin",
"unsanitize",
"Upsert",
"urfave",
"usecase",
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
// Excerpt from:
// Copyright 2024 Woodpecker Authors
pat-s marked this conversation as resolved.
Show resolved Hide resolved
//
// 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 (
"fmt"

"src.techknowlogick.com/xormigrate"
"xorm.io/xorm"
)

var unSanitizeOrgAndUserNames = xormigrate.Migration{
ID: "unsanitize-org-and-user-names",
MigrateSession: func(sess *xorm.Session) (err error) {
type user struct {
Login string `xorm:"TEXT 'login'"`
}

type org struct {
Name string `xorm:"TEXT 'name'"`
}

if err := sess.Sync(new(user), new(org)); err != nil {
return fmt.Errorf("sync new models failed: %w", err)
}

// get all users
var us []*user
pat-s marked this conversation as resolved.
Show resolved Hide resolved
if err := sess.Find(&us); err != nil {
pat-s marked this conversation as resolved.
Show resolved Hide resolved
return fmt.Errorf("find all repos failed: %w", err)
}

for _, user := range us {
pat-s marked this conversation as resolved.
Show resolved Hide resolved
userOrg := &org{}
_, err := sess.Where("name = ?", user.Login).Get(userOrg)
if err != nil {
return fmt.Errorf("getting org failed: %w", err)
}
if user.Login != userOrg.Name {
anbraten marked this conversation as resolved.
Show resolved Hide resolved
userOrg.Name = user.Login
if _, err := sess.ID(userOrg.Name).Cols("Name").Update(userOrg); err != nil {
return fmt.Errorf("updating org name failed: %w", err)
}
}
}
return nil
},
}
4 changes: 2 additions & 2 deletions server/store/datastore/migration/common.go
Original file line number Diff line number Diff line change
Expand Up @@ -134,7 +134,7 @@ func alterColumnDefault(sess *xorm.Session, table, column, defValue string) erro
dialect := sess.Engine().Dialect().URI().DBType
switch dialect {
case schemas.MYSQL:
sql := fmt.Sprintf("SHOW COLUMNS FROM `%s` WHERE lower(field) = '%s'", table, strings.ToLower(column))
sql := fmt.Sprintf("SHOW COLUMNS FROM `%s` WHERE lower(field) = '%s'", table, column)
xoxys marked this conversation as resolved.
Show resolved Hide resolved
res, err := sess.Query(sql)
if err != nil {
return err
Expand Down Expand Up @@ -170,7 +170,7 @@ func alterColumnNull(sess *xorm.Session, table, column string, null bool) error
dialect := sess.Engine().Dialect().URI().DBType
switch dialect {
case schemas.MYSQL:
sql := fmt.Sprintf("SHOW COLUMNS FROM `%s` WHERE lower(field) = '%s'", table, strings.ToLower(column))
sql := fmt.Sprintf("SHOW COLUMNS FROM `%s` WHERE lower(field) = '%s'", table, column)
anbraten marked this conversation as resolved.
Show resolved Hide resolved
res, err := sess.Query(sql)
if err != nil {
return err
Expand Down
1 change: 1 addition & 0 deletions server/store/datastore/migration/migration.go
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@ var migrationTasks = []*xormigrate.Migration{
&renameTokenFields,
&setNewDefaultsForRequireApproval,
&removeRepoScm,
&unSanitizeOrgAndUserNames,
}

var allBeans = []any{
Expand Down
20 changes: 14 additions & 6 deletions server/store/datastore/org.go
Original file line number Diff line number Diff line change
Expand Up @@ -28,13 +28,24 @@ func (s storage) OrgCreate(org *model.Org) error {
}

func (s storage) orgCreate(org *model.Org, sess *xorm.Session) error {
// sanitize
org.Name = strings.ToLower(org.Name)
// Get all organizations to check for conflicts (this should actually be impossible in the forge but better double-check)
orgs, err := s.OrgList(nil)
if err != nil {
return err
}

for _, existingOrg := range orgs {
if strings.EqualFold(existingOrg.Name, org.Name) {
return fmt.Errorf("organization name already exists")
}
}
xoxys marked this conversation as resolved.
Show resolved Hide resolved

if org.Name == "" {
return fmt.Errorf("org name is empty")
}

// insert
_, err := sess.Insert(org)
_, err = sess.Insert(org)
return err
}

Expand All @@ -48,8 +59,6 @@ func (s storage) OrgUpdate(org *model.Org) error {
}

func (s storage) orgUpdate(sess *xorm.Session, org *model.Org) error {
// sanitize
org.Name = strings.ToLower(org.Name)
// update
_, err := sess.ID(org.ID).AllCols().Update(org)
return err
Expand Down Expand Up @@ -84,7 +93,6 @@ func (s storage) OrgFindByName(name string) (*model.Org, error) {

func (s storage) orgFindByName(sess *xorm.Session, name string) (*model.Org, error) {
// sanitize
name = strings.ToLower(name)
org := new(model.Org)
return org, wrapGet(sess.Where("name = ?", name).Get(org))
xoxys marked this conversation as resolved.
Show resolved Hide resolved
}
Expand Down
6 changes: 3 additions & 3 deletions server/store/datastore/org_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ func TestOrgCRUD(t *testing.T) {

// create first org to play with
assert.NoError(t, store.OrgCreate(org1))
assert.EqualValues(t, "someawesomeorg", org1.Name)
assert.EqualValues(t, "someAwesomeOrg", org1.Name)

// retrieve it
orgOne, err := store.OrgGet(org1.ID)
Expand All @@ -48,13 +48,13 @@ func TestOrgCRUD(t *testing.T) {
assert.Error(t, store.OrgCreate(&model.Org{Name: "reNamedorg"}))

// find updated org by name
orgOne, err = store.OrgFindByName("renamedorG")
orgOne, err = store.OrgFindByName("RenamedOrg")
assert.NoError(t, err)
assert.NotEqualValues(t, org1, orgOne)
assert.EqualValues(t, org1.ID, orgOne.ID)
assert.EqualValues(t, false, orgOne.IsUser)
assert.EqualValues(t, false, orgOne.Private)
assert.EqualValues(t, "renamedorg", orgOne.Name)
assert.EqualValues(t, "RenamedOrg", orgOne.Name)

// create two more orgs and repos
someUser := &model.Org{Name: "some_other_u", IsUser: true}
Expand Down
3 changes: 1 addition & 2 deletions server/store/datastore/repo.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@ package datastore
import (
"errors"
"fmt"
"strings"

"xorm.io/builder"
"xorm.io/xorm"
Expand Down Expand Up @@ -73,7 +72,7 @@ func (s storage) GetRepoName(fullName string) (*model.Repo, error) {

func (s storage) getRepoName(e *xorm.Session, fullName string) (*model.Repo, error) {
repo := new(model.Repo)
return repo, wrapGet(e.Where("LOWER(full_name) = ?", strings.ToLower(fullName)).Get(repo))
return repo, wrapGet(e.Where("full_name = ?", fullName).Get(repo))
xoxys marked this conversation as resolved.
Show resolved Hide resolved
}

func (s storage) GetRepoCount() (int64, error) {
Expand Down
6 changes: 1 addition & 5 deletions server/store/datastore/repo_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -113,11 +113,7 @@ func TestGetRepoName(t *testing.T) {

// case-insensitive
getrepo, err = store.GetRepoName("Bradrydzewski/test")
assert.NoError(t, err)
assert.Equal(t, repo.ID, getrepo.ID)
assert.Equal(t, repo.UserID, getrepo.UserID)
assert.Equal(t, repo.Owner, getrepo.Owner)
assert.Equal(t, repo.Name, getrepo.Name)
assert.Error(t, err)
}

func TestRepoList(t *testing.T) {
Expand Down