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

@@ -39,3 +39,9 @@
   uid: 20
   org_id: 19
   is_public: true
+
+-
+  id: 8
+  uid: 24
+  org_id: 25
+  is_public: true

models/fixtures/team.yml

@@ -77,4 +77,13 @@
   name: review_team
   authorize: 1 # read
   num_repos: 1
-  num_members: 1
+  num_members: 1
+
+-
+  id: 10
+  org_id: 25
+  lower_name: notowners
+  name: NotOwners
+  authorize: 1 # owner
+  num_repos: 0
+  num_members: 1

models/fixtures/team_user.yml

@@ -62,4 +62,10 @@
   id: 11
   org_id: 17
   team_id: 9
-  uid: 20
+  uid: 20
+
+-
+  id: 12
+  org_id: 25
+  team_id: 10
+  uid: 24

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

models/fixtures/user.yml

@@ -365,4 +365,39 @@
   is_active: true
   num_members: 0
   num_teams: 0
-  visibility: 2
+  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

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{
+	ownerTeam, err := getOwnerTeam(e, orgID)
-		OrgID: orgID,
+	if err != nil {
-		Name:  ownerTeamName,
+		if err == ErrTeamNotExist {
-	}
+			log.Error("Organization does not have owner team: %d", orgID)
-	if has, err := e.Get(ownerTeam); err != nil {
+			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)
 }

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)

models/user.go

@@ -139,11 +139,12 @@ type User struct {
 	NumRepos     int
 
 	// For organization
-	NumTeams   int
+	NumTeams        int
-	NumMembers int
+	NumMembers      int
-	Teams      []*Team             `xorm:"-"`
+	Teams           []*Team             `xorm:"-"`
-	Members    []*User             `xorm:"-"`
+	Members         UserList            `xorm:"-"`
-	Visibility structs.VisibleType `xorm:"NOT NULL DEFAULT 0"`
+	MembersIsPublic map[int64]bool      `xorm:"-"`
+	Visibility      structs.VisibleType `xorm:"NOT NULL DEFAULT 0"`
 
 	// Preferences
 	DiffViewStyle string `xorm:"NOT NULL DEFAULT ''"`

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})

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
+}

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())
+}

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 {

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">