prevent admins from removing their own admin permissions (#10012)

frontend/src/metabase/admin/people/components/GroupDetail.jsx

@@ -157,6 +157,7 @@ const UserRow = ({ user, showRemoveButton, onRemoveUserClicked }) => (
 const MembersTable = ({
   group,
   members,
+  currentUser: { id: currentUserId } = {},
   users,
   showAddUser,
   text,
@@ -169,8 +170,9 @@ const MembersTable = ({
   onRemoveUserFromSelection,
 }) => {
   // you can't remove people from Default and you can't remove the last user from Admin
-  const showRemoveMemeberButton =
-    !isDefaultGroup(group) && (!isAdminGroup(group) || members.length > 1);
+  const isCurrentUser = ({ user_id }) => user_id === currentUserId;
+  const showRemoveMemeberButton = user =>
+    !isDefaultGroup(group) && !(isAdminGroup(group) && isCurrentUser(user));
 
   return (
     <div>
@@ -192,7 +194,7 @@ const MembersTable = ({
             <UserRow
               key={index}
               user={user}
-              showRemoveButton={showRemoveMemeberButton}
+              showRemoveButton={showRemoveMemeberButton(user)}
               onRemoveUserClicked={onRemoveUserClicked}
             />
           ))}
@@ -305,7 +307,7 @@ export default class GroupDetail extends Component {
   render() {
     // users = array of all users for purposes of adding new users to group
     // [group.]members = array of users currently in the group
-    let { group, users } = this.props;
+    let { currentUser, group, users } = this.props;
     const { text, selectedUsers, addUserVisible, alertMessage } = this.state;
     const members = this.getMembers();
 
@@ -334,6 +336,7 @@ export default class GroupDetail extends Component {
       >
         <GroupDescription group={group} />
         <MembersTable
+          currentUser={currentUser}
           group={group}
           members={members}
           users={filteredUsers}

frontend/src/metabase/admin/people/components/GroupSelect.jsx

@@ -12,8 +12,13 @@ import {
 import cx from "classnames";
 import _ from "underscore";
 
-export const GroupOption = ({ group, selectedGroups = {}, onGroupChange }) => {
-  const disabled = !canEditMembership(group);
+export const GroupOption = ({
+  group,
+  selectedGroups = {},
+  onGroupChange,
+  isDisabled = false,
+}) => {
+  const disabled = isDisabled || !canEditMembership(group);
   const selected = isDefaultGroup(group) || selectedGroups[group.id];
   return (
     <div
@@ -30,7 +35,12 @@ export const GroupOption = ({ group, selectedGroups = {}, onGroupChange }) => {
   );
 };
 
-export const GroupSelect = ({ groups, selectedGroups, onGroupChange }) => {
+export const GroupSelect = ({
+  groups,
+  selectedGroups,
+  onGroupChange,
+  isCurrentUser,
+}) => {
   const other = groups.filter(g => !isAdminGroup(g) && !isDefaultGroup(g));
   return (
     <div className="GroupSelect scroll-y py1">
@@ -38,6 +48,7 @@ export const GroupSelect = ({ groups, selectedGroups, onGroupChange }) => {
         group={_.find(groups, isAdminGroup)}
         selectedGroups={selectedGroups}
         onGroupChange={onGroupChange}
+        isDisabled={isCurrentUser}
       />
       <GroupOption
         group={_.find(groups, isDefaultGroup)}

frontend/src/metabase/admin/people/components/UserGroupSelect.jsx

@@ -37,6 +37,7 @@ export default class UserGroupSelect extends Component {
     groups: PropTypes.array,
     createMembership: PropTypes.func.isRequired,
     deleteMembership: PropTypes.func.isRequired,
+    isCurrentUser: PropTypes.bool.isRequired,
   };
 
   static defaultProps = {
@@ -48,7 +49,13 @@ export default class UserGroupSelect extends Component {
   }
 
   render() {
-    let { user, groups, createMembership, deleteMembership } = this.props;
+    const {
+      user,
+      groups,
+      createMembership,
+      deleteMembership,
+      isCurrentUser,
+    } = this.props;
 
     if (!groups || groups.length === 0 || !user.memberships) {
       return <LoadingSpinner />;
@@ -82,6 +89,7 @@ export default class UserGroupSelect extends Component {
           groups={groups}
           selectedGroups={user.memberships}
           onGroupChange={changeMembership}
+          isCurrentUser={isCurrentUser}
         />
       </PopoverWithTrigger>
     );

frontend/src/metabase/admin/people/containers/GroupDetailApp.jsx

@@ -4,12 +4,14 @@ import { connect } from "react-redux";
 import User from "metabase/entities/users";
 import Group from "metabase/entities/groups";
 import { getUsersWithMemberships } from "../selectors";
+import { getUser } from "metabase/selectors/user";
 
 import GroupDetail from "../components/GroupDetail.jsx";
 
 @User.loadList()
 @Group.load({ id: (state, props) => props.params.groupId })
 @connect((state, props) => ({
+  currentUser: getUser(state),
   users: getUsersWithMemberships(state, props),
 }))
 export default class GroupDetailApp extends Component {

frontend/src/metabase/admin/people/containers/PeopleListingApp.jsx

@@ -75,7 +75,6 @@ export default class PeopleListingApp extends Component {
     let { user, users, groups } = this.props;
     let { showDeactivated } = this.state;
 
-    const isAdmin = u => u.is_superuser;
     const isCurrentUser = u => user && user.id === u.id;
 
     // TODO - this should be done in connect
@@ -185,6 +184,7 @@ export default class PeopleListingApp extends Component {
                             groups={groups}
                             createMembership={this.props.createMembership}
                             deleteMembership={this.props.deleteMembership}
+                            isCurrentUser={isCurrentUser(user)}
                           />
                         </td>,
                         <td key="last_login">
@@ -204,11 +204,10 @@ export default class PeopleListingApp extends Component {
                                 title: t`Reset password`,
                                 link: Urls.resetPassword(user.id),
                               },
-                              !isAdmin(user) &&
-                                !isCurrentUser(user) && {
-                                  title: t`Deactivate user`,
-                                  link: Urls.deactivateUser(user.id),
-                                },
+                              !isCurrentUser(user) && {
+                                title: t`Deactivate user`,
+                                link: Urls.deactivateUser(user.id),
+                              },
                             ]}
                           />
                         </td>,

frontend/test/metabase/admin/people/containers/GroupDetailApp.integ.spec.js

 import React from "react";
 import mock from "xhr-mock";
 import { mountWithStore } from "__support__/integration_tests";
+import { refreshCurrentUser } from "metabase/redux/user";
 
 import GroupDetailApp from "metabase/admin/people/containers/GroupDetailApp";
 
-const MOCK_USER = {
-  id: 1,
-  first_name: "Testy",
-  last_name: "McTestFace",
-  email: "test@metabase.com",
-};
+const MOCK_USERS = [
+  {
+    id: 1,
+    first_name: "Testy",
+    last_name: "McTestFace",
+    email: "test@metabase.com",
+  },
+  {
+    id: 2,
+    first_name: "David",
+    last_name: "Attenborough",
+    email: "dattenborough@metabase.com",
+  },
+];
+
+const MOCK_MEMBERS = MOCK_USERS.map(({ id, ...user }) => ({
+  ...user,
+  user_id: id,
+}));
 
 describe("GroupDetailApp", () => {
   beforeEach(() => mock.setup());
@@ -18,22 +32,48 @@ describe("GroupDetailApp", () => {
   it("should load the user", async () => {
     expect.assertions(1);
 
-    mock.get("/api/user", (req, res) => res.json([MOCK_USER]));
+    mock.get("/api/user/current", (req, res) => res.json(MOCK_USERS[0]));
+    mock.get("/api/user", (req, res) => res.json([MOCK_USERS[0]]));
     mock.get("/api/permissions/group/42", (req, res) => {
       return res.json({
         id: 42,
         name: "Administrators",
-        members: [MOCK_USER],
+        members: [MOCK_MEMBERS[0]],
       });
     });
 
-    const { wrapper } = mountWithStore(
+    const { wrapper, store } = mountWithStore(
       <GroupDetailApp params={{ groupId: 42 }} />,
     );
+    store.dispatch(refreshCurrentUser());
 
     expect((await wrapper.async.find("tr td")).map(td => td.text())).toEqual([
       "Testy McTestFace",
       "test@metabase.com",
     ]);
   });
+
+  it("should not the current user remove themselvs from Administrators", async () => {
+    expect.assertions(2);
+
+    mock.get("/api/user/current", (req, res) => res.json(MOCK_USERS[0]));
+    mock.get("/api/user", (req, res) => res.json(MOCK_USERS));
+    mock.get("/api/permissions/group/42", (req, res) => {
+      return res.json({
+        id: 42,
+        name: "Administrators",
+        members: MOCK_MEMBERS,
+      });
+    });
+
+    const { wrapper, store } = mountWithStore(
+      <GroupDetailApp params={{ groupId: 42 }} />,
+    );
+    store.dispatch(refreshCurrentUser());
+
+    const rows = await wrapper.async.find("tr");
+    // we should only have a third column (removal option) for other users
+    expect(rows.at(1).find("td").length).toBe(2);
+    expect(rows.at(2).find("td").length).toBe(3);
+  });
 });

frontend/test/metabase/admin/people/containers/PeopleListingApp.integ.spec.js

+import React from "react";
+import mock from "xhr-mock";
+import { mountWithStore } from "__support__/integration_tests";
+import { click } from "__support__/enzyme_utils";
+import { refreshCurrentUser } from "metabase/redux/user";
+import UserAvatar from "metabase/components/UserAvatar.jsx";
+import Radio from "metabase/components/Radio";
+import EntityMenu from "metabase/components/EntityMenu";
+import EntityMenuItem from "metabase/components/EntityMenuItem";
+import EntityMenuTrigger from "metabase/components/EntityMenuTrigger";
+
+import PeopleListingApp from "metabase/admin/people/containers/PeopleListingApp";
+
+const MOCK_USERS = [
+  {
+    id: 1,
+    first_name: "Testy",
+    last_name: "McTestFace",
+    email: "test@metabase.com",
+    is_active: true,
+  },
+  {
+    id: 2,
+    first_name: "David",
+    last_name: "Attenborough",
+    email: "dattenborough@metabase.com",
+    is_active: true,
+  },
+  {
+    id: 3,
+    first_name: "Hooty",
+    last_name: "McOwlface",
+    email: "hmcowlface@metabase.com",
+    is_active: false,
+  },
+];
+
+const MOCK_MEMBERS = MOCK_USERS.map(({ id, ...user }) => ({
+  ...user,
+  user_id: id,
+}));
+
+const MOCK_GROUPS = [
+  { id: 1, name: "Administrators", member_count: MOCK_MEMBERS.length },
+];
+
+const mockApiCalls = () => {
+  mock.get("/api/user/current", (req, res) => res.json(MOCK_USERS[0]));
+  mock.get("/api/user?include_deactivated=true", (req, res) =>
+    res.json(MOCK_USERS),
+  );
+  mock.get("/api/permissions/group", (req, res) => res.json(MOCK_GROUPS));
+  mock.get("/api/permissions/membership", (req, res) =>
+    res.json({
+      "1": [
+        { membership_id: 1, group_id: 1, user_id: 1 },
+        { membership_id: 2, group_id: 1, user_id: 2 },
+        { membership_id: 3, group_id: 1, user_id: 3 },
+      ],
+    }),
+  );
+};
+
+describe("PeopleListingApp", () => {
+  beforeEach(() => mock.setup());
+  afterEach(() => mock.teardown());
+
+  it("should load active users", async () => {
+    expect.assertions(1);
+
+    mockApiCalls();
+
+    const { wrapper, store } = mountWithStore(<PeopleListingApp />);
+    store.dispatch(refreshCurrentUser());
+
+    const users = await wrapper.async.find(UserAvatar);
+    expect(users.map(u => u.text())).toEqual(["DA", "TM"]);
+  });
+
+  it("should load inactive users", async () => {
+    expect.assertions(1);
+
+    mockApiCalls();
+
+    const { wrapper, store } = mountWithStore(<PeopleListingApp />);
+    store.dispatch(refreshCurrentUser());
+
+    const radio = await wrapper.async.find(Radio);
+    click(radio.find("input[value=true]"));
+
+    const users = await wrapper.async.find(UserAvatar);
+    expect(users.map(u => u.text())).toEqual(["HM"]);
+  });
+
+  it("should not let the current user deactivate themselves", async () => {
+    expect.assertions(1);
+
+    mockApiCalls();
+
+    const { wrapper, store } = mountWithStore(<PeopleListingApp />);
+    store.dispatch(refreshCurrentUser());
+
+    const menus = await wrapper.async.find(EntityMenu);
+
+    // the current user is at index 1 because of sorting
+    click(menus.at(1).find(EntityMenuTrigger));
+    const menuItems = await wrapper.async.find(EntityMenuItem);
+    const menuItemLabels = menuItems.map(mi => mi.text());
+    expect(menuItemLabels).toEqual(["Edit user", "Reset password"]);
+  });
+
+  it("should let the current user deactivate other admins", async () => {
+    expect.assertions(1);
+
+    mockApiCalls();
+
+    const { wrapper, store } = mountWithStore(<PeopleListingApp />);
+    store.dispatch(refreshCurrentUser());
+
+    const menus = await wrapper.async.find(EntityMenu);
+
+    click(menus.at(0).find(EntityMenuTrigger));
+    const menuItems = await wrapper.async.find(EntityMenuItem);
+    const menuItemLabels = menuItems.map(mi => mi.text());
+    expect(menuItemLabels).toEqual([
+      "Edit user",
+      "Reset password",
+      "Deactivate user",
+    ]);
+  });
+});