From df106cf243aee6d33790c8f37483be1359c4713d Mon Sep 17 00:00:00 2001 From: Daniel Jiang Date: Tue, 15 Sep 2020 09:40:08 +0800 Subject: [PATCH] Revert "Store User ID in session instead of the whole user model (#12984)" This reverts commit 6fc0c9d75a3dffba0de3e443f92b434ee4db78cc. Because this erases the AdminRoleInAuth attribute in user model as it is not stored in DB and it will break the admin group of LDAP. Signed-off-by: Daniel Jiang --- src/core/api/base.go | 4 +-- .../middleware/security/auth_proxy_test.go | 16 ++++++------ .../middleware/security/basic_auth_test.go | 23 +---------------- .../middleware/security/security_test.go | 8 ------ src/server/middleware/security/session.go | 21 +++------------- .../middleware/security/session_test.go | 25 ++++--------------- 6 files changed, 21 insertions(+), 76 deletions(-) diff --git a/src/core/api/base.go b/src/core/api/base.go index f29d3d322..3c22aaa38 100644 --- a/src/core/api/base.go +++ b/src/core/api/base.go @@ -35,11 +35,11 @@ import ( "github.com/goharbor/harbor/src/pkg/repository" "github.com/goharbor/harbor/src/pkg/retention" "github.com/goharbor/harbor/src/pkg/scheduler" - sec "github.com/goharbor/harbor/src/server/middleware/security" ) const ( yamlFileContentType = "application/x-yaml" + userSessionKey = "user" ) // the managers/controllers used globally @@ -176,7 +176,7 @@ func (b *BaseController) WriteYamlData(object interface{}) { // PopulateUserSession generates a new session ID and fill the user model in parm to the session func (b *BaseController) PopulateUserSession(u models.User) { b.SessionRegenerateID() - b.SetSession(sec.UserIDSessionKey, u.UserID) + b.SetSession(userSessionKey, u) } // Init related objects/configurations for the API controllers diff --git a/src/server/middleware/security/auth_proxy_test.go b/src/server/middleware/security/auth_proxy_test.go index 863fd5260..32b4ed068 100644 --- a/src/server/middleware/security/auth_proxy_test.go +++ b/src/server/middleware/security/auth_proxy_test.go @@ -17,23 +17,25 @@ package security import ( "encoding/json" "fmt" - "io/ioutil" - "net/http" - "net/http/httptest" - "net/url" - "testing" - "github.com/goharbor/harbor/src/common" "github.com/goharbor/harbor/src/common/models" + "github.com/goharbor/harbor/src/common/utils/test" _ "github.com/goharbor/harbor/src/core/auth/authproxy" "github.com/goharbor/harbor/src/core/config" "github.com/goharbor/harbor/src/lib" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "io/ioutil" "k8s.io/api/authentication/v1beta1" + "net/http" + "net/http/httptest" + "net/url" + "testing" ) func TestAuthProxy(t *testing.T) { + config.Init() + test.InitDatabaseFromEnv() authProxy := &authProxy{} server, err := newAuthProxyTestServer() @@ -47,7 +49,7 @@ func TestAuthProxy(t *testing.T) { common.HTTPAuthProxyTokenReviewEndpoint: server.URL, common.AUTHMode: common.HTTPAuth, } - config.InitWithSettings(c) + config.Upload(c) v, e := config.HTTPAuthProxySetting() require.Nil(t, e) assert.Equal(t, *v, models.HTTPAuthProxy{ diff --git a/src/server/middleware/security/basic_auth_test.go b/src/server/middleware/security/basic_auth_test.go index 8c50ace7f..dd3e23e97 100644 --- a/src/server/middleware/security/basic_auth_test.go +++ b/src/server/middleware/security/basic_auth_test.go @@ -15,39 +15,18 @@ package security import ( - "fmt" - - "github.com/goharbor/harbor/src/common" - "github.com/goharbor/harbor/src/common/dao" - "github.com/goharbor/harbor/src/common/models" _ "github.com/goharbor/harbor/src/core/auth/db" - "github.com/goharbor/harbor/src/core/config" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" - "net/http" "testing" ) func TestBasicAuth(t *testing.T) { - c := map[string]interface{}{ - common.AUTHMode: common.DBAuth, - } - config.InitWithSettings(c) - - user := models.User{ - Username: "tester", - Password: "Harbor12345", - } - uid, err := dao.Register(user) - defer func(id int64) { - sql := fmt.Sprintf("DELETE FROM harbor_user WHERE user_id=%d", id) - dao.ExecuteBatchSQL([]string{sql}) - }(uid) basicAuth := &basicAuth{} req, err := http.NewRequest(http.MethodGet, "http://127.0.0.1/api/projects/", nil) require.Nil(t, err) - req.SetBasicAuth("tester", "Harbor12345") + req.SetBasicAuth("admin", "Harbor12345") ctx := basicAuth.Generate(req) assert.NotNil(t, ctx) } diff --git a/src/server/middleware/security/security_test.go b/src/server/middleware/security/security_test.go index 64a892891..5f2a1f5ed 100644 --- a/src/server/middleware/security/security_test.go +++ b/src/server/middleware/security/security_test.go @@ -15,21 +15,13 @@ package security import ( - "github.com/goharbor/harbor/src/common/dao" "github.com/goharbor/harbor/src/common/security" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" - "net/http" - "os" "testing" ) -func TestMain(m *testing.M) { - dao.PrepareTestForPostgresSQL() - os.Exit(m.Run()) -} - func TestSecurity(t *testing.T) { var ctx security.Context var exist bool diff --git a/src/server/middleware/security/session.go b/src/server/middleware/security/session.go index 637cf4b64..5f6f8467d 100644 --- a/src/server/middleware/security/session.go +++ b/src/server/middleware/security/session.go @@ -19,7 +19,6 @@ import ( "net/http/httptest" "github.com/astaxie/beego" - "github.com/goharbor/harbor/src/common/dao" "github.com/goharbor/harbor/src/common/models" "github.com/goharbor/harbor/src/common/security" "github.com/goharbor/harbor/src/common/security/local" @@ -27,9 +26,6 @@ import ( "github.com/goharbor/harbor/src/lib/log" ) -// UserIDSessionKey is the key for storing user id in session -const UserIDSessionKey = "user_id" - type session struct{} func (s *session) Generate(req *http.Request) security.Context { @@ -39,24 +35,15 @@ func (s *session) Generate(req *http.Request) security.Context { log.Errorf("failed to get the session store for request: %v", err) return nil } - userInterface := store.Get(UserIDSessionKey) + userInterface := store.Get("user") if userInterface == nil { return nil } - uid, ok := userInterface.(int) + user, ok := userInterface.(models.User) if !ok { - log.Warning("can not convert the data in session to user id") - return nil - } - user, err := dao.GetUser(models.User{UserID: uid}) - if err != nil { - log.Errorf("failed to get user info, error: %v, skip generating security context via session", err) - return nil - } - if user == nil { - log.Errorf("the user id from session: %d, not exist in DB", uid) + log.Warning("can not convert the user in session to user model") return nil } log.Debugf("a session security context generated for request %s %s", req.Method, req.URL.Path) - return local.NewSecurityContext(user, config.GlobalProjectMgr) + return local.NewSecurityContext(&user, config.GlobalProjectMgr) } diff --git a/src/server/middleware/security/session_test.go b/src/server/middleware/security/session_test.go index 1a5dead2b..2ca3c3918 100644 --- a/src/server/middleware/security/session_test.go +++ b/src/server/middleware/security/session_test.go @@ -15,15 +15,11 @@ package security import ( - "fmt" - "github.com/astaxie/beego" beegosession "github.com/astaxie/beego/session" - "github.com/goharbor/harbor/src/common/dao" "github.com/goharbor/harbor/src/common/models" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" - "net/http" "net/http/httptest" "path/filepath" @@ -45,31 +41,20 @@ func TestSession(t *testing.T) { beego.GlobalSessions, err = beegosession.NewManager("memory", conf) require.Nil(t, err) - user := &models.User{ - Username: "session_test", - Email: "session_test@example.com", + user := models.User{ + Username: "admin", + UserID: 1, + Email: "admin@example.com", SysAdminFlag: true, } - err = dao.OnBoardUser(user) - require.Nil(t, err) - defer func(id int) { - sql := fmt.Sprintf("DELETE FROM harbor_user WHERE user_id=%d", id) - dao.ExecuteBatchSQL([]string{sql}) - }(user.UserID) - req, err := http.NewRequest(http.MethodGet, "http://127.0.0.1/api/projects/", nil) require.Nil(t, err) store, err := beego.GlobalSessions.SessionStart(httptest.NewRecorder(), req) require.Nil(t, err) - err = store.Set(UserIDSessionKey, 999) + err = store.Set("user", user) require.Nil(t, err) session := &session{} ctx := session.Generate(req) - assert.Nil(t, ctx) - err = store.Set(UserIDSessionKey, user.UserID) - require.Nil(t, err) - ctx = session.Generate(req) assert.NotNil(t, ctx) - }