Viewer badge

app/components/Sharing/components/Suggestions.tsx

@@ -13,6 +13,7 @@ import Group from "~/models/Group";
 import User from "~/models/User";
 import ArrowKeyNavigation from "~/components/ArrowKeyNavigation";
 import { Avatar, GroupAvatar, AvatarSize, IAvatar } from "~/components/Avatar";
+import Badge from "~/components/Badge";
 import Empty from "~/components/Empty";
 import Placeholder from "~/components/List/Placeholder";
 import Scrollable from "~/components/Scrollable";
@@ -154,11 +155,12 @@ export const Suggestions = observer(
       }
       return {
         title: suggestion.name,
-        subtitle: suggestion.email
-          ? suggestion.email
-          : suggestion.isViewer
-          ? t("Viewer")
-          : t("Editor"),
+        subtitle: (
+          <>
+            {suggestion.email ? `${suggestion.email} ` : ""}
+            {suggestion.isViewer && <Badge>{t("Viewer")}</Badge>}
+          </>
+        ),
         image: (
           <Avatar
             model={suggestion}

server/models/UserMembership.ts

@@ -18,8 +18,14 @@ import {
   AfterCreate,
   AfterUpdate,
   Length,
+  BeforeCreate,
 } from "sequelize-typescript";
-import { CollectionPermission, DocumentPermission } from "@shared/types";
+import {
+  CollectionPermission,
+  DocumentPermission,
+  UserRole,
+} from "@shared/types";
+import { ValidationError } from "@server/errors";
 import Collection from "./Collection";
 import Document from "./Document";
 import User from "./User";
@@ -197,6 +203,30 @@ class UserMembership extends IdModel<
 
   // hooks
 
+  @BeforeCreate
+  static async checkViewerPermission(
+    model: UserMembership,
+    options: SaveOptions<UserMembership>
+  ) {
+    if (
+      model.permission === DocumentPermission.Read ||
+      model.permission === CollectionPermission.Read ||
+      model.sourceId
+    ) {
+      return;
+    }
+
+    const user = await User.findByPk(model.userId, {
+      transaction: options.transaction,
+    });
+
+    if (user?.role === UserRole.Viewer) {
+      throw ValidationError(
+        "Users with `viewer` role can not be given write permissions"
+      );
+    }
+  }
+
   @AfterCreate
   static async createSourcedMemberships(
     model: UserMembership,

server/policies/collection.test.ts

@@ -294,7 +294,7 @@ describe("member", () => {
 });
 
 describe("viewer", () => {
-  describe("read_write permission", () => {
+  describe("read permission", () => {
     it("should allow read permissions for viewer", async () => {
       const team = await buildTeam();
       const user = await buildUser({
@@ -303,7 +303,7 @@ describe("viewer", () => {
       });
       const collection = await buildCollection({
         teamId: team.id,
-        permission: CollectionPermission.ReadWrite,
+        permission: CollectionPermission.Read,
       });
       const abilities = serialize(user, collection);
       expect(abilities.read).toBeTruthy();
@@ -328,38 +328,8 @@ describe("viewer", () => {
         createdById: user.id,
         collectionId: collection.id,
         userId: user.id,
-        permission: CollectionPermission.ReadWrite,
-      });
-      // reload to get membership
-      const reloaded = await Collection.scope({
-        method: ["withMembership", user.id],
-      }).findByPk(collection.id);
-      const abilities = serialize(user, reloaded);
-      expect(abilities.read).toBeTruthy();
-      expect(abilities.readDocument).toBeTruthy();
-      expect(abilities.share).toBeTruthy();
-      expect(abilities.update).toEqual(false);
-      expect(abilities.archive).toEqual(false);
-    });
-  });
-
-  describe("read permission", () => {
-    it("should allow override with read_write membership permission", async () => {
-      const team = await buildTeam();
-      const user = await buildUser({
-        role: UserRole.Viewer,
-        teamId: team.id,
-      });
-      const collection = await buildCollection({
-        teamId: team.id,
         permission: CollectionPermission.Read,
       });
-      await UserMembership.create({
-        createdById: user.id,
-        collectionId: collection.id,
-        userId: user.id,
-        permission: CollectionPermission.ReadWrite,
-      });
       // reload to get membership
       const reloaded = await Collection.scope({
         method: ["withMembership", user.id],
@@ -367,8 +337,7 @@ describe("viewer", () => {
       const abilities = serialize(user, reloaded);
       expect(abilities.read).toBeTruthy();
       expect(abilities.readDocument).toBeTruthy();
-      expect(abilities.createDocument).toBeTruthy();
-      expect(abilities.share).toBeTruthy();
+      expect(abilities.share).toEqual(false);
       expect(abilities.update).toEqual(false);
       expect(abilities.archive).toEqual(false);
     });
@@ -406,7 +375,7 @@ describe("viewer", () => {
         createdById: user.id,
         collectionId: collection.id,
         userId: user.id,
-        permission: CollectionPermission.ReadWrite,
+        permission: CollectionPermission.Read,
       });
       // reload to get membership
       const reloaded = await Collection.scope({
@@ -415,8 +384,8 @@ describe("viewer", () => {
       const abilities = serialize(user, reloaded);
       expect(abilities.read).toBeTruthy();
       expect(abilities.readDocument).toBeTruthy();
-      expect(abilities.createDocument).toBeTruthy();
-      expect(abilities.share).toBeTruthy();
+      expect(abilities.createDocument).toEqual(false);
+      expect(abilities.share).toEqual(false);
       expect(abilities.update).toEqual(false);
       expect(abilities.archive).toEqual(false);
     });

server/policies/collection.ts

@@ -1,6 +1,6 @@
 import invariant from "invariant";
 import filter from "lodash/filter";
-import { CollectionPermission } from "@shared/types";
+import { CollectionPermission, UserRole } from "@shared/types";
 import { Collection, User, Team } from "@server/models";
 import { allow, can } from "./cancan";
 import { and, isTeamAdmin, isTeamModel, isTeamMutable, or } from "./utils";
@@ -41,7 +41,11 @@ allow(User, "read", Collection, (user, collection) => {
   }
 
   if (collection.isPrivate || user.isGuest) {
-    return includesMembership(collection, Object.values(CollectionPermission));
+    return includesMembership(
+      user,
+      collection,
+      Object.values(CollectionPermission)
+    );
   }
 
   return true;
@@ -58,6 +62,7 @@ allow(
 
     if (collection.isPrivate || user.isGuest) {
       return includesMembership(
+        user,
         collection,
         Object.values(CollectionPermission)
       );
@@ -96,7 +101,7 @@ allow(User, "share", Collection, (user, collection) => {
     collection.permission !== CollectionPermission.ReadWrite ||
     user.isViewer
   ) {
-    return includesMembership(collection, [
+    return includesMembership(user, collection, [
       CollectionPermission.ReadWrite,
       CollectionPermission.Admin,
     ]);
@@ -119,7 +124,7 @@ allow(User, "updateDocument", Collection, (user, collection) => {
     user.isViewer ||
     user.isGuest
   ) {
-    return includesMembership(collection, [
+    return includesMembership(user, collection, [
       CollectionPermission.ReadWrite,
       CollectionPermission.Admin,
     ]);
@@ -151,7 +156,7 @@ allow(
       user.isViewer ||
       user.isGuest
     ) {
-      return includesMembership(collection, [
+      return includesMembership(user, collection, [
         CollectionPermission.ReadWrite,
         CollectionPermission.Admin,
       ]);
@@ -167,7 +172,7 @@ allow(User, ["update", "archive"], Collection, (user, collection) =>
     !!collection?.isActive,
     or(
       isTeamAdmin(user, collection),
-      includesMembership(collection, [CollectionPermission.Admin])
+      includesMembership(user, collection, [CollectionPermission.Admin])
     )
   )
 );
@@ -178,7 +183,7 @@ allow(User, "delete", Collection, (user, collection) =>
     !collection?.deletedAt,
     or(
       isTeamAdmin(user, collection),
-      includesMembership(collection, [CollectionPermission.Admin])
+      includesMembership(user, collection, [CollectionPermission.Admin])
     )
   )
 );
@@ -189,12 +194,13 @@ allow(User, "restore", Collection, (user, collection) =>
     !collection?.isActive,
     or(
       isTeamAdmin(user, collection),
-      includesMembership(collection, [CollectionPermission.Admin])
+      includesMembership(user, collection, [CollectionPermission.Admin])
     )
   )
 );
 
 function includesMembership(
+  user: User,
   collection: Collection | null,
   permissions: CollectionPermission[]
 ) {
@@ -211,10 +217,28 @@ function includesMembership(
     "Development: collection groupMemberships not preloaded, did you forget `withMembership` scope?"
   );
 
-  const membershipIds = filter(
-    [...collection.memberships, ...collection.groupMemberships],
-    (m) => permissions.includes(m.permission as CollectionPermission)
-  ).map((m) => m.id);
+  const userMemberships = filter(collection.memberships, (m) =>
+    permissions.includes(m.permission as CollectionPermission)
+  );
+
+  // Is a non-read permission included in the permissions list
+  const isNonRead =
+    permissions.filter((p) => p !== CollectionPermission.Read).length > 0;
+
+  const groupMemberships = filter(collection.groupMemberships, (m) => {
+    // If the user is a viewer and the permission is non-read provided through a group membership
+    // they can't access. If the permission is provided through a user membership it is allowed
+    // for backwards compatability.
+    if (isNonRead && user.role === UserRole.Viewer) {
+      return false;
+    }
+
+    return permissions.includes(m.permission as CollectionPermission);
+  });
+
+  const membershipIds = [...userMemberships, ...groupMemberships].map(
+    (m) => m.id
+  );
 
   return membershipIds.length > 0 ? membershipIds : false;
 }

server/policies/document.test.ts

@@ -355,7 +355,7 @@ describe("read document", () => {
 });
 
 describe("read_write document", () => {
-  for (const role of Object.values(UserRole)) {
+  for (const role of [UserRole.Guest, UserRole.Admin, UserRole.Member]) {
     it(`should allow write permissions for ${role}`, async () => {
       const team = await buildTeam();
       const user = await buildUser({ teamId: team.id, role });
@@ -394,7 +394,7 @@ describe("read_write document", () => {
 });
 
 describe("manage document", () => {
-  for (const role of Object.values(UserRole)) {
+  for (const role of [UserRole.Guest, UserRole.Admin, UserRole.Member]) {
     it(`should allow write permissions, user management, and sub-document creation for ${role}`, async () => {
       const team = await buildTeam();
       const user = await buildUser({

server/routes/api/attachments/attachments.test.ts

@@ -1,5 +1,4 @@
-import { AttachmentPreset, CollectionPermission } from "@shared/types";
-import { UserMembership } from "@server/models";
+import { AttachmentPreset } from "@shared/types";
 import Attachment from "@server/models/Attachment";
 import {
   buildUser,
@@ -7,7 +6,6 @@ import {
   buildCollection,
   buildAttachment,
   buildDocument,
-  buildViewer,
 } from "@server/test/factories";
 import { getTestServer } from "@server/test/support";
 
@@ -110,70 +108,6 @@ describe("#attachments.create", () => {
       expect(res.status).toEqual(400);
     });
   });
-
-  describe("viewer", () => {
-    it("should allow attachment creation for documents in collections with edit access", async () => {
-      const user = await buildViewer();
-      const collection = await buildCollection({
-        teamId: user.teamId,
-        permission: null,
-      });
-      const document = await buildDocument({
-        teamId: user.teamId,
-        collectionId: collection.id,
-      });
-
-      await UserMembership.create({
-        createdById: user.id,
-        collectionId: collection.id,
-        userId: user.id,
-        permission: CollectionPermission.ReadWrite,
-      });
-
-      const res = await server.post("/api/attachments.create", {
-        body: {
-          name: "test.png",
-          contentType: "image/png",
-          size: 1000,
-          documentId: document.id,
-          preset: AttachmentPreset.DocumentAttachment,
-          token: user.getJwtToken(),
-        },
-      });
-      expect(res.status).toEqual(200);
-    });
-
-    it("should not allow attachment creation for documents", async () => {
-      const user = await buildViewer();
-      const document = await buildDocument({ teamId: user.teamId });
-
-      const res = await server.post("/api/attachments.create", {
-        body: {
-          name: "test.png",
-          contentType: "image/png",
-          size: 1000,
-          documentId: document.id,
-          preset: AttachmentPreset.DocumentAttachment,
-          token: user.getJwtToken(),
-        },
-      });
-      expect(res.status).toEqual(403);
-    });
-
-    it("should allow upload using avatar preset", async () => {
-      const user = await buildViewer();
-      const res = await server.post("/api/attachments.create", {
-        body: {
-          name: "test.png",
-          contentType: "image/png",
-          size: 1000,
-          preset: AttachmentPreset.Avatar,
-          token: user.getJwtToken(),
-        },
-      });
-      expect(res.status).toEqual(200);
-    });
-  });
 });
 
 describe("#attachments.delete", () => {

shared/i18n/locales/en_US/translation.json

@@ -328,7 +328,6 @@
   "Add": "Add",
   "Add or invite": "Add or invite",
   "Viewer": "Viewer",
-  "Editor": "Editor",
   "Suggestions for invitation": "Suggestions for invitation",
   "No matches": "No matches",
   "Can view": "Can view",
@@ -730,6 +729,7 @@
   "Invited {{roleName}} will receive access to": "Invited {{roleName}} will receive access to",
   "{{collectionCount}} collections": "{{collectionCount}} collections",
   "Can manage all workspace settings": "Can manage all workspace settings",
+  "Editor": "Editor",
   "Can create, edit, and delete documents": "Can create, edit, and delete documents",
   "Can view and comment": "Can view and comment",
   "Invite people to join your workspace. They can sign in with {{signinMethods}} or use their email address.": "Invite people to join your workspace. They can sign in with {{signinMethods}} or use their email address.",