Feat add api endoints for file storage 2082 #4715

apburnes · 2025-02-06T23:22:14Z

Changes proposed in this pull request:

Closes https://github.com/cloud-gov/private/issues/2082

security considerations

Adds api endpoints for managing file storage service

cloud-gov-pages-operations · 2025-02-06T23:33:44Z

🤖 This is an automated code coverage report

Total coverage (lines): 18.3%
Coverage diff: 0% 📈

drewbo

Great, well thought-out feature with tests to boot. I added a few notes on some small changes that I think could clean up the code a little bit but overall looks great. I didn't focus too much on models since I had already seen most of that.

Haven't checked on dev yet but I'll see if I can mess with the functionality today

drewbo · 2025-02-07T14:00:34Z

api/authorizers/file-storage.js

+const { FileStorageService, Site } = require('../models');
+const { isSiteOrgManager, isOrgManager, isOrgUser } = require('./utils');
+
+const canCreateSiteStorage = async ({ id: userId }, { id: siteId }) => {


All of canCreateSiteStorage, canAdminCreateSiteFileStorage, isFileStorageManager, and isFileStorageUser have function signatures that are nice for tests but when used in controllers require constructing an object, just to destructure it immediately. I think a "more ergonomic" signature would be fn({ userId, siteId }) or fn(userId, siteId). Some of these functions also return a single-property object which is immediately destructured, it might be easier to just return the property directly

We had this fn({ id: userId }, { id: siteId }) convention with other authorizers so I kept it but it did give me a bit of a bad taste. I like the fn(userId, siteId) so I will update them to that new signature.

There are a few authorizers that are returning the model instance we pass in the function argument. I will leave those as is and not update them to the new signature.

drewbo · 2025-02-07T14:10:24Z

api/authorizers/utils.js

+const { Organization, Site } = require('../models');
+const { fetchModelById } = require('../utils/queryDatabase');
+
+const authorize = async ({ id: userId }, { id: siteId }) => {


Same comment in this file regarding function signatures and return values

drewbo · 2025-02-07T14:45:57Z

api/controllers/file-storage-service.js

+const { SiteFileStorageSerivce } = require('../services/file-storage');
+const badRequest = require('../responses/badRequest');
+
+module.exports = wrapHandlers({


Because we need to create the service/client in each method (to ensure we have s3 credentials), there's a decent chunk of repeated code throughout the controller:

const siteStorageService = new SiteFileStorageSerivce(fileStorageService, user.id); const client = await siteStorageService.createClient(); const results = await client.<some method> ...

For now it might be nice to wrap this in a helper function to DRY the code a bit. In the future, we could potentially optimize this more by caching the S3 credentials to avoid needing to fetch them on each request. (but for now it feels like premature optimization)

For DRY, this seems like a bit premature for how often we use the class but there could be two options to optimize.

Create a private method that checks if the credentials have been fetched for all methods and then either fetches or uses the instance credentials when a method is called.

const client = new SiteFileStorageSerivce(fileStorageService, user.id); const results = await client.<some method>

Or we create a functions that instantiate the class, create the client and then pass its args to the method.

async function createFile(fileStorageService, userId, key, name, blob) { const siteStorageService = new SiteFileStorageSerivce(fileStorageService, userId); const client = await siteStorageService.createClient(); return client.createFile(key, name, blob) }

Our S3 services rotate credentials so we would have to build some type of credentials session to share between instances and identify the refresh schedule. That adds additional work beyond any possible gains and UX improvements at this point in development. Tracking this going forward would be worth while to track and make that decision at a later date.

drewbo · 2025-02-07T14:48:11Z

api/middlewares/mulipart-form.js

@@ -0,0 +1,6 @@
+const multer = require('multer');


Any concerns with using this package because of age?

I went with multer because it was specifically called out in the middleware docs but we can test it against formidable since that package has a more recent update cadence.

drewbo · 2025-02-07T15:03:09Z

api/services/file-storage/index.js

+    return result;
+  }
+
+  async deleteDirectory() {}


Can be removed?

drewbo · 2025-02-07T15:09:40Z

api/utils/index.js

  const limit = toInt(params.limit) || 25;
  const page = toInt(params.page) || 1;
  const offset = limit * (page - 1);

  const pQuery = {
    limit,
    offset,
-    order: [['createdAt', 'DESC']],
+    order: [order],


This might have already been incorrect but api/admin/controllers/domain.js uses the paginate function twice with order supplied in the query; I'd check that this doesn't create two separate sorts

I'll take a look

It is just overwriting the preset order if the order is added in the query params. I'm going to move the order into the query params.

api/utils/index.js

drewbo · 2025-02-07T19:15:58Z

api/admin/controllers/file-storage.js

+      const data = serializeFileStorageService(fss);
+      return res.send(data);
+    } catch (error) {
+      return res.status(error.status).send(error);


One issue with this pattern is that unhandled errors won't be logged anywhere, it will first show that it doesn't have an associated status:
TypeError: Invalid status code: undefined. Status code must be an intege

cg-ci-bot · 2025-02-11T22:35:32Z

🤖 This is an automated code coverage report

Total coverage (lines): 18.3%
Coverage diff: 0% 📈

Signed-off-by: Andrew Burnes <[email protected]>

drewbo requested changes Feb 7, 2025

View reviewed changes

drewbo reviewed Feb 7, 2025

View reviewed changes

drewbo force-pushed the feat-add-api-endoints-for-file-storage-2082 branch from 1fba65f to 84a7e3e Compare February 7, 2025 19:18

apburnes force-pushed the feat-add-api-endoints-for-file-storage-2082 branch from 84a7e3e to c550bc2 Compare February 10, 2025 19:16

drewbo previously approved these changes Feb 12, 2025

View reviewed changes

apburnes dismissed drewbo’s stale review via a84c028 February 12, 2025 22:05

apburnes requested a review from drewbo February 12, 2025 22:06

apburnes force-pushed the feat-add-api-endoints-for-file-storage-2082 branch from a84c028 to ebc2984 Compare February 12, 2025 22:27

drewbo approved these changes Feb 13, 2025

View reviewed changes

apburnes force-pushed the feat-add-api-endoints-for-file-storage-2082 branch 2 times, most recently from ea73f7a to 1b126fc Compare February 13, 2025 20:56

feat: Add API endpoints for file storage #2082

2a7a687

Signed-off-by: Andrew Burnes <[email protected]>

apburnes force-pushed the feat-add-api-endoints-for-file-storage-2082 branch from 1b126fc to 2a7a687 Compare February 13, 2025 21:07

apburnes merged commit 6f5c141 into main Feb 13, 2025
7 of 8 checks passed

apburnes deleted the feat-add-api-endoints-for-file-storage-2082 branch February 13, 2025 21:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feat add api endoints for file storage 2082 #4715

Feat add api endoints for file storage 2082 #4715

apburnes commented Feb 6, 2025 •

edited

Loading

cloud-gov-pages-operations commented Feb 6, 2025 •

edited

Loading

drewbo left a comment •

edited

Loading

drewbo Feb 7, 2025 •

edited

Loading

apburnes Feb 10, 2025

apburnes Feb 10, 2025 •

edited

Loading

drewbo Feb 7, 2025 •

edited

Loading

drewbo Feb 7, 2025 •

edited

Loading

apburnes Feb 12, 2025

drewbo Feb 7, 2025

apburnes Feb 10, 2025

drewbo Feb 7, 2025

drewbo Feb 7, 2025

apburnes Feb 10, 2025

apburnes Feb 10, 2025

drewbo Feb 7, 2025

apburnes Feb 10, 2025

cg-ci-bot commented Feb 11, 2025

Feat add api endoints for file storage 2082 #4715

Feat add api endoints for file storage 2082 #4715

Conversation

apburnes commented Feb 6, 2025 • edited Loading

Changes proposed in this pull request:

security considerations

cloud-gov-pages-operations commented Feb 6, 2025 • edited Loading

🤖 This is an automated code coverage report

drewbo left a comment • edited Loading

Choose a reason for hiding this comment

drewbo Feb 7, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apburnes Feb 10, 2025 • edited Loading

Choose a reason for hiding this comment

drewbo Feb 7, 2025 • edited Loading

Choose a reason for hiding this comment

drewbo Feb 7, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cg-ci-bot commented Feb 11, 2025

🤖 This is an automated code coverage report

apburnes commented Feb 6, 2025 •

edited

Loading

cloud-gov-pages-operations commented Feb 6, 2025 •

edited

Loading

drewbo left a comment •

edited

Loading

drewbo Feb 7, 2025 •

edited

Loading

apburnes Feb 10, 2025 •

edited

Loading

drewbo Feb 7, 2025 •

edited

Loading

drewbo Feb 7, 2025 •

edited

Loading