From 74a79837576bf5de0242ecc42374bbfbf5c5eb15 Mon Sep 17 00:00:00 2001 From: Trevor Buckner Date: Sun, 8 Dec 2024 23:39:26 -0500 Subject: [PATCH 1/3] Refactor and clean up "getBrew()" Some redundant logic and sprawling formatting --- server/homebrew.api.js | 70 ++++++++++++++++++------------------------ 1 file changed, 30 insertions(+), 40 deletions(-) diff --git a/server/homebrew.api.js b/server/homebrew.api.js index b8d4024ad..f536ec45e 100644 --- a/server/homebrew.api.js +++ b/server/homebrew.api.js @@ -87,76 +87,66 @@ const api = { // Create middleware with the accessType passed in as part of the scope return async (req, res, next)=>{ // Get relevant IDs for the brew - const { id, googleId } = api.getId(req); + let { id, googleId } = api.getId(req); const accessMap = { edit : { editId: id }, share : { shareId: id }, - admin : { - $or : [ - { editId: id }, - { shareId: id }, - ] } + admin : { $or : [{ editId: id }, { shareId: id }] } }; // Try to find the document in the Homebrewery database -- if it doesn't exist, that's fine. let stub = await HomebrewModel.get(accessMap[accessType]) .catch((err)=>{ - if(googleId) { + if(googleId) console.warn(`Unable to find document stub for ${accessType}Id ${id}`); - } else { + else console.warn(err); - } }); stub = stub?.toObject(); + googleId ??= stub?.googleId; + + const isAuthor = stub?.authors?.includes(req.account?.username); + const isInvited = stub?.invitedAuthors?.includes(req.account?.username); + + if(accessType === 'edit' && !(isOwner || isAuthor || isInvited)) { + const accessError = { name: 'Access Error', status: 401, authors: stub.authors, brewTitle: stub.title, shareId: stub.shareId }; + if(req.account) + throw { ...accessError, message: 'User is not an Author', HBErrorCode: '03' }; + else + throw { ...accessError, message: 'User is not logged in', HBErrorCode: '04' }; + } if(stub?.lock?.locked && accessType != 'edit') { throw { HBErrorCode: '51', code: stub.lock.code, message: stub.lock.shareMessage, brewId: stub.shareId, brewTitle: stub.title }; } // If there is a google id, try to find the google brew - if(!stubOnly && (googleId || stub?.googleId)) { - let googleError; const googleBrew = await GoogleActions.getGoogleBrew(googleId || stub?.googleId, id, accessType) - .catch((err)=>{ - googleError = err; + if(!stubOnly && googleId) { + + .catch((googleError)=>{ + const reason = googleError.errors?.[0].reason; + if(reason == 'notFound') + throw { ...googleError, HBErrorCode: '02', authors: stub?.authors, account: req.account?.username }; + else + throw { ...googleError, HBErrorCode: '01' }; }); - // Throw any error caught while attempting to retrieve Google brew. - if(googleError) { - const reason = googleError.errors?.[0].reason; - if(reason == 'notFound') { - throw { ...googleError, HBErrorCode: '02', authors: stub?.authors, account: req.account?.username }; - } else { - throw { ...googleError, HBErrorCode: '01' }; - } - } + // Combine the Homebrewery stub with the google brew, or if the stub doesn't exist just use the google brew stub = stub ? _.assign({ ...api.excludeStubProps(stub), stubbed: true }, api.excludeGoogleProps(googleBrew)) : googleBrew; } - const authorsExist = stub?.authors?.length > 0; - const isAuthor = stub?.authors?.includes(req.account?.username); - const isInvited = stub?.invitedAuthors?.includes(req.account?.username); - if(accessType === 'edit' && (authorsExist && !(isAuthor || isInvited))) { - const accessError = { name: 'Access Error', status: 401 }; - if(req.account){ - throw { ...accessError, message: 'User is not an Author', HBErrorCode: '03', authors: stub.authors, brewTitle: stub.title, shareId: stub.shareId }; - } - throw { ...accessError, message: 'User is not logged in', HBErrorCode: '04', authors: stub.authors, brewTitle: stub.title, shareId: stub.shareId }; - } // If after all of that we still don't have a brew, throw an exception - if(!stub && !stubOnly) { + if(!stub) throw { name: 'BrewLoad Error', message: 'Brew not found', status: 404, HBErrorCode: '05', accessType: accessType, brewId: id }; - } // Clean up brew: fill in missing fields with defaults / fix old invalid values - if(stub) { - stub.tags = stub.tags || undefined; // Clear empty strings - stub.renderer = stub.renderer || undefined; // Clear empty strings - stub = _.defaults(stub, DEFAULT_BREW_LOAD); // Fill in blank fields - } + stub.tags = stub.tags || undefined; // Clear empty strings + stub.renderer = stub.renderer || undefined; // Clear empty strings + stub = _.defaults(stub, DEFAULT_BREW_LOAD); // Fill in blank fields - req.brew = stub ?? {}; + req.brew = stub; next(); }; }, From 9758797e2b1c887e335f12680f91b645a809b864 Mon Sep 17 00:00:00 2001 From: Trevor Buckner Date: Sun, 8 Dec 2024 23:42:14 -0500 Subject: [PATCH 2/3] If user is owner, fetch Google Brew with user auth Fixes the case where a user can see a Google Brew under their account (`listBrew()` uses their personal auth) but can't actually delete it (`getBrew()` only uses the serviceAccount). Occurs if a Google brew has lost its permissions somehow (set to "restricted", etc.) such that serviceAccount can no longer interact with it. --- server/googleActions.js | 4 ++-- server/homebrew.api.js | 4 +++- server/homebrew.api.spec.js | 2 +- 3 files changed, 6 insertions(+), 4 deletions(-) diff --git a/server/googleActions.js b/server/googleActions.js index ec541e5f5..51da56218 100644 --- a/server/googleActions.js +++ b/server/googleActions.js @@ -241,8 +241,8 @@ const GoogleActions = { return obj.data.id; }, - getGoogleBrew : async (id, accessId, accessType)=>{ - const drive = googleDrive.drive({ version: 'v3', auth: defaultAuth }); + getGoogleBrew : async (auth, id, accessId, accessType)=>{ + const drive = googleDrive.drive({ version: 'v3', auth: auth || defaultAuth }); const obj = await drive.files.get({ fileId : id, diff --git a/server/homebrew.api.js b/server/homebrew.api.js index f536ec45e..a75887742 100644 --- a/server/homebrew.api.js +++ b/server/homebrew.api.js @@ -106,6 +106,7 @@ const api = { stub = stub?.toObject(); googleId ??= stub?.googleId; + const isOwner = stub?.authors?.length === 0 || stub?.authors?.[0] === req.account?.username; const isAuthor = stub?.authors?.includes(req.account?.username); const isInvited = stub?.invitedAuthors?.includes(req.account?.username); @@ -122,9 +123,10 @@ const api = { } // If there is a google id, try to find the google brew - const googleBrew = await GoogleActions.getGoogleBrew(googleId || stub?.googleId, id, accessType) if(!stubOnly && googleId) { + const oAuth2Client = isOwner? GoogleActions.authCheck(req.account, res) : undefined; + const googleBrew = await GoogleActions.getGoogleBrew(oAuth2Client, googleId, id, accessType) .catch((googleError)=>{ const reason = googleError.errors?.[0].reason; if(reason == 'notFound') diff --git a/server/homebrew.api.spec.js b/server/homebrew.api.spec.js index 814db26f8..8270b1568 100644 --- a/server/homebrew.api.spec.js +++ b/server/homebrew.api.spec.js @@ -298,7 +298,7 @@ describe('Tests for api', ()=>{ expect(next).toHaveBeenCalled(); expect(api.getId).toHaveBeenCalledWith(req); expect(model.get).toHaveBeenCalledWith({ shareId: '1' }); - expect(google.getGoogleBrew).toHaveBeenCalledWith('2', '1', 'share'); + expect(google.getGoogleBrew).toHaveBeenCalledWith(undefined, '2', '1', 'share'); }); it('access is denied to a locked brew', async()=>{ From c7d94b077963fd5b964053ac84c95f5f7c8f350f Mon Sep 17 00:00:00 2001 From: Trevor Buckner Date: Sun, 8 Dec 2024 23:59:27 -0500 Subject: [PATCH 3/3] Small cleanup --- server/googleActions.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/server/googleActions.js b/server/googleActions.js index 51da56218..2c2cbac73 100644 --- a/server/googleActions.js +++ b/server/googleActions.js @@ -241,8 +241,8 @@ const GoogleActions = { return obj.data.id; }, - getGoogleBrew : async (auth, id, accessId, accessType)=>{ - const drive = googleDrive.drive({ version: 'v3', auth: auth || defaultAuth }); + getGoogleBrew : async (auth = defaultAuth, id, accessId, accessType)=>{ + const drive = googleDrive.drive({ version: 'v3', auth: auth }); const obj = await drive.files.get({ fileId : id,