Merge pull request #2345 from flowforge/team-slug-api

Remove overloading of /api/v1/teams endpoint

forge/lib/permissions.js

@@ -9,6 +9,7 @@ const Permissions = {
     'user:team:list': { description: 'List a Users teams', role: Roles.Admin, self: true },
     // Team Scoped Actions
     'team:create': { description: 'Create Team' },
+    'team:list': { description: 'List Teams', role: Roles.Admin },
     'team:read': { description: 'View a Team', role: Roles.Dashboard },
     'team:edit': { description: 'Edit Team', role: Roles.Owner },
     'team:delete': { description: 'Delete Team', role: Roles.Owner },

forge/routes/api/team.js

@@ -20,8 +20,21 @@ const TeamMembers = require('./teamMembers.js')
  */
 module.exports = async function (app) {
     app.addHook('preHandler', async (request, reply) => {
-        if (request.params.teamId !== undefined) {
-            if (request.params.teamId) {
+        if (request.params.teamId !== undefined || request.params.teamSlug !== undefined) {
+            // The route may provide either :teamId or :teamSlug
+            if (request.params.teamId || request.params.teamSlug) {
+                let teamId = request.params.teamId
+                if (request.params.teamSlug) {
+                    // If :teamSlug is provided, need to lookup the team to get
+                    // its id for subsequent checks
+                    request.team = await app.db.models.Team.bySlug(request.params.teamSlug)
+                    if (!request.team) {
+                        reply.code(404).send({ code: 'not_found', error: 'Not Found' })
+                        return
+                    }
+                    teamId = request.team.hashid
+                }
+
                 try {
                     if (!request.session.User) {
                         // If request.session.User is not defined, this request is being
@@ -39,7 +52,7 @@ module.exports = async function (app) {
                                 }
                             })
                             // Ensure the token's project is in the team being accessed
-                            if (project && project.Team.hashid === request.params.teamId) {
+                            if (project && project.Team.hashid === teamId) {
                                 return
                             }
                         } else if (request.session.ownerType === 'device') {
@@ -54,21 +67,24 @@ module.exports = async function (app) {
                                 }
                             })
                             // Ensure the device is in the team being accessed
-                            if (device && device.Team.hashid === request.params.teamId) {
+                            if (device && device.Team.hashid === teamId) {
                                 return
                             }
                         }
                         reply.code(404).send({ code: 'not_found', error: 'Not Found' })
                         return
                     }
-                    request.teamMembership = await request.session.User.getTeamMembership(request.params.teamId)
+                    request.teamMembership = await request.session.User.getTeamMembership(teamId)
                     if (!request.teamMembership && !request.session.User?.admin) {
                         reply.code(404).send({ code: 'not_found', error: 'Not Found' })
                         return
                     }
-                    request.team = await app.db.models.Team.byId(request.params.teamId)
                     if (!request.team) {
-                        reply.code(404).send({ code: 'not_found', error: 'Not Found' })
+                        // For a :teamId route, we can now lookup the full team object
+                        request.team = await app.db.models.Team.byId(request.params.teamId)
+                        if (!request.team) {
+                            reply.code(404).send({ code: 'not_found', error: 'Not Found' })
+                        }
                     }
                 } catch (err) {
                     reply.code(404).send({ code: 'not_found', error: 'Not Found' })
@@ -143,37 +159,29 @@ module.exports = async function (app) {
     })
 
     /**
-     * Return all teams (admin-only) or details of a specific team if 'slug' query
-     * parameter is set
+     * Get the details of a team via its slug
      *
-     * @name /api/v1/teams
+     * @name /api/v1/teams/slug/:teamSlug
      * @static
      * @memberof forge.routes.api.team
      */
-    app.get('/', async (request, reply) => {
-        // This isn't the most pleasant overloading of an api end-point.
-        // We can probably do better.
-        if (request.query.slug) {
-            const team = await app.db.models.Team.bySlug(request.query.slug)
-            if (team) {
-                const teamMembership = await request.session.User.getTeamMembership(team.id)
-                if (!teamMembership && !request.session.User?.admin) {
-                    reply.code(404).send({ code: 'not_found', error: 'Not Found' })
-                    return
-                }
-                await getTeamDetails(request, reply, team)
-            } else {
-                reply.code(404).send({ code: 'not_found', error: 'Not Found' })
-            }
-        } else if (!request.session.User || !request.session.User.admin) {
-            reply.code(401).send({ code: 'unauthorized', error: 'unauthorized' })
-        } else {
-            // Admin request for all teams
-            const paginationOptions = app.getPaginationOptions(request)
-            const teams = await app.db.models.Team.getAll(paginationOptions)
-            teams.teams = teams.teams.map(t => app.db.views.Team.team(t))
-            reply.send(teams)
-        }
+    app.get('/slug/:teamSlug', {
+        preHandler: app.needsPermission('team:read')
+    }, async (request, reply) => {
+        await getTeamDetails(request, reply, request.team)
+    })
+
+    /**
+     * Get a list of all teams (admin-only)
+     */
+    app.get('/', {
+        preHandler: app.needsPermission('team:list')
+    }, async (request, reply) => {
+        // Admin request for all teams
+        const paginationOptions = app.getPaginationOptions(request)
+        const teams = await app.db.models.Team.getAll(paginationOptions)
+        teams.teams = teams.teams.map(t => app.db.views.Team.team(t))
+        reply.send(teams)
     })
 
     /**

frontend/src/api/team.js

@@ -21,7 +21,7 @@ const getTeams = () => {
 const getTeam = (team) => {
     let url
     if (typeof team === 'object') {
-        url = `/api/v1/teams/?slug=${team.slug}`
+        url = `/api/v1/teams/slug/${team.slug}`
     } else {
         url = `/api/v1/teams/${team}`
     }

test/e2e/frontend/cypress/support/commands.js

@@ -81,14 +81,14 @@ Cypress.Commands.add('enableBilling', () => {
         })
     }).as('getSettings')
 
-    cy.intercept('/api/*/teams/*', (req) => {
+    cy.intercept('/api/*/teams/slug/*', (req) => {
         req.reply((response) => {
             response.body.billing = {
                 active: true
             }
             return response
         })
-    }).as('getTeam')
+    }).as('getTeamBySlug')
 
     cy.intercept('/api/*/teams/*', (req) => {
         req.reply((response) => {

test/e2e/frontend/cypress/tests/applications.spec.js

@@ -281,6 +281,7 @@ describe('FlowForge - Applications - With Billing', () => {
 
             cy.visit(`/team/${team.slug}/applications/create`)
 
+            cy.wait('@getTeamBySlug')
             cy.wait('@getTeamBilling')
 
             cy.get('[data-el="charges-table"]').should('not.exist')

test/e2e/frontend/cypress/tests/team/billing.js

@@ -11,7 +11,7 @@ describe('FlowForge - Team Billing', () => {
             cy.request('GET', 'api/v1/teams').then((response) => {
                 const team = response.body.teams[0]
 
-                cy.visit(`/team/${team.slug}/biling`)
+                cy.visit(`/team/${team.slug}/billing`)
 
                 cy.get('[data-el="credit-balance-banner"]').should('not.exist')
             })
@@ -26,6 +26,8 @@ describe('FlowForge - Team Billing', () => {
                 const team = response.body.teams[0]
 
                 cy.visit(`/team/${team.slug}/billing`)
+                cy.wait('@getTeamBySlug')
+                cy.wait('@getTeamBilling')
 
                 cy.get('[data-el="credit-balance-banner"]').should('exist').contains('$43.21')
             })

test/unit/forge/routes/api/team_spec.js

@@ -69,13 +69,65 @@ describe('Team API', function () {
 
     describe('Team API', function async () {
         describe('Get team details', async function () {
-            // GET /api/v1/teams/:teamId
-            // - Must be admin or team owner/member
+            it('admin can access any team', async function () {
+                const response = await app.inject({
+                    method: 'GET',
+                    url: `/api/v1/teams/${TestObjects.BTeam.hashid}`,
+                    cookies: { sid: TestObjects.tokens.alice }
+                })
+                response.statusCode.should.equal(200)
+                const result = response.json()
+                result.should.have.property('id', TestObjects.BTeam.hashid)
+            })
+            it('member can access team', async function () {
+                const response = await app.inject({
+                    method: 'GET',
+                    url: `/api/v1/teams/${TestObjects.BTeam.hashid}`,
+                    cookies: { sid: TestObjects.tokens.bob }
+                })
+                response.statusCode.should.equal(200)
+                const result = response.json()
+                result.should.have.property('id', TestObjects.BTeam.hashid)
+            })
+            it('non-member cannot access team', async function () {
+                const response = await app.inject({
+                    method: 'GET',
+                    url: `/api/v1/teams/${TestObjects.BTeam.hashid}`,
+                    cookies: { sid: TestObjects.tokens.chris }
+                })
+                response.statusCode.should.equal(404)
+            })
         })
 
         describe('Get team details by slug', async function () {
-            // GET /api/v1/teams/:teamId?slug=<teamSlug>
-            // - Must be admin or team owner/member
+            it('admin can access any team', async function () {
+                const response = await app.inject({
+                    method: 'GET',
+                    url: `/api/v1/teams/slug/${TestObjects.BTeam.slug}`,
+                    cookies: { sid: TestObjects.tokens.alice }
+                })
+                response.statusCode.should.equal(200)
+                const result = response.json()
+                result.should.have.property('id', TestObjects.BTeam.hashid)
+            })
+            it('member can access team', async function () {
+                const response = await app.inject({
+                    method: 'GET',
+                    url: `/api/v1/teams/slug/${TestObjects.BTeam.slug}`,
+                    cookies: { sid: TestObjects.tokens.bob }
+                })
+                response.statusCode.should.equal(200)
+                const result = response.json()
+                result.should.have.property('id', TestObjects.BTeam.hashid)
+            })
+            it('non-member cannot access team', async function () {
+                const response = await app.inject({
+                    method: 'GET',
+                    url: `/api/v1/teams/slug/${TestObjects.BTeam.slug}`,
+                    cookies: { sid: TestObjects.tokens.chris }
+                })
+                response.statusCode.should.equal(404)
+            })
         })
 
         describe('Get list of teams', async function () {

test/unit/frontend/api/team.spec.js

@@ -71,7 +71,7 @@ describe('Team API', async () => {
         const teamslug = 'teamslug'
         TeamAPI.default.getTeam({ slug: teamslug })
         expect(mockGet).toHaveBeenCalledOnce()
-        expect(mockGet).toHaveBeenCalledWith('/api/v1/teams/?slug=' + teamslug)
+        expect(mockGet).toHaveBeenCalledWith('/api/v1/teams/slug/' + teamslug)
     })
 
     test('getTeam calls the correct API endpoint when provided a string', () => {