Merge pull request #2198 from overleaf/ta-user-membership-template-graph-fix

New Approach to Template Graph Access Fix

GitOrigin-RevId: 5865d8cfaf6f825f8cb76724a04091f3659f9f0f

services/web/app/src/Features/UserMembership/UserMembershipMiddleware.js

@@ -132,7 +132,7 @@ let UserMembershipMiddleware = {
     fetchV1Template(),
     requireV1Template(),
     fetchEntityConfig('publisher'),
-    fetchEntityIfSet(), // at this point the entity is the template's publisher, if any
+    fetchPublisherFromTemplate(),
     allowAccessIfAny([
       UserMembershipAuthorization.hasEntityAccess(),
       UserMembershipAuthorization.hasStaffAccess('publisherMetrics')
@@ -175,9 +175,6 @@ let UserMembershipMiddleware = {
         new HttpErrors.NotFoundError(`incorrect entity name: ${entityName}`)
       )
     }
-    if (entityName === 'Template') {
-      req.params.templateId = req.params.id
-    }
     // run the list of middleware functions in series. This is essencially
     // a poor man's middleware runner
     async.eachSeries(middleware, (fn, callback) => fn(req, res, callback), next)
@@ -208,12 +205,16 @@ function fetchEntity() {
   })
 }
 
-function fetchEntityIfSet() {
+function fetchPublisherFromTemplate() {
   return (req, res, next) => {
-    if (!req.params.id) {
+    if (req.template.brand.slug) {
+      // set the id as the publisher's id as it's the entity used for access
+      // control
+      req.params.id = req.template.brand.slug
+      return fetchEntity()(req, res, next)
+    } else {
       return next()
     }
-    return fetchEntity()(req, res, next)
   }
 }
 
@@ -252,18 +253,13 @@ function requireEntityOrCreate(creationStaffAccess) {
 // fetch the template from v1, and set it in the request
 function fetchV1Template() {
   return expressify(async (req, res, next) => {
-    const templateId = req.params.templateId
+    const templateId = req.params.id
     const body = await TemplatesManager.promises.fetchFromV1(templateId)
     req.template = {
       id: body.id,
       title: body.title,
       brand: body.brand
     }
-    if (req.template.brand.slug) {
-      // set the id as the publisher's id as it's the entity used for access
-      // control
-      req.params.id = req.template.brand.slug
-    }
     next()
   })
 }

services/web/test/acceptance/src/UserMembershipAuthorizationTests.js

@@ -339,28 +339,62 @@ describe('UserMembershipAuthorization', function() {
   })
 
   describe('graph', function() {
-    it('allow admins only', function(done) {
-      const url = '/graphs/foo?resource_type=admin'
-      async.series(
-        [
-          this.user.login.bind(this.user),
-          expectAccess(this.user, url, 403),
-          this.user.ensureAdmin.bind(this.user),
-          this.user.login.bind(this.user),
-          expectAccess(this.user, url, 200)
-        ],
-        done
-      )
+    describe('admin', function() {
+      it('allow admins only', function(done) {
+        const url = '/graphs/foo?resource_type=admin'
+        async.series(
+          [
+            this.user.login.bind(this.user),
+            expectAccess(this.user, url, 403),
+            this.user.ensureAdmin.bind(this.user),
+            this.user.login.bind(this.user),
+            expectAccess(this.user, url, 200)
+          ],
+          done
+        )
+      })
+
+      it('handle missing resource type', function(done) {
+        const url = '/graphs/foo'
+        expectAccess(this.user, url, 404)(done)
+      })
+
+      it('handle incorrect resource type', function(done) {
+        const url = '/graphs/foo?resource_type=evil'
+        expectAccess(this.user, url, 404)(done)
+      })
     })
 
-    it('handle missing resource type', function(done) {
-      const url = '/graphs/foo'
-      expectAccess(this.user, url, 404)(done)
-    })
+    describe('template', function() {
+      beforeEach(function(done) {
+        this.publisher = new Publisher({})
+        async.series(
+          [
+            this.publisher.ensureExists.bind(this.publisher),
+            cb => this.user.login(cb)
+          ],
+          done
+        )
+      })
 
-    it('handle incorrect resource type', function(done) {
-      const url = '/graphs/foo?resource_type=evil'
-      expectAccess(this.user, url, 404)(done)
+      it('get template graphs', function(done) {
+        MockV1Api.setTemplates({
+          123: {
+            id: 123,
+            title: '123 title',
+            brand: { slug: this.publisher.slug }
+          }
+        })
+        const url = '/graphs/foo?resource_type=template&resource_id=123'
+        async.series(
+          [
+            this.user.ensureAdmin.bind(this.user),
+            this.user.login.bind(this.user),
+            expectAccess(this.user, url, 200)
+          ],
+          done
+        )
+      })
     })
   })