Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

updates proxy to support access control allow list #3407

Merged
merged 6 commits into from
Apr 26, 2023

Conversation

seanmalbert
Copy link
Collaborator

@seanmalbert seanmalbert commented Apr 18, 2023

This fixes the cors issue application start pages are seeing by updating Nginx to support passing in an allow list (now a required parameter).

I also updated the README to include instructions for running locally.

04/24 update:
There is also an issue with the purge requests. This was because of two reasons.

  1. Newest NGINX version fails on the purge requests. My theory is that it is functionality that moved to the paid version of nginx.
    • Resolved by downgrading nginx to a version that works (still newer than what is in prod)
  2. The partner site was where we were making the call to the proxy for the purge after listing edit. This was causing a CORs error
    • Resolved by moving the purge calls to the backend.

@netlify
Copy link

netlify bot commented Apr 18, 2023

Deploy Preview for bloom-exygy-dev ready!

Name Link
🔨 Latest commit 87729d3
🔍 Latest deploy log https://app.netlify.com/sites/bloom-exygy-dev/deploys/6449593077a036000888adfa
😎 Deploy Preview https://deploy-preview-3407--bloom-exygy-dev.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@ludtkemorgan
Copy link
Collaborator

@seanmalbert this is working locally, but when I deploy it to Heroku the issue still persists. I deployed it and added the allow list here: https://dashboard.heroku.com/apps/bloom-backend-main-proxy/settings. But when going to https://bloom.exygy.dev/ and applying to a listing the issue still persists

@seanmalbert
Copy link
Collaborator Author

@seanmalbert this is working locally, but when I deploy it to Heroku the issue still persists. I deployed it and added the allow list here: https://dashboard.heroku.com/apps/bloom-backend-main-proxy/settings. But when going to https://bloom.exygy.dev/ and applying to a listing the issue still persists

There's something funny going on here that I ran into locally while testing things. The header is getting set, but incorrectly: The 'Access-Control-Allow-Origin' header contains multiple values 'https://bloom.exygy.dev, https://bloom.exygy.dev', but only one is allowed.

@ludtkemorgan
Copy link
Collaborator

The deployment issue has been resolved. It was applying two of the same header. Most recent change removes the header from the backend and reapplies it from the proxy.

@ludtkemorgan ludtkemorgan added the 2 reviews needed Requires 2 more review before ready to merge label Apr 18, 2023
@emilyjablonski
Copy link
Collaborator

@ludtkemorgan would you recommend testing locally or in a netlify environment?

@ludtkemorgan
Copy link
Collaborator

@ludtkemorgan would you recommend testing locally or in a netlify environment?

It is probably best to test it in the deployed environment https://bloom.exygy.dev/ and https://partners.bloom.exygy.dev/ since the issue is specific to the proxy setup

Copy link
Collaborator

@YazeedLoonat YazeedLoonat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @ludtkemorgan I think this looks good, I did run into that problem with the listings not being purged properly

I did the following:
look at public site and pick a listing
go to partner site and update the name of that listing
save and look back at the public site

the listing wasn't updated. I did try adding localhost:3100 localhost:3000|localhost:3001|localhost:3100 as the ALLOW_LIST variable and I was still unable to purge the listing

@YazeedLoonat YazeedLoonat added needs changes The author must make changes and then re-request review before merging and removed 2 reviews needed Requires 2 more review before ready to merge labels Apr 20, 2023
@ludtkemorgan
Copy link
Collaborator

@YazeedLoonat I moved the purge api call from the partner site to the backend and it is now working. Also I had to downgrade the version of nginx to solve the purge requests failing so you'll need to rebuild the proxy docker image locally again to test this.

@YazeedLoonat YazeedLoonat self-requested a review April 25, 2023 16:08
Copy link
Collaborator

@YazeedLoonat YazeedLoonat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @ludtkemorgan looks good, looking for your thoughts on await the proxy call

backend/core/src/listings/listings.service.ts Outdated Show resolved Hide resolved
@ludtkemorgan ludtkemorgan added 1 review needed Requires 1 more review before ready to merge and removed needs changes The author must make changes and then re-request review before merging labels Apr 25, 2023
@YazeedLoonat YazeedLoonat self-requested a review April 25, 2023 21:24
Copy link
Collaborator

@YazeedLoonat YazeedLoonat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@ludtkemorgan ludtkemorgan merged commit 3b816da into main Apr 26, 2023
ludtkemorgan added a commit to housingbayarea/bloom that referenced this pull request May 2, 2023
* feat: updates proxy to support access control allow list

* fix: remove downstream access-control-allow-origin

* fix: update readme for m1

* fix: move purge call to the backend

* fix: test fix and add await

* fix: moving cache purge to helper

---------

Co-authored-by: Morgan Ludtke <ludtkemorgan@gmail.com>
Co-authored-by: Yazeed Loonat <yazeedloonat@gmail.com>
ludtkemorgan added a commit to housingbayarea/bloom that referenced this pull request May 9, 2023
* feat: updates proxy to support access control allow list

* fix: remove downstream access-control-allow-origin

* fix: update readme for m1

* fix: move purge call to the backend

* fix: test fix and add await

* fix: moving cache purge to helper

---------

Co-authored-by: Morgan Ludtke <ludtkemorgan@gmail.com>
Co-authored-by: Yazeed Loonat <yazeedloonat@gmail.com>
ludtkemorgan added a commit to housingbayarea/bloom that referenced this pull request May 18, 2023
* fix: uptake latest uic Modal, ActionBlock, FormCard breaking changes (bloom-housing#3358)

* feat: upgrade react to 18 (bloom-housing#3360)

* feat: upgrade nextjs to 13 (bloom-housing#3375)

* feat: upgrade nextjs to 13

* fix: attempt to get cypress test working

* feat: changing auth over to cookies (bloom-housing#3357)

* fix: resolves issues around markedAsDuplicate (bloom-housing#3373)

* fix: react type errors (bloom-housing#3382)

* refactor: add cloudinary fxn to partners (bloom-housing#3393)

* refactor: uptake seeds FormErrorMessage (bloom-housing#3369)

* fix: add startDate to open house submit event (bloom-housing#3399)

* fix: add three new fields to base view (bloom-housing#3406)

* feat: removing sensative info from leasing agent (bloom-housing#3409)

* feat: removing sensative info from leasing agent

* fix: adding swagger changes

* fix: updates for tests

* chore(deps): bump cookiejar from 2.1.2 to 2.1.4 (bloom-housing#3295)

Bumps [cookiejar](https://github.com/bmeck/node-cookiejar) from 2.1.2 to 2.1.4.
- [Release notes](https://github.com/bmeck/node-cookiejar/releases)
- [Commits](https://github.com/bmeck/node-cookiejar/commits)

---
updated-dependencies:
- dependency-name: cookiejar
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* fix: updates around cookies (bloom-housing#3405)

* fix: updates around cookies

* fix: creating new migration for token -> code

* fix: Searching on the applications table causes the page to crash (bloom-housing#3408)

* fix: pass proper value to to_tsquery function

* fix: search applications using ILIKE

* fix: change where to orWhere

* feat: add application search by confirmation code

* updates proxy to support access control allow list (bloom-housing#3407)

* feat: updates proxy to support access control allow list

* fix: remove downstream access-control-allow-origin

* fix: update readme for m1

* fix: move purge call to the backend

* fix: test fix and add await

* fix: moving cache purge to helper

---------

Co-authored-by: Morgan Ludtke <ludtkemorgan@gmail.com>
Co-authored-by: Yazeed Loonat <yazeedloonat@gmail.com>

* fix: escape quote in translation update

* fix: add translation for 64 characters error (bloom-housing#3423)

* fix: downgrade version of nest axios (bloom-housing#3419)

* fix: now removes criteria file if a url is input (bloom-housing#3421)

* fix: remove check in test not applicable for hba

* fix: update ui-c to latest version (bloom-housing#3420)

* fix: update application test

* feat: 3291/listing export take 2 (bloom-housing#3424)

* fix: functional frontend

* fix: hooks and service updates

* fix: hitting service file

* fix: wip config work

* fix: wip config 2

* fix: completed query updates

* fix: complete column naming and basic formatting

* fix: clean up formatting

* fix: wip testing debugging

* fix: functional unit tests

* fix: cypress tests + formatting

* fix: unadded test changes

* fix: internal csv testing

* fix: exporter test fix

* fix: more detailed csv checks

* fix: testing + formatting tweaks

* fix: exporter testing improvements

* fix: updates per pr feedback

* fix: match config pattern

* fix: add close status option

* fix: reset netlify testing

* fix: final cleanup

* fix: rent type formatting

* fix: remove console log

* fix: missing state data (bloom-housing#3450)

* feat: adding knownError flag

* feat: adding knownError flag

* fix: partners highlight field on backend error (bloom-housing#3448)

* fix: partners highlight field on backend error

* fix: community type and disableUnitsAccordion fix

* fix: unit type fix for partial units

* fix: review comment addressed

* fix: phone number fix

---------

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: Yazeed Loonat <YazeedLoonat@gmail.com>
Co-authored-by: Emily Jablonski <65367387+emilyjablonski@users.noreply.github.com>
Co-authored-by: Krzysztof Zięcina <kziecina@airnauts.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Sean Albert <smabert@gmail.com>
Co-authored-by: ColinBuyck <53269332+ColinBuyck@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1 review needed Requires 1 more review before ready to merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants