From d3930ae17c0a044cf3d149c6a7aaef803bfd226a Mon Sep 17 00:00:00 2001 From: stone Date: Thu, 17 May 2018 16:23:51 +0800 Subject: [PATCH] Put user info into session (#4885) Fix the following issues. 1) GroupList is not found in SecurityContext user info 2) Retrieve multiple memberof information from LDAP. 3) If user is in two groups with guest, administrator role separately, display the max privilege role. --- src/common/dao/project.go | 8 +++++- src/common/utils/ldap/ldap.go | 5 +++- src/ui/auth/ldap/ldap.go | 2 ++ src/ui/controllers/base.go | 5 +--- src/ui/filter/security.go | 22 ++++++++-------- src/ui/filter/security_test.go | 47 +++++++++++++++++++++++++++++++--- 6 files changed, 68 insertions(+), 21 deletions(-) diff --git a/src/common/dao/project.go b/src/common/dao/project.go index 5e3165ff2..75e2fc3ed 100644 --- a/src/common/dao/project.go +++ b/src/common/dao/project.go @@ -292,8 +292,10 @@ func GetRolesByLDAPGroup(projectID int64, groupDNCondition string) ([]int, error return roles, nil } o := GetOrmer() + //Because an LDAP user can be memberof multiple groups, + //the role is in descent order (1-admin, 2-developer, 3-guest), use min to select the max privilege role. sql := fmt.Sprintf( - `select pm.role from project_member pm + `select min(pm.role) from project_member pm left join user_group ug on pm.entity_type = 'g' and pm.entity_id = ug.id where ug.ldap_group_dn in ( %s ) and pm.project_id = ? `, groupDNCondition) @@ -302,5 +304,9 @@ func GetRolesByLDAPGroup(projectID int64, groupDNCondition string) ([]int, error log.Warningf("Error in GetRolesByLDAPGroup, error: %v", err) return nil, err } + //If there is no row selected, the min returns an empty row, to avoid return 0 as role + if len(roles) == 1 && roles[0] == 0 { + return []int{}, nil + } return roles, nil } diff --git a/src/common/utils/ldap/ldap.go b/src/common/utils/ldap/ldap.go index 505b3f277..5ca2fbacd 100644 --- a/src/common/utils/ldap/ldap.go +++ b/src/common/utils/ldap/ldap.go @@ -202,7 +202,10 @@ func (session *Session) SearchUser(username string) ([]models.LdapUser, error) { case "email": u.Email = val case "memberof": - groupDNList = append(groupDNList, val) + for _, dnItem := range attr.Values { + groupDNList = append(groupDNList, strings.TrimSpace(dnItem)) + log.Debugf("Found memberof %v", dnItem) + } } u.GroupDNList = groupDNList } diff --git a/src/ui/auth/ldap/ldap.go b/src/ui/auth/ldap/ldap.go index 26a28fdcb..f530935c0 100644 --- a/src/ui/auth/ldap/ldap.go +++ b/src/ui/auth/ldap/ldap.go @@ -59,6 +59,8 @@ func (l *Auth) Authenticate(m models.AuthModel) (*models.User, error) { ldapUsers, err := ldapSession.SearchUser(p) + log.Debugf("Found ldap user %+v", ldapUsers[0]) + if err != nil { log.Warningf("ldap search fail: %v", err) return nil, err diff --git a/src/ui/controllers/base.go b/src/ui/controllers/base.go index 79d1b3bf1..8cdad8463 100644 --- a/src/ui/controllers/base.go +++ b/src/ui/controllers/base.go @@ -69,10 +69,7 @@ func (cc *CommonController) Login() { if user == nil { cc.CustomAbort(http.StatusUnauthorized, "") } - - cc.SetSession("userId", user.UserID) - cc.SetSession("username", user.Username) - cc.SetSession("isSysAdmin", user.HasAdminRole) + cc.SetSession("user", *user) } // LogOut Habor UI diff --git a/src/ui/filter/security.go b/src/ui/filter/security.go index de6c3d733..afa6fc851 100644 --- a/src/ui/filter/security.go +++ b/src/ui/filter/security.go @@ -215,7 +215,6 @@ func (b *basicAuthReqCtxModifier) Modify(ctx *beegoctx.Context) bool { pm := config.GlobalProjectMgr log.Debug("creating local database security context...") securCtx := local.NewSecurityContext(user, pm) - setSecurCtxAndPM(ctx.Request, securCtx, pm) return true } @@ -223,24 +222,25 @@ func (b *basicAuthReqCtxModifier) Modify(ctx *beegoctx.Context) bool { type sessionReqCtxModifier struct{} func (s *sessionReqCtxModifier) Modify(ctx *beegoctx.Context) bool { - username := ctx.Input.Session("username") - if username == nil { + var user models.User + userInterface := ctx.Input.Session("user") + + if userInterface == nil { + log.Debug("can not get user information from session") return false } log.Debug("got user information from session") - user := &models.User{ - Username: username.(string), + user, ok := userInterface.(models.User) + if !ok { + log.Info("can not get user information from session") + return false } - isSysAdmin := ctx.Input.Session("isSysAdmin") - if isSysAdmin != nil && isSysAdmin.(bool) { - user.HasAdminRole = true - } - + log.Debug("Getting user %+v", user) log.Debug("using local database project manager") pm := config.GlobalProjectMgr log.Debug("creating local database security context...") - securCtx := local.NewSecurityContext(user, pm) + securCtx := local.NewSecurityContext(&user, pm) setSecurCtxAndPM(ctx.Request, securCtx, pm) diff --git a/src/ui/filter/security_test.go b/src/ui/filter/security_test.go index e581f7ea1..d1e7818d1 100644 --- a/src/ui/filter/security_test.go +++ b/src/ui/filter/security_test.go @@ -30,6 +30,7 @@ import ( "github.com/astaxie/beego/session" "github.com/stretchr/testify/assert" "github.com/vmware/harbor/src/common/dao" + "github.com/vmware/harbor/src/common/models" commonsecret "github.com/vmware/harbor/src/common/secret" "github.com/vmware/harbor/src/common/security" "github.com/vmware/harbor/src/common/security/local" @@ -146,6 +147,12 @@ func TestBasicAuthReqCtxModifier(t *testing.T) { } func TestSessionReqCtxModifier(t *testing.T) { + user := models.User{ + Username: "admin", + UserID: 1, + Email: "admin@example.com", + HasAdminRole: true, + } req, err := http.NewRequest(http.MethodGet, "http://127.0.0.1/api/projects/", nil) if err != nil { @@ -155,10 +162,7 @@ func TestSessionReqCtxModifier(t *testing.T) { if err != nil { t.Fatalf("failed to create session store: %v", err) } - if err = store.Set("username", "admin"); err != nil { - t.Fatalf("failed to set session: %v", err) - } - if err = store.Set("isSysAdmin", true); err != nil { + if err = store.Set("user", user); err != nil { t.Fatalf("failed to set session: %v", err) } @@ -184,6 +188,41 @@ func TestSessionReqCtxModifier(t *testing.T) { assert.Equal(t, "admin", s.GetUsername()) assert.True(t, s.IsSysAdmin()) assert.NotNil(t, projectManager(ctx)) + +} + +func TestSessionReqCtxModifierFailed(t *testing.T) { + user := "admin" + req, err := http.NewRequest(http.MethodGet, + "http://127.0.0.1/api/projects/", nil) + if err != nil { + t.Fatalf("failed to create request: %v", req) + } + store, err := beego.GlobalSessions.SessionStart(httptest.NewRecorder(), req) + if err != nil { + t.Fatalf("failed to create session store: %v", err) + } + if err = store.Set("user", user); err != nil { + t.Fatalf("failed to set session: %v", err) + } + + req, err = http.NewRequest(http.MethodGet, + "http://127.0.0.1/api/projects/", nil) + if err != nil { + t.Fatalf("failed to create request: %v", req) + } + addSessionIDToCookie(req, store.SessionID()) + + ctx, err := newContext(req) + if err != nil { + t.Fatalf("failed to crate context: %v", err) + } + + modifier := &sessionReqCtxModifier{} + modified := modifier.Modify(ctx) + + assert.False(t, modified) + } // TODO add test case