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

API: Allow CORS API requests no matter what FQDN the UI is hosted on #441

Merged
merged 5 commits into from
Nov 29, 2024

Conversation

peteski22
Copy link
Contributor

@peteski22 peteski22 commented Nov 26, 2024

What's changing

This PR changes the backend API configuration to allow setting the CORS allowed origins that are accepted. It also defaults to http://localhost and http://localhost:3000 when nothing is configured.

Previously it was limited to only accepting requests from the UI when it the UI is hosted via HTTP on localhost ports 80 or 3000.

How to test it

  • gh pr checkout 441

1. No environment variable set

This example starts Lumigator with no env var configured, the default will be used.

  • make local-up
  • cd lumigator/frontend
  • npm install
  • npm run dev

Open http://127.0.0.1:3000 in a browser and the health check (top right) should report OK (no errors shown, no errors in the developer tools network tab/console).

Also the logs in the backend container will show:

INFO     | backend.main:create_app:57 - Configuring CORS for API, allowed origins: ['http://localhost', 'http://localhost:3000']

2. Configure local environment variable

This example starts Lumigator specifying a list of allowed origins that should be used.

  • LUMI_API_CORS_ALLOWED_ORIGINS="http://localhost:8080" make local-up
    (note: you can also use LUMI_API_CORS_ALLOWED_ORIGINS="*" make local-up to allow all)
  • cd lumigator/frontend
  • npm install
  • npm run build
  • cd dist
  • npx http-server

Open http://127.0.0.1:8080 in a browser and the health check (top right) should report OK (no errors shown, no errors in the developer tools network tab/console).

Also the logs in the backend container will show:

INFO     | backend.main:create_app:57 - Configuring CORS for API, allowed origins: ['http://localhost:8080']

3. Configure environment variable without required origin

This example starts Lumigator, specifying allowed origins, but NOT the one for 8080 which is required for our test UI to work.

  • LUMI_API_CORS_ALLOWED_ORIGINS="http://localhost:9999" make local-up
  • cd lumigator/frontend
  • npm install
  • npm run build
  • cd dist
  • npx http-server

Open http://127.0.0.1:8080 in a browser and the health check (top right) should fail.

image

image

Also the logs in the backend container will show:

INFO     | backend.main:create_app:57 - Configuring CORS for API, allowed origins: ['http://localhost:9999']

Additional notes for reviewers

The UI is hosted on 8080 in this example, which means a CORS pre-flight OPTIONS request will be sent to the backend asking if it allows cross-domain requests from 'http://localhost:8080' (the scheme+host+port are important to CORS).

Previously only http://localhost:80 and http://localhost:3000 were allowed so the UI would fail to make requests to the API to get health status, datasets etc. but now it works.

I already...

  • added some tests for any new functionality
  • updated the documentation
  • checked if a (backend) DB migration step was required and included it if required

@peteski22 peteski22 added backend api Changes which impact API/presentation layer labels Nov 26, 2024
@peteski22 peteski22 marked this pull request as draft November 27, 2024 10:51
@peteski22 peteski22 force-pushed the api/allow-all-origins branch from cd952b3 to 7a2db88 Compare November 29, 2024 13:27
@peteski22 peteski22 marked this pull request as ready for review November 29, 2024 13:35
@agpituk
Copy link
Contributor

agpituk commented Nov 29, 2024

Thanks for all the changes! looks good!! :)

@peteski22 peteski22 merged commit 6fa429b into main Nov 29, 2024
9 checks passed
@peteski22 peteski22 deleted the api/allow-all-origins branch November 29, 2024 15:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api Changes which impact API/presentation layer backend
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants