]> source.dussan.org Git - gitea.git/commitdiff
org/members: display 2FA members states + optimize sql requests (#7621)
authorAntoine GIRARD <sapk@users.noreply.github.com>
Fri, 2 Aug 2019 16:06:28 +0000 (18:06 +0200)
committertechknowlogick <techknowlogick@gitea.io>
Fri, 2 Aug 2019 16:06:27 +0000 (12:06 -0400)
* 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

13 files changed:
models/fixtures/org_user.yml
models/fixtures/team.yml
models/fixtures/team_user.yml
models/fixtures/two_factor.yml [new file with mode: 0644]
models/fixtures/user.yml
models/org.go
models/org_team.go
models/user.go
models/user_test.go
models/userlist.go [new file with mode: 0644]
models/userlist_test.go [new file with mode: 0644]
routers/org/members.go
templates/org/member/members.tmpl

index 5bb23571fc26616f1314b4b4ef584dc6df826c6f..385492dd68d11f000cd308fc7547ed6d49636078 100644 (file)
@@ -39,3 +39,9 @@
   uid: 20
   org_id: 19
   is_public: true
+
+-
+  id: 8
+  uid: 24
+  org_id: 25
+  is_public: true
index 2d0dd9cd56ca11dc1c79327d4af41f38d1723a7b..b7265ec49e4099af9bc62e5d5da2efbc0e254ff9 100644 (file)
   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
index e20b5c9684bfffeac55bd695cfeaab2640c20fb3..4fc609791d3962150479ee4b836d6d4d699c73ab 100644 (file)
   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 (file)
index 0000000..d8cb852
--- /dev/null
@@ -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
index ed60e7f5eab06d2880fc8e452e2aca2a4b4ea28f..d89dc3c1267314345fc051a5d1445180ec65c53f 100644 (file)
   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
index d86109de57e10f48b66378c5bd5571c83c04ef14..7032f6698eeb0b24d6311dbd662a025fe807c577 100644 (file)
@@ -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)
 }
index dcf07437403d3fcba09e7b1590c6a903ec8d8543..1786376d02c81832479e2087713de3641338c84e 100644 (file)
@@ -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)
index 1f684a594045a0c1045b573dc70c2b3b39281378..ab29683a7bf14af91d9f2e49f047976fd1d7a6d7 100644 (file)
@@ -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 ''"`
index 10420a143f191d58991d64015fb3ca5ef127c0bb..290253c4b1a0fc7cb18066c337fc0aaaf2b4021f 100644 (file)
@@ -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 (file)
index 0000000..43838a6
--- /dev/null
@@ -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 (file)
index 0000000..ca08cc9
--- /dev/null
@@ -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())
+}
index d65bc2a00844abe7b305b80a727d60c90ec5a1b1..20f80cefcd1fbb96356ed47ff6aa63582e1270f8 100644 (file)
@@ -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 {
index 7f0a763610e3b283922e85f32bd9d0f0f8c855de..9db506ee5bf9d1106a943cdd24de9738e097de8e 100644 (file)
@@ -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}}">
                                                <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}}
                                                        {{$.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">