Skip to content

Commit

Permalink
Fix session key conflict with database keyword (#28613)
Browse files Browse the repository at this point in the history
This is a regression from #28220 .
`builder.Cond` will not add `` ` `` automatically but xorm method
`Get/Find` adds `` ` ``.

This PR also adds tests to prevent the method from being implemented
incorrectly. The tests are added in `integrations` to test every
database.
  • Loading branch information
lunny authored Dec 27, 2023
1 parent a1dfffd commit 4c29c75
Show file tree
Hide file tree
Showing 2 changed files with 47 additions and 7 deletions.
17 changes: 10 additions & 7 deletions models/auth/session.go
Original file line number Diff line number Diff line change
Expand Up @@ -41,12 +41,15 @@ func ReadSession(ctx context.Context, key string) (*Session, error) {
}
defer committer.Close()

session, exist, err := db.Get[Session](ctx, builder.Eq{"key": key})
session, exist, err := db.Get[Session](ctx, builder.Eq{"`key`": key})
if err != nil {
return nil, err
} else if !exist {
session.Expiry = timeutil.TimeStampNow()
if err := db.Insert(ctx, &session); err != nil {
session = &Session{
Key: key,
Expiry: timeutil.TimeStampNow(),
}
if err := db.Insert(ctx, session); err != nil {
return nil, err
}
}
Expand All @@ -56,7 +59,7 @@ func ReadSession(ctx context.Context, key string) (*Session, error) {

// ExistSession checks if a session exists
func ExistSession(ctx context.Context, key string) (bool, error) {
return db.Exist[Session](ctx, builder.Eq{"key": key})
return db.Exist[Session](ctx, builder.Eq{"`key`": key})
}

// DestroySession destroys a session
Expand All @@ -75,13 +78,13 @@ func RegenerateSession(ctx context.Context, oldKey, newKey string) (*Session, er
}
defer committer.Close()

if has, err := db.Exist[Session](ctx, builder.Eq{"key": newKey}); err != nil {
if has, err := db.Exist[Session](ctx, builder.Eq{"`key`": newKey}); err != nil {
return nil, err
} else if has {
return nil, fmt.Errorf("session Key: %s already exists", newKey)
}

if has, err := db.Exist[Session](ctx, builder.Eq{"key": oldKey}); err != nil {
if has, err := db.Exist[Session](ctx, builder.Eq{"`key`": oldKey}); err != nil {
return nil, err
} else if !has {
if err := db.Insert(ctx, &Session{
Expand All @@ -96,7 +99,7 @@ func RegenerateSession(ctx context.Context, oldKey, newKey string) (*Session, er
return nil, err
}

s, _, err := db.Get[Session](ctx, builder.Eq{"key": newKey})
s, _, err := db.Get[Session](ctx, builder.Eq{"`key`": newKey})
if err != nil {
// is not exist, it should be impossible
return nil, err
Expand Down
37 changes: 37 additions & 0 deletions tests/integration/session_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
// Copyright 2023 The Gitea Authors. All rights reserved.
// SPDX-License-Identifier: MIT

package integration

import (
"testing"

"code.gitea.io/gitea/models/auth"
"code.gitea.io/gitea/models/db"
"code.gitea.io/gitea/models/unittest"
"code.gitea.io/gitea/tests"

"github.com/stretchr/testify/assert"
)

func Test_RegenerateSession(t *testing.T) {
defer tests.PrepareTestEnv(t)()

assert.NoError(t, unittest.PrepareTestDatabase())

key := "new_key890123456" // it must be 16 characters long
key2 := "new_key890123457" // it must be 16 characters
exist, err := auth.ExistSession(db.DefaultContext, key)
assert.NoError(t, err)
assert.False(t, exist)

sess, err := auth.RegenerateSession(db.DefaultContext, "", key)
assert.NoError(t, err)
assert.EqualValues(t, key, sess.Key)
assert.Len(t, sess.Data, 0)

sess, err = auth.ReadSession(db.DefaultContext, key2)
assert.NoError(t, err)
assert.EqualValues(t, key2, sess.Key)
assert.Len(t, sess.Data, 0)
}

0 comments on commit 4c29c75

Please sign in to comment.