From 76408d50fb338e9239ee06bb26eec28453167300 Mon Sep 17 00:00:00 2001
From: Antoine GIRARD <sapk@users.noreply.github.com>
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 5bb23571fc..385492dd68 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 2d0dd9cd56..b7265ec49e 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 e20b5c9684..4fc609791d 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 0000000000..d8cb85274b
--- /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 ed60e7f5ea..d89dc3c126 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 d86109de57..7032f6698e 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 dcf0743740..1786376d02 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 1f684a5940..ab29683a7b 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 10420a143f..290253c4b1 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 0000000000..43838a6804
--- /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 0000000000..ca08cc90ce
--- /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 d65bc2a008..20f80cefcd 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 7f0a763610..9db506ee5b 100644
--- a/templates/org/member/members.tmpl
+++ b/templates/org/member/members.tmpl
@@ -5,7 +5,7 @@
{{template "base/alert" .}}
<div class="list">
- {{range .Members}}
+ {{ range .Members}}
<div class="item ui grid">
<div class="ui one wide column">
<img class="ui avatar" src="{{.SizedRelAvatarLink 48}}">
@@ -14,12 +14,12 @@
<div class="meta"><a href="{{.HomeLink}}">{{.Name}}</a></div>
<div class="meta">{{.FullName}}</div>
</div>
- <div class="ui five wide column center">
+ <div class="ui four wide column center">
<div class="meta">
{{$.i18n.Tr "org.members.membership_visibility"}}
</div>
<div class="meta">
- {{ $isPublic := .IsPublicMember $.Org.ID}}
+ {{ $isPublic := index $.MembersIsPublicMember .ID}}
{{if $isPublic}}
<strong>{{$.i18n.Tr "org.members.public"}}</strong>
{{if or (eq $.SignedUser.ID .ID) $.IsOrganizationOwner}}(<a href="{{$.OrgLink}}/members/action/private?uid={{.ID}}">{{$.i18n.Tr "org.members.public_helper"}}</a>){{end}}
@@ -34,7 +34,15 @@
{{$.i18n.Tr "org.members.member_role"}}
</div>
<div class="meta">
- <strong>{{if .IsUserOrgOwner $.Org.ID}}<span class="octicon octicon-shield"></span> {{$.i18n.Tr "org.members.owner"}}{{else}}{{$.i18n.Tr "org.members.member"}}{{end}}</strong>
+ <strong>{{if index $.MembersIsUserOrgOwner .ID}}<span class="octicon octicon-shield"></span> {{$.i18n.Tr "org.members.owner"}}{{else}}{{$.i18n.Tr "org.members.member"}}{{end}}</strong>
+ </div>
+ </div>
+ <div class="ui one wide column center">
+ <div class="meta">
+ 2FA
+ </div>
+ <div class="meta">
+ <strong><span class="octicon {{if index $.MembersTwoFaStatus .ID}}octicon-check text green{{else}}octicon-x{{end}}"></span></strong>
</div>
</div>
<div class="ui four wide column">