Team access changes for editors when editorsCanAdmin is enabled (#45405)

* filter teams for editors to only show the teams that they are members of * frontend changes to only allow clicking on teams that the user can edit * update frontend test snapshots * extend docs * reword * remove the comment for now * Update backend tests * reword the warning, and add it back in * docs feedback Co-authored-by: gamab <gabi.mabs@gmail.com>
3 years ago · 11433cba97
parent d718ee1918
commit 11433cba97
6 changed files with 162 additions and 93 deletions
--- a/docs/sources/http_api/team.md
+++ b/docs/sources/http_api/team.md
@ -13,7 +13,8 @@ Access to these API endpoints is restricted as follows:

 - All authenticated users are able to view details of teams they are a member of.
 - Organization Admins are able to manage all teams and team members.
- If the `editors_can_admin` configuration flag is enabled, Organization Editors are able to view details of all teams and to manage teams that they are Admin members of.
+- If you enable `editors_can_admin` configuration flag, then Organization Editors can create teams and manage teams where they are Admin.
+  - If you enable `editors_can_admin` configuration flag, Editors can find out whether a team that they are not members of exists by trying to create a team with the same name.

 > If you are running Grafana Enterprise and have [Fine-grained access control]({{< relref "../enterprise/access-control/_index.md" >}}) enabled, access to endpoints will be controlled by Fine-grained access control permissions.
 > Refer to specific endpoints to understand what permissions are required.
--- a/pkg/api/team.go
+++ b/pkg/api/team.go
@ -133,7 +133,7 @@ func (hs *HTTPServer) SearchTeams(c *models.ReqContext) response.Response {
 	// Using accesscontrol the filtering is done based on user permissions
 	userIdFilter := models.FilterIgnoreUser
 	if !hs.Features.IsEnabled(featuremgmt.FlagAccesscontrol) {
-		userIdFilter = userFilter(hs.Cfg.EditorsCanAdmin, c)
+		userIdFilter = userFilter(c)
 	}

 	query := models.SearchTeamsQuery{
@ -189,14 +189,12 @@ func (hs *HTTPServer) getTeamAccessControlMetadata(c *models.ReqContext, teamID

 // UserFilter returns the user ID used in a filter when querying a team
 // 1. If the user is a viewer or editor, this will return the user's ID.
-// 2. If EditorsCanAdmin is enabled and the user is an editor, this will return models.FilterIgnoreUser (0)
-// 3. If the user is an admin, this will return models.FilterIgnoreUser (0)
-func userFilter(editorsCanAdmin bool, c *models.ReqContext) int64 {
+// 2. If the user is an admin, this will return models.FilterIgnoreUser (0)
+func userFilter(c *models.ReqContext) int64 {
 	userIdFilter := c.SignedInUser.UserId
-	if (editorsCanAdmin && c.OrgRole == models.ROLE_EDITOR) || c.OrgRole == models.ROLE_ADMIN {
+	if c.OrgRole == models.ROLE_ADMIN {
 		userIdFilter = models.FilterIgnoreUser
 	}
-
 	return userIdFilter
 }

@ -210,7 +208,7 @@ func (hs *HTTPServer) GetTeamByID(c *models.ReqContext) response.Response {
 	// Using accesscontrol the filtering has already been performed at middleware layer
 	userIdFilter := models.FilterIgnoreUser
 	if !hs.Features.IsEnabled(featuremgmt.FlagAccesscontrol) {
-		userIdFilter = userFilter(hs.Cfg.EditorsCanAdmin, c)
+		userIdFilter = userFilter(c)
 	}

 	query := models.GetTeamByIdQuery{
--- a/pkg/api/team_test.go
+++ b/pkg/api/team_test.go
@ -40,39 +40,69 @@ func TestTeamAPIEndpoint(t *testing.T) {
 		hs.SQLStore = store
 		mock := &mockstore.SQLStoreMock{}

-		loggedInUserScenario(t, "When calling GET on", "/api/teams/search", "/api/teams/search", func(sc *scenarioContext) {
-			_, err := hs.SQLStore.CreateTeam("team1", "", 1)
-			require.NoError(t, err)
-			_, err = hs.SQLStore.CreateTeam("team2", "", 1)
-			require.NoError(t, err)
-
-			sc.handlerFunc = hs.SearchTeams
-			sc.fakeReqWithParams("GET", sc.url, map[string]string{}).exec()
-			require.Equal(t, http.StatusOK, sc.resp.Code)
-			var resp models.SearchTeamQueryResult
-			err = json.Unmarshal(sc.resp.Body.Bytes(), &resp)
-			require.NoError(t, err)
-
-			assert.EqualValues(t, 2, resp.TotalCount)
-			assert.Equal(t, 2, len(resp.Teams))
-		}, mock)
-
-		loggedInUserScenario(t, "When calling GET on", "/api/teams/search", "/api/teams/search", func(sc *scenarioContext) {
-			_, err := hs.SQLStore.CreateTeam("team1", "", 1)
-			require.NoError(t, err)
-			_, err = hs.SQLStore.CreateTeam("team2", "", 1)
-			require.NoError(t, err)
-
-			sc.handlerFunc = hs.SearchTeams
-			sc.fakeReqWithParams("GET", sc.url, map[string]string{"perpage": "10", "page": "2"}).exec()
-			require.Equal(t, http.StatusOK, sc.resp.Code)
-			var resp models.SearchTeamQueryResult
-			err = json.Unmarshal(sc.resp.Body.Bytes(), &resp)
-			require.NoError(t, err)
-
-			assert.EqualValues(t, 2, resp.TotalCount)
-			assert.Equal(t, 0, len(resp.Teams))
-		}, mock)
+		loggedInUserScenarioWithRole(t, "When admin is calling GET on", "GET", "/api/teams/search", "/api/teams/search",
+			models.ROLE_ADMIN, func(sc *scenarioContext) {
+				_, err := hs.SQLStore.CreateTeam("team1", "", 1)
+				require.NoError(t, err)
+				_, err = hs.SQLStore.CreateTeam("team2", "", 1)
+				require.NoError(t, err)
+
+				sc.handlerFunc = hs.SearchTeams
+				sc.fakeReqWithParams("GET", sc.url, map[string]string{}).exec()
+				require.Equal(t, http.StatusOK, sc.resp.Code)
+				var resp models.SearchTeamQueryResult
+				err = json.Unmarshal(sc.resp.Body.Bytes(), &resp)
+				require.NoError(t, err)
+
+				assert.EqualValues(t, 2, resp.TotalCount)
+				assert.Equal(t, 2, len(resp.Teams))
+			}, mock)
+
+		loggedInUserScenario(t, "When editor (with editors_can_admin) is calling GET on", "/api/teams/search",
+			"/api/teams/search", func(sc *scenarioContext) {
+				team1, err := hs.SQLStore.CreateTeam("team1", "", 1)
+				require.NoError(t, err)
+				_, err = hs.SQLStore.CreateTeam("team2", "", 1)
+				require.NoError(t, err)
+
+				// Adding the test user to the teams in order for him to list them
+				err = hs.SQLStore.AddTeamMember(testUserID, testOrgID, team1.Id, false, 0)
+				require.NoError(t, err)
+
+				sc.handlerFunc = hs.SearchTeams
+				sc.fakeReqWithParams("GET", sc.url, map[string]string{}).exec()
+				require.Equal(t, http.StatusOK, sc.resp.Code)
+				var resp models.SearchTeamQueryResult
+				err = json.Unmarshal(sc.resp.Body.Bytes(), &resp)
+				require.NoError(t, err)
+
+				assert.EqualValues(t, 1, resp.TotalCount)
+				assert.Equal(t, 1, len(resp.Teams))
+			}, mock)
+
+		loggedInUserScenario(t, "When editor (with editors_can_admin) calling GET with pagination on",
+			"/api/teams/search", "/api/teams/search", func(sc *scenarioContext) {
+				team1, err := hs.SQLStore.CreateTeam("team1", "", 1)
+				require.NoError(t, err)
+				team2, err := hs.SQLStore.CreateTeam("team2", "", 1)
+				require.NoError(t, err)
+
+				// Adding the test user to the teams in order for him to list them
+				err = hs.SQLStore.AddTeamMember(testUserID, testOrgID, team1.Id, false, 0)
+				require.NoError(t, err)
+				err = hs.SQLStore.AddTeamMember(testUserID, testOrgID, team2.Id, false, 0)
+				require.NoError(t, err)
+
+				sc.handlerFunc = hs.SearchTeams
+				sc.fakeReqWithParams("GET", sc.url, map[string]string{"perpage": "10", "page": "2"}).exec()
+				require.Equal(t, http.StatusOK, sc.resp.Code)
+				var resp models.SearchTeamQueryResult
+				err = json.Unmarshal(sc.resp.Body.Bytes(), &resp)
+				require.NoError(t, err)
+
+				assert.EqualValues(t, 2, resp.TotalCount)
+				assert.Equal(t, 0, len(resp.Teams))
+			}, mock)
 	})

 	t.Run("When creating team with API key", func(t *testing.T) {
--- a/pkg/services/sqlstore/team.go
+++ b/pkg/services/sqlstore/team.go
@ -76,6 +76,18 @@ func getTeamSelectSQLBase(filteredUsers []string) string {
 		` FROM team as team `
 }

+func getTeamSelectWithPermissionsSQLBase(filteredUsers []string) string {
+	return `SELECT
+		team.id AS id,
+		team.org_id,
+		team.name AS name,
+		team.email AS email,
+		team_member.permission, ` +
+		getTeamMemberCount(filteredUsers) +
+		` FROM team AS team
+		INNER JOIN team_member ON team.id = team_member.team_id AND team_member.user_id = ? `
+}
+
 func (ss *SQLStore) CreateTeam(name, email string, orgID int64) (models.Team, error) {
 	team := models.Team{
 		Name:    name,
@ -188,14 +200,14 @@ func (ss *SQLStore) SearchTeams(ctx context.Context, query *models.SearchTeamsQu
 	params := make([]interface{}, 0)

 	filteredUsers := getFilteredUsers(query.SignedInUser, query.HiddenUsers)
-	sql.WriteString(getTeamSelectSQLBase(filteredUsers))
-
 	for _, user := range filteredUsers {
 		params = append(params, user)
 	}

-	if query.UserIdFilter != models.FilterIgnoreUser {
-		sql.WriteString(` INNER JOIN team_member ON team.id = team_member.team_id AND team_member.user_id = ?`)
+	if query.UserIdFilter == models.FilterIgnoreUser {
+		sql.WriteString(getTeamSelectSQLBase(filteredUsers))
+	} else {
+		sql.WriteString(getTeamSelectWithPermissionsSQLBase(filteredUsers))
 		params = append(params, query.UserIdFilter)
 	}

--- a/public/app/features/teams/TeamList.tsx
+++ b/public/app/features/teams/TeamList.tsx
@ -69,11 +69,9 @@ export class TeamList extends PureComponent<Props, State> {
    const { editorsCanAdmin, signedInUser } = this.props;
    const permission = team.permission;
    const teamUrl = `org/teams/edit/${team.id}`;
-    const canDelete = contextSrv.hasAccessInMetadata(
-      AccessControlAction.ActionTeamsDelete,
-      team,
-      isPermissionTeamAdmin({ permission, editorsCanAdmin, signedInUser })
-    );
+    const isTeamAdmin = isPermissionTeamAdmin({ permission, editorsCanAdmin, signedInUser });
+    const canDelete = contextSrv.hasAccessInMetadata(AccessControlAction.ActionTeamsDelete, team, isTeamAdmin);
+    const canReadTeam = contextSrv.hasAccessInMetadata(AccessControlAction.ActionTeamsRead, team, isTeamAdmin);
    const canSeeTeamRoles = contextSrv.hasAccessInMetadata(AccessControlAction.ActionTeamsRolesList, team, false);
    const canUpdateTeamRoles =
      contextSrv.hasAccess(AccessControlAction.ActionTeamsRolesAdd, false) ||
@ -86,20 +84,34 @@ export class TeamList extends PureComponent<Props, State> {
    return (
      <tr key={team.id}>
        <td className="width-4 text-center link-td">
-          <a href={teamUrl}>
+          {canReadTeam ? (
+            <a href={teamUrl}>
+              <img className="filter-table__avatar" src={team.avatarUrl} alt="Team avatar" />
+            </a>
+          ) : (
            <img className="filter-table__avatar" src={team.avatarUrl} alt="Team avatar" />
-          </a>
+          )}
        </td>
        <td className="link-td">
-          <a href={teamUrl}>{team.name}</a>
+          {canReadTeam ? <a href={teamUrl}>{team.name}</a> : <div style={{ padding: '0px 8px' }}>{team.name}</div>}
        </td>
        <td className="link-td">
-          <a href={teamUrl} aria-label={team.email?.length > 0 ? undefined : 'Empty email cell'}>
-            {team.email}
-          </a>
+          {canReadTeam ? (
+            <a href={teamUrl} aria-label={team.email?.length > 0 ? undefined : 'Empty email cell'}>
+              {team.email}
+            </a>
+          ) : (
+            <div style={{ padding: '0px 8px' }} aria-label={team.email?.length > 0 ? undefined : 'Empty email cell'}>
+              {team.email}
+            </div>
+          )}
        </td>
        <td className="link-td">
-          <a href={teamUrl}>{team.memberCount}</a>
+          {canReadTeam ? (
+            <a href={teamUrl}>{team.memberCount}</a>
+          ) : (
+            <div style={{ padding: '0px 8px' }}>{team.memberCount}</div>
+          )}
        </td>
        {displayRolePicker && (
          <td>
--- a/public/app/features/teams/snapshots/TeamList.test.tsx.snap
+++ b/public/app/features/teams/snapshots/TeamList.test.tsx.snap
@ -445,42 +445,50 @@ exports[`Render when feature toggle editorsCanAdmin is turned on and signedin us
              <td
                className="width-4 text-center link-td"
              >
-                <a
-                  href="org/teams/edit/1"
-                >
-                  <img
-                    alt="Team avatar"
-                    className="filter-table__avatar"
-                    src="some/url/"
-                  />
-                </a>
+                <img
+                  alt="Team avatar"
+                  className="filter-table__avatar"
+                  src="some/url/"
+                />
              </td>
              <td
                className="link-td"
              >
-                <a
-                  href="org/teams/edit/1"
+                <div
+                  style={
+                    Object {
+                      "padding": "0px 8px",
+                    }
+                  }
                >
                  test-1
-                </a>
+                </div>
              </td>
              <td
                className="link-td"
              >
-                <a
-                  href="org/teams/edit/1"
+                <div
+                  style={
+                    Object {
+                      "padding": "0px 8px",
+                    }
+                  }
                >
                  test-1@test.com
-                </a>
+                </div>
              </td>
              <td
                className="link-td"
              >
-                <a
-                  href="org/teams/edit/1"
+                <div
+                  style={
+                    Object {
+                      "padding": "0px 8px",
+                    }
+                  }
                >
                  1
-                </a>
+                </div>
              </td>
              <td
                className="text-right"
@ -583,42 +591,50 @@ exports[`Render when feature toggle editorsCanAdmin is turned on and signedin us
              <td
                className="width-4 text-center link-td"
              >
-                <a
-                  href="org/teams/edit/1"
-                >
-                  <img
-                    alt="Team avatar"
-                    className="filter-table__avatar"
-                    src="some/url/"
-                  />
-                </a>
+                <img
+                  alt="Team avatar"
+                  className="filter-table__avatar"
+                  src="some/url/"
+                />
              </td>
              <td
                className="link-td"
              >
-                <a
-                  href="org/teams/edit/1"
+                <div
+                  style={
+                    Object {
+                      "padding": "0px 8px",
+                    }
+                  }
                >
                  test-1
-                </a>
+                </div>
              </td>
              <td
                className="link-td"
              >
-                <a
-                  href="org/teams/edit/1"
+                <div
+                  style={
+                    Object {
+                      "padding": "0px 8px",
+                    }
+                  }
                >
                  test-1@test.com
-                </a>
+                </div>
              </td>
              <td
                className="link-td"
              >
-                <a
-                  href="org/teams/edit/1"
+                <div
+                  style={
+                    Object {
+                      "padding": "0px 8px",
+                    }
+                  }
                >
                  1
-                </a>
+                </div>
              </td>
              <td
                className="text-right"