From 76408d50fb338e9239ee06bb26eec28453167300 Mon Sep 17 00:00:00 2001 From: Antoine GIRARD Date: Fri, 2 Aug 2019 18:06:28 +0200 Subject: [PATCH] org/members: display 2FA members states + optimize sql requests (#7621) * org/members: display 2FA state * fix comment typo * lay down UserList bases * add basic test for previous methods * add comment for UserList type * add valid two-fa account * test new UserList methods * optimize MembersIsPublic by side loading info on GetMembers + fix integrations tests * respect fmt rules * use map for data * Optimize GetTwoFaStatus * rewrite by using existing sub func * Optimize IsUserOrgOwner * remove un-used code * tests: cover empty org + fix import order * tests: add ErrTeamNotExist path * tests: fix wrong expected result --- models/fixtures/org_user.yml | 6 ++ models/fixtures/team.yml | 11 +++- models/fixtures/team_user.yml | 8 ++- models/fixtures/two_factor.yml | 9 +++ models/fixtures/user.yml | 37 +++++++++++- models/org.go | 17 +++--- models/org_team.go | 5 ++ models/user.go | 11 ++-- models/user_test.go | 59 ++++++++++++++++++- models/userlist.go | 95 +++++++++++++++++++++++++++++++ models/userlist_test.go | 90 +++++++++++++++++++++++++++++ routers/org/members.go | 7 ++- templates/org/member/members.tmpl | 16 ++++-- 13 files changed, 346 insertions(+), 25 deletions(-) create mode 100644 models/fixtures/two_factor.yml create mode 100644 models/userlist.go create mode 100644 models/userlist_test.go diff --git a/models/fixtures/org_user.yml b/models/fixtures/org_user.yml index 5bb23571fc26..385492dd68d1 100644 --- a/models/fixtures/org_user.yml +++ b/models/fixtures/org_user.yml @@ -39,3 +39,9 @@ uid: 20 org_id: 19 is_public: true + +- + id: 8 + uid: 24 + org_id: 25 + is_public: true diff --git a/models/fixtures/team.yml b/models/fixtures/team.yml index 2d0dd9cd56ca..b7265ec49e40 100644 --- a/models/fixtures/team.yml +++ b/models/fixtures/team.yml @@ -77,4 +77,13 @@ name: review_team authorize: 1 # read num_repos: 1 - num_members: 1 \ No newline at end of file + num_members: 1 + +- + id: 10 + org_id: 25 + lower_name: notowners + name: NotOwners + authorize: 1 # owner + num_repos: 0 + num_members: 1 diff --git a/models/fixtures/team_user.yml b/models/fixtures/team_user.yml index e20b5c9684bf..4fc609791d39 100644 --- a/models/fixtures/team_user.yml +++ b/models/fixtures/team_user.yml @@ -62,4 +62,10 @@ id: 11 org_id: 17 team_id: 9 - uid: 20 \ No newline at end of file + uid: 20 + +- + id: 12 + org_id: 25 + team_id: 10 + uid: 24 diff --git a/models/fixtures/two_factor.yml b/models/fixtures/two_factor.yml new file mode 100644 index 000000000000..d8cb85274b68 --- /dev/null +++ b/models/fixtures/two_factor.yml @@ -0,0 +1,9 @@ +- + id: 1 + uid: 24 + secret: KlDporn6Ile4vFcKI8z7Z6sqK1Scj2Qp0ovtUzCZO6jVbRW2lAoT7UDxDPtrab8d2B9zKOocBRdBJnS8orsrUNrsyETY+jJHb79M82uZRioKbRUz15sfOpmJmEzkFeSg6S4LicUBQos= + scratch_salt: Qb5bq2DyR2 + scratch_hash: 068eb9b8746e0bcfe332fac4457693df1bda55800eb0f6894d14ebb736ae6a24e0fc8fc5333c19f57f81599788f0b8e51ec1 + last_used_passcode: + created_unix: 1564253724 + updated_unix: 1564253724 diff --git a/models/fixtures/user.yml b/models/fixtures/user.yml index ed60e7f5eab0..d89dc3c12673 100644 --- a/models/fixtures/user.yml +++ b/models/fixtures/user.yml @@ -365,4 +365,39 @@ is_active: true num_members: 0 num_teams: 0 - visibility: 2 \ No newline at end of file + visibility: 2 + +- + id: 24 + lower_name: user24 + name: user24 + full_name: "user24" + email: user24@example.com + keep_email_private: true + passwd: 7d93daa0d1e6f2305cc8fa496847d61dc7320bb16262f9c55dd753480207234cdd96a93194e408341971742f4701772a025a # password + type: 0 # individual + salt: ZogKvWdyEx + is_admin: false + avatar: avatar24 + avatar_email: user24@example.com + num_repos: 0 + num_stars: 0 + num_followers: 0 + num_following: 0 + is_active: true + +- + id: 25 + lower_name: org25 + name: org25 + full_name: "org25" + email: org25@example.com + passwd: 7d93daa0d1e6f2305cc8fa496847d61dc7320bb16262f9c55dd753480207234cdd96a93194e408341971742f4701772a025a # password + type: 1 # organization + salt: ZogKvWdyEx + is_admin: false + avatar: avatar25 + avatar_email: org25@example.com + num_repos: 0 + num_members: 1 + num_teams: 1 diff --git a/models/org.go b/models/org.go index d86109de57e1..7032f6698eeb 100644 --- a/models/org.go +++ b/models/org.go @@ -72,9 +72,12 @@ func (org *User) GetMembers() error { } var ids = make([]int64, len(ous)) + var idsIsPublic = make(map[int64]bool, len(ous)) for i, ou := range ous { ids[i] = ou.UID + idsIsPublic[ou.UID] = ou.IsPublic } + org.MembersIsPublic = idsIsPublic org.Members, err = GetUsersByIDs(ids) return err } @@ -298,15 +301,13 @@ type OrgUser struct { } func isOrganizationOwner(e Engine, orgID, uid int64) (bool, error) { - ownerTeam := &Team{ - OrgID: orgID, - Name: ownerTeamName, - } - if has, err := e.Get(ownerTeam); err != nil { + ownerTeam, err := getOwnerTeam(e, orgID) + if err != nil { + if err == ErrTeamNotExist { + log.Error("Organization does not have owner team: %d", orgID) + return false, nil + } return false, err - } else if !has { - log.Error("Organization does not have owner team: %d", orgID) - return false, nil } return isTeamMember(e, orgID, ownerTeam.ID, uid) } diff --git a/models/org_team.go b/models/org_team.go index dcf07437403d..1786376d02c8 100644 --- a/models/org_team.go +++ b/models/org_team.go @@ -362,6 +362,11 @@ func GetTeam(orgID int64, name string) (*Team, error) { return getTeam(x, orgID, name) } +// getOwnerTeam returns team by given team name and organization. +func getOwnerTeam(e Engine, orgID int64) (*Team, error) { + return getTeam(e, orgID, ownerTeamName) +} + func getTeamByID(e Engine, teamID int64) (*Team, error) { t := new(Team) has, err := e.ID(teamID).Get(t) diff --git a/models/user.go b/models/user.go index 1f684a594045..ab29683a7bf1 100644 --- a/models/user.go +++ b/models/user.go @@ -139,11 +139,12 @@ type User struct { NumRepos int // For organization - NumTeams int - NumMembers int - Teams []*Team `xorm:"-"` - Members []*User `xorm:"-"` - Visibility structs.VisibleType `xorm:"NOT NULL DEFAULT 0"` + NumTeams int + NumMembers int + Teams []*Team `xorm:"-"` + Members UserList `xorm:"-"` + MembersIsPublic map[int64]bool `xorm:"-"` + Visibility structs.VisibleType `xorm:"NOT NULL DEFAULT 0"` // Preferences DiffViewStyle string `xorm:"NOT NULL DEFAULT ''"` diff --git a/models/user_test.go b/models/user_test.go index 10420a143f19..290253c4b1a0 100644 --- a/models/user_test.go +++ b/models/user_test.go @@ -5,6 +5,7 @@ package models import ( + "fmt" "math/rand" "strings" "testing" @@ -15,6 +16,58 @@ import ( "github.com/stretchr/testify/assert" ) +func TestUserIsPublicMember(t *testing.T) { + assert.NoError(t, PrepareTestDatabase()) + + tt := []struct { + uid int64 + orgid int64 + expected bool + }{ + {2, 3, true}, + {4, 3, false}, + {5, 6, true}, + {5, 7, false}, + } + for _, v := range tt { + t.Run(fmt.Sprintf("UserId%dIsPublicMemberOf%d", v.uid, v.orgid), func(t *testing.T) { + testUserIsPublicMember(t, v.uid, v.orgid, v.expected) + }) + } +} + +func testUserIsPublicMember(t *testing.T, uid int64, orgID int64, expected bool) { + user, err := GetUserByID(uid) + assert.NoError(t, err) + assert.Equal(t, expected, user.IsPublicMember(orgID)) +} + +func TestIsUserOrgOwner(t *testing.T) { + assert.NoError(t, PrepareTestDatabase()) + + tt := []struct { + uid int64 + orgid int64 + expected bool + }{ + {2, 3, true}, + {4, 3, false}, + {5, 6, true}, + {5, 7, true}, + } + for _, v := range tt { + t.Run(fmt.Sprintf("UserId%dIsOrgOwnerOf%d", v.uid, v.orgid), func(t *testing.T) { + testIsUserOrgOwner(t, v.uid, v.orgid, v.expected) + }) + } +} + +func testIsUserOrgOwner(t *testing.T, uid int64, orgID int64, expected bool) { + user, err := GetUserByID(uid) + assert.NoError(t, err) + assert.Equal(t, expected, user.IsUserOrgOwner(orgID)) +} + func TestGetUserEmailsByNames(t *testing.T) { assert.NoError(t, PrepareTestDatabase()) @@ -83,7 +136,7 @@ func TestSearchUsers(t *testing.T) { []int64{7, 17}) testOrgSuccess(&SearchUserOptions{OrderBy: "id ASC", Page: 3, PageSize: 2}, - []int64{19}) + []int64{19, 25}) testOrgSuccess(&SearchUserOptions{Page: 4, PageSize: 2}, []int64{}) @@ -95,13 +148,13 @@ func TestSearchUsers(t *testing.T) { } testUserSuccess(&SearchUserOptions{OrderBy: "id ASC", Page: 1}, - []int64{1, 2, 4, 5, 8, 9, 10, 11, 12, 13, 14, 15, 16, 18, 20, 21}) + []int64{1, 2, 4, 5, 8, 9, 10, 11, 12, 13, 14, 15, 16, 18, 20, 21, 24}) testUserSuccess(&SearchUserOptions{Page: 1, IsActive: util.OptionalBoolFalse}, []int64{9}) testUserSuccess(&SearchUserOptions{OrderBy: "id ASC", Page: 1, IsActive: util.OptionalBoolTrue}, - []int64{1, 2, 4, 5, 8, 10, 11, 12, 13, 14, 15, 16, 18, 20, 21}) + []int64{1, 2, 4, 5, 8, 10, 11, 12, 13, 14, 15, 16, 18, 20, 21, 24}) testUserSuccess(&SearchUserOptions{Keyword: "user1", OrderBy: "id ASC", Page: 1, IsActive: util.OptionalBoolTrue}, []int64{1, 10, 11, 12, 13, 14, 15, 16, 18}) diff --git a/models/userlist.go b/models/userlist.go new file mode 100644 index 000000000000..43838a680441 --- /dev/null +++ b/models/userlist.go @@ -0,0 +1,95 @@ +// Copyright 2019 The Gitea Authors. All rights reserved. +// Use of this source code is governed by a MIT-style +// license that can be found in the LICENSE file. + +package models + +import ( + "fmt" + + "code.gitea.io/gitea/modules/log" +) + +//UserList is a list of user. +// This type provide valuable methods to retrieve information for a group of users efficiently. +type UserList []*User + +func (users UserList) getUserIDs() []int64 { + userIDs := make([]int64, len(users)) + for _, user := range users { + userIDs = append(userIDs, user.ID) //Considering that user id are unique in the list + } + return userIDs +} + +// IsUserOrgOwner returns true if user is in the owner team of given organization. +func (users UserList) IsUserOrgOwner(orgID int64) map[int64]bool { + results := make(map[int64]bool, len(users)) + for _, user := range users { + results[user.ID] = false //Set default to false + } + ownerMaps, err := users.loadOrganizationOwners(x, orgID) + if err == nil { + for _, owner := range ownerMaps { + results[owner.UID] = true + } + } + return results +} + +func (users UserList) loadOrganizationOwners(e Engine, orgID int64) (map[int64]*TeamUser, error) { + if len(users) == 0 { + return nil, nil + } + ownerTeam, err := getOwnerTeam(e, orgID) + if err != nil { + if err == ErrTeamNotExist { + log.Error("Organization does not have owner team: %d", orgID) + return nil, nil + } + return nil, err + } + + userIDs := users.getUserIDs() + ownerMaps := make(map[int64]*TeamUser) + err = e.In("uid", userIDs). + And("org_id=?", orgID). + And("team_id=?", ownerTeam.ID). + Find(&ownerMaps) + if err != nil { + return nil, fmt.Errorf("find team users: %v", err) + } + return ownerMaps, nil +} + +// GetTwoFaStatus return state of 2FA enrollement +func (users UserList) GetTwoFaStatus() map[int64]bool { + results := make(map[int64]bool, len(users)) + for _, user := range users { + results[user.ID] = false //Set default to false + } + tokenMaps, err := users.loadTwoFactorStatus(x) + if err == nil { + for _, token := range tokenMaps { + results[token.UID] = true + } + } + + return results +} + +func (users UserList) loadTwoFactorStatus(e Engine) (map[int64]*TwoFactor, error) { + if len(users) == 0 { + return nil, nil + } + + userIDs := users.getUserIDs() + tokenMaps := make(map[int64]*TwoFactor, len(userIDs)) + err := e. + In("uid", userIDs). + Find(&tokenMaps) + if err != nil { + return nil, fmt.Errorf("find two factor: %v", err) + } + return tokenMaps, nil +} diff --git a/models/userlist_test.go b/models/userlist_test.go new file mode 100644 index 000000000000..ca08cc90ce89 --- /dev/null +++ b/models/userlist_test.go @@ -0,0 +1,90 @@ +// Copyright 2019 The Gitea Authors. All rights reserved. +// Use of this source code is governed by a MIT-style +// license that can be found in the LICENSE file. + +package models + +import ( + "fmt" + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestUserListIsPublicMember(t *testing.T) { + assert.NoError(t, PrepareTestDatabase()) + tt := []struct { + orgid int64 + expected map[int64]bool + }{ + {3, map[int64]bool{2: true, 4: false}}, + {6, map[int64]bool{5: true}}, + {7, map[int64]bool{5: false}}, + {25, map[int64]bool{24: true}}, + {22, map[int64]bool{}}, + } + for _, v := range tt { + t.Run(fmt.Sprintf("IsPublicMemberOfOrdIg%d", v.orgid), func(t *testing.T) { + testUserListIsPublicMember(t, v.orgid, v.expected) + }) + } +} +func testUserListIsPublicMember(t *testing.T, orgID int64, expected map[int64]bool) { + org, err := GetUserByID(orgID) + assert.NoError(t, err) + assert.NoError(t, org.GetMembers()) + assert.Equal(t, expected, org.MembersIsPublic) + +} + +func TestUserListIsUserOrgOwner(t *testing.T) { + assert.NoError(t, PrepareTestDatabase()) + tt := []struct { + orgid int64 + expected map[int64]bool + }{ + {3, map[int64]bool{2: true, 4: false}}, + {6, map[int64]bool{5: true}}, + {7, map[int64]bool{5: true}}, + {25, map[int64]bool{24: false}}, // ErrTeamNotExist + {22, map[int64]bool{}}, // No member + } + for _, v := range tt { + t.Run(fmt.Sprintf("IsUserOrgOwnerOfOrdIg%d", v.orgid), func(t *testing.T) { + testUserListIsUserOrgOwner(t, v.orgid, v.expected) + }) + } +} + +func testUserListIsUserOrgOwner(t *testing.T, orgID int64, expected map[int64]bool) { + org, err := GetUserByID(orgID) + assert.NoError(t, err) + assert.NoError(t, org.GetMembers()) + assert.Equal(t, expected, org.Members.IsUserOrgOwner(orgID)) +} + +func TestUserListIsTwoFaEnrolled(t *testing.T) { + assert.NoError(t, PrepareTestDatabase()) + tt := []struct { + orgid int64 + expected map[int64]bool + }{ + {3, map[int64]bool{2: false, 4: false}}, + {6, map[int64]bool{5: false}}, + {7, map[int64]bool{5: false}}, + {25, map[int64]bool{24: true}}, + {22, map[int64]bool{}}, + } + for _, v := range tt { + t.Run(fmt.Sprintf("IsTwoFaEnrolledOfOrdIg%d", v.orgid), func(t *testing.T) { + testUserListIsTwoFaEnrolled(t, v.orgid, v.expected) + }) + } +} + +func testUserListIsTwoFaEnrolled(t *testing.T, orgID int64, expected map[int64]bool) { + org, err := GetUserByID(orgID) + assert.NoError(t, err) + assert.NoError(t, org.GetMembers()) + assert.Equal(t, expected, org.Members.GetTwoFaStatus()) +} diff --git a/routers/org/members.go b/routers/org/members.go index d65bc2a00844..20f80cefcd1f 100644 --- a/routers/org/members.go +++ b/routers/org/members.go @@ -19,7 +19,7 @@ const ( tplMembers base.TplName = "org/member/members" ) -// Members render orgnization users page +// Members render organization users page func Members(ctx *context.Context) { org := ctx.Org.Organization ctx.Data["Title"] = org.FullName @@ -30,11 +30,14 @@ func Members(ctx *context.Context) { return } ctx.Data["Members"] = org.Members + ctx.Data["MembersIsPublicMember"] = org.MembersIsPublic + ctx.Data["MembersIsUserOrgOwner"] = org.Members.IsUserOrgOwner(org.ID) + ctx.Data["MembersTwoFaStatus"] = org.Members.GetTwoFaStatus() ctx.HTML(200, tplMembers) } -// MembersAction response for operation to a member of orgnization +// MembersAction response for operation to a member of organization func MembersAction(ctx *context.Context) { uid := com.StrTo(ctx.Query("uid")).MustInt64() if uid == 0 { diff --git a/templates/org/member/members.tmpl b/templates/org/member/members.tmpl index 7f0a763610e3..9db506ee5bf9 100644 --- a/templates/org/member/members.tmpl +++ b/templates/org/member/members.tmpl @@ -5,7 +5,7 @@ {{template "base/alert" .}}
- {{range .Members}} + {{ range .Members}}
@@ -14,12 +14,12 @@
{{.Name}}
{{.FullName}}
-
+
{{$.i18n.Tr "org.members.membership_visibility"}}
- {{ $isPublic := .IsPublicMember $.Org.ID}} + {{ $isPublic := index $.MembersIsPublicMember .ID}} {{if $isPublic}} {{$.i18n.Tr "org.members.public"}} {{if or (eq $.SignedUser.ID .ID) $.IsOrganizationOwner}}({{$.i18n.Tr "org.members.public_helper"}}){{end}} @@ -34,7 +34,15 @@ {{$.i18n.Tr "org.members.member_role"}}
- {{if .IsUserOrgOwner $.Org.ID}} {{$.i18n.Tr "org.members.owner"}}{{else}}{{$.i18n.Tr "org.members.member"}}{{end}} + {{if index $.MembersIsUserOrgOwner .ID}} {{$.i18n.Tr "org.members.owner"}}{{else}}{{$.i18n.Tr "org.members.member"}}{{end}} +
+
+
+
+ 2FA +
+
+