feat: make GetUsers query use sql filter

coder · Emyrk · Jul 11, 2023 · Jul 12, 2023 · Jul 12, 2023 · Jul 12, 2023
commit 34a0e42009b87db4fe673a33f3f871e29d857580
diff --git a/coderd/database/dbauthz/dbauthz.go b/coderd/database/dbauthz/dbauthz.go
@@ -620,8 +620,12 @@ func (q *querier) GetAuthorizedUserCount(ctx context.Context, arg database.GetFi
 }
 
 func (q *querier) GetUsersWithCount(ctx context.Context, arg database.GetUsersParams) ([]database.User, int64, error) {
-	// TODO Implement this with a SQL filter. The count is incorrect without it.
-	rowUsers, err := q.db.GetUsers(ctx, arg)
+	prep, err := prepareSQLFilter(ctx, q.auth, rbac.ActionRead, rbac.ResourceUser.Type)
+	if err != nil {
+		return nil, -1, xerrors.Errorf("failed to prepare sql filter: %w", err)
+	}
+
+	rowUsers, err := q.db.GetAuthorizedUsers(ctx, arg, prep)
 	if err != nil {
 		return nil, -1, err
 	}
@@ -630,18 +634,8 @@ func (q *querier) GetUsersWithCount(ctx context.Context, arg database.GetUsersPa
 		return []database.User{}, 0, nil
 	}
 
-	act, ok := ActorFromContext(ctx)
-	if !ok {
-		return nil, -1, NoActorError
-	}
-
 	// TODO: Is this correct? Should we return a restricted user?
 	users := database.ConvertUserRows(rowUsers)
-	users, err = rbac.Filter(ctx, q.auth, act, rbac.ActionRead, users)
-	if err != nil {
-		return nil, -1, err
-	}
-
 	return users, rowUsers[0].Count, nil
 }
 
@@ -699,6 +693,13 @@ func authorizedTemplateVersionFromJob(ctx context.Context, q *querier, job datab
 	}
 }
 
+// GetAuthorizedUsers is not required for dbauthz since GetUsers is already
+// authenticated.
+func (q *querier) GetAuthorizedUsers(ctx context.Context, arg database.GetUsersParams, _ rbac.PreparedAuthorized) ([]database.GetUsersRow, error) {
+	// GetUsers is authenticated.
+	return q.GetUsers(ctx, arg)
+}
+
 func (q *querier) AcquireLock(ctx context.Context, id int64) error {
 	return q.db.AcquireLock(ctx, id)
 }
@@ -1427,8 +1428,12 @@ func (q *querier) GetUserLinkByUserIDLoginType(ctx context.Context, arg database
 }
 
 func (q *querier) GetUsers(ctx context.Context, arg database.GetUsersParams) ([]database.GetUsersRow, error) {
-	// TODO: We should use GetUsersWithCount with a better method signature.
-	return fetchWithPostFilter(q.auth, q.db.GetUsers)(ctx, arg)
+	// This does the filtering in SQL.
+	prep, err := prepareSQLFilter(ctx, q.auth, rbac.ActionRead, rbac.ResourceUser.Type)
+	if err != nil {
+		return nil, xerrors.Errorf("(dev error) prepare sql filter: %w", err)
+	}
+	return q.db.GetAuthorizedUsers(ctx, arg, prep)
 }
 
 // GetUsersByIDs is only used for usernames on workspace return data.

diff --git a/coderd/database/dbmock/dbmock.go b/coderd/database/dbmock/dbmock.go
diff --git a/coderd/database/modelqueries.go b/coderd/database/modelqueries.go
@@ -252,9 +252,70 @@ func (q *sqlQuerier) GetAuthorizedWorkspaces(ctx context.Context, arg GetWorkspa
 }
 
 type userQuerier interface {
+	GetAuthorizedUsers(ctx context.Context, arg GetUsersParams, prepared rbac.PreparedAuthorized) ([]GetUsersRow, error)
 	GetAuthorizedUserCount(ctx context.Context, arg GetFilteredUserCountParams, prepared rbac.PreparedAuthorized) (int64, error)
 }
 
+func (q *sqlQuerier) GetAuthorizedUsers(ctx context.Context, arg GetUsersParams, prepared rbac.PreparedAuthorized) ([]GetUsersRow, error) {
+	authorizedFilter, err := prepared.CompileToSQL(ctx, regosql.ConvertConfig{
+		VariableConverter: regosql.UserConverter(),
+	})
+
+	if err != nil {
+		return nil, xerrors.Errorf("compile authorized filter: %w", err)
+	}
+
+	filtered, err := insertAuthorizedFilter(getUsers, fmt.Sprintf(" AND %s", authorizedFilter))
+	if err != nil {
+		return nil, xerrors.Errorf("insert authorized filter: %w", err)
+	}
+
+	query := fmt.Sprintf("-- name: GetAuthorizedUsers :many\n%s", filtered)
+	rows, err := q.db.QueryContext(ctx, query,
+		arg.AfterID,
+		arg.Search,
+		pq.Array(arg.Status),
+		pq.Array(arg.RbacRole),
+		arg.LastSeenBefore,
+		arg.LastSeenAfter,
+		arg.OffsetOpt,
+		arg.LimitOpt,
+	)
+	if err != nil {
+		return nil, err
+	}
+	defer rows.Close()
+	var items []GetUsersRow
+	for rows.Next() {
+		var i GetUsersRow
+		if err := rows.Scan(
+			&i.ID,
+			&i.Email,
+			&i.Username,
+			&i.HashedPassword,
+			&i.CreatedAt,
+			&i.UpdatedAt,
+			&i.Status,
+			&i.RBACRoles,
+			&i.LoginType,
+			&i.AvatarURL,
+			&i.Deleted,
+			&i.LastSeenAt,
+			&i.Count,
+		); err != nil {
+			return nil, err
+		}
+		items = append(items, i)
+	}
+	if err := rows.Close(); err != nil {
+		return nil, err
+	}
+	if err := rows.Err(); err != nil {
+		return nil, err
+	}
+	return items, nil
+}
+
 func (q *sqlQuerier) GetAuthorizedUserCount(ctx context.Context, arg GetFilteredUserCountParams, prepared rbac.PreparedAuthorized) (int64, error) {
 	authorizedFilter, err := prepared.CompileToSQL(ctx, regosql.ConvertConfig{
 		VariableConverter: regosql.UserConverter(),

diff --git a/coderd/database/queries.sql.go b/coderd/database/queries.sql.go
diff --git a/coderd/database/queries/users.sql b/coderd/database/queries/users.sql
@@ -208,6 +208,7 @@ WHERE
 		ELSE true
 	END
 	-- End of filters
+
 ORDER BY
 	-- Deterministic and consistent ordering of all users. This is to ensure consistent pagination.
 	LOWER(username) ASC OFFSET @offset_opt

diff --git a/site/src/components/Navbar/Navbar.tsx b/site/src/components/Navbar/Navbar.tsx
@@ -16,6 +16,7 @@ export const Navbar: FC = () => {
   const canViewAuditLog =
     featureVisibility["audit_log"] && Boolean(permissions.viewAuditLog)
   const canViewDeployment = Boolean(permissions.viewDeploymentValues)
+  const canViewUsers = Boolean(permissions.readAllUsers)
   const onSignOut = () => authSend("SIGN_OUT")
   const proxyContextValue = useProxy()
   const dashboard = useDashboard()
@@ -29,6 +30,7 @@ export const Navbar: FC = () => {
       onSignOut={onSignOut}
       canViewAuditLog={canViewAuditLog}
       canViewDeployment={canViewDeployment}
+      canViewUsers={canViewUsers}
       proxyContextValue={
         dashboard.experiments.includes("moons") ? proxyContextValue : undefined
       }

diff --git a/site/src/components/Navbar/NavbarView.test.tsx b/site/src/components/Navbar/NavbarView.test.tsx
@@ -48,6 +48,7 @@ describe("NavbarView", () => {
         onSignOut={noop}
         canViewAuditLog
         canViewDeployment
+        canViewUsers
       />,
     )
     const workspacesLink = await screen.findByText(navLanguage.workspaces)
@@ -62,6 +63,7 @@ describe("NavbarView", () => {
         onSignOut={noop}
         canViewAuditLog
         canViewDeployment
+        canViewUsers
       />,
     )
     const templatesLink = await screen.findByText(navLanguage.templates)
@@ -76,6 +78,7 @@ describe("NavbarView", () => {
         onSignOut={noop}
         canViewAuditLog
         canViewDeployment
+        canViewUsers
       />,
     )
     const userLink = await screen.findByText(navLanguage.users)
@@ -98,6 +101,7 @@ describe("NavbarView", () => {
         onSignOut={noop}
         canViewAuditLog
         canViewDeployment
+        canViewUsers
       />,
     )
 
@@ -115,6 +119,7 @@ describe("NavbarView", () => {
         onSignOut={noop}
         canViewAuditLog
         canViewDeployment
+        canViewUsers
       />,
     )
     const auditLink = await screen.findByText(navLanguage.audit)
@@ -129,6 +134,7 @@ describe("NavbarView", () => {
         onSignOut={noop}
         canViewAuditLog={false}
         canViewDeployment
+        canViewUsers
       />,
     )
     const auditLink = screen.queryByText(navLanguage.audit)
@@ -143,6 +149,7 @@ describe("NavbarView", () => {
         onSignOut={noop}
         canViewAuditLog
         canViewDeployment
+        canViewUsers
       />,
     )
     const auditLink = await screen.findByText(navLanguage.deployment)
@@ -159,6 +166,7 @@ describe("NavbarView", () => {
         onSignOut={noop}
         canViewAuditLog={false}
         canViewDeployment={false}
+        canViewUsers={false}
       />,
     )
     const auditLink = screen.queryByText(navLanguage.deployment)

diff --git a/site/src/components/Navbar/NavbarView.tsx b/site/src/components/Navbar/NavbarView.tsx
@@ -36,13 +36,15 @@ export interface NavbarViewProps {
   onSignOut: () => void
   canViewAuditLog: boolean
   canViewDeployment: boolean
+  canViewUsers: boolean
   proxyContextValue?: ProxyContextValue
 }
 
 export const Language = {
   workspaces: "Workspaces",
   templates: "Templates",
   users: "Users",
+  groups: "Groups",
   audit: "Audit",
   deployment: "Deployment",
 }
@@ -52,8 +54,9 @@ const NavItems: React.FC<
     className?: string
     canViewAuditLog: boolean
     canViewDeployment: boolean
+    canViewUsers: boolean
   }>
-> = ({ className, canViewAuditLog, canViewDeployment }) => {
+> = ({ className, canViewAuditLog, canViewUsers, canViewDeployment }) => {
   const styles = useStyles()
   const location = useLocation()
 
@@ -77,7 +80,7 @@ const NavItems: React.FC<
       </ListItem>
       <ListItem button className={styles.item}>
         <NavLink className={styles.link} to={USERS_LINK}>
-          {Language.users}
+          {canViewUsers ? Language.users : Language.groups}
         </NavLink>
       </ListItem>
       {canViewAuditLog && (
@@ -105,6 +108,7 @@ export const NavbarView: FC<NavbarViewProps> = ({
   onSignOut,
   canViewAuditLog,
   canViewDeployment,
+  canViewUsers,
   proxyContextValue,
 }) => {
   const styles = useStyles()
@@ -142,6 +146,7 @@ export const NavbarView: FC<NavbarViewProps> = ({
             <NavItems
               canViewAuditLog={canViewAuditLog}
               canViewDeployment={canViewDeployment}
+              canViewUsers={canViewUsers}
             />
           </div>
         </Drawer>
@@ -158,6 +163,7 @@ export const NavbarView: FC<NavbarViewProps> = ({
           className={styles.desktopNavItems}
           canViewAuditLog={canViewAuditLog}
           canViewDeployment={canViewDeployment}
+          canViewUsers={canViewUsers}
         />
 
         <Box