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

Fix the cors issues #1396

Merged
merged 9 commits into from
Apr 17, 2023
Merged

Conversation

MichaelsJP
Copy link
Member

@MichaelsJP MichaelsJP commented Apr 12, 2023

it was previously not possible to read in list of strings from the
config file.
This can now be done in a generic way.### Pull Request Checklist

  • 1. I have rebased the latest version of the master branch into my feature branch and all conflicts
    have been resolved.
  • 2. I have added information about the change/addition to functionality to the CHANGELOG.md file under the
    [Unreleased] heading.
  • 3. I have documented my code using JDocs tags.
  • 4. I have removed unnecessary commented out code, imports and System.out.println statements.
  • 5. I have written JUnit tests for any new methods/classes and ensured that they pass.
  • 6. I have created API tests for any new functionality exposed to the API.
  • 7. If changes/additions are made to the ors-config.json file, I have added these to the ors config documentation
    along with a short description of what it is for, and documented this in the Pull Request (below).
  • 8. I have built graphs with my code of the Heidelberg.osm.gz file and run the api-tests with all test passing
  • 9. I have referenced the Issue Number in the Pull Request (if the changes were from an issue).
  • 10. For new features or changes involving building of graphs, I have tested on a larger dataset
    (at least Germany), and the graphs build without problems (i.e. no out-of-memory errors).
  • 11. For new features or changes involving the graphbuilding process (i.e. changing encoders, updating the
    importer etc.), I have generated longer distance routes for the affected profiles with different options
    (avoid features, max weight etc.) and compared these with the routes of the same parameters and start/end
    points generated from the current live ORS.
    If there are differences then the reasoning for these MUST be documented in the pull request.
  • 12. I have written in the Pull Request information about the changes made including their intended usage
    and why the change was needed.
  • 13. For changes touching the API documentation, I have tested that the API playground renders correctly.

Fixes #1394

Information about the changes

  • Key functionality added: Correct CORS handling and configuration
  • Reason for change: Currently, CORS cannot be handled correctly from the backend.

Examples and reasons for differences between live ORS routes, and those generated from this pull request

Required changes to ors config (if applicable)

"api_settings": {
      "cors": {
        "allowed": {
          "origins": [
			"*",
            "https://example.org"
          ],
          "methods": [
            "GET",
            "POST",
            "OPTIONS"
          ],
          "headers": [
            "Content-Type",
            "X-Requested-With",
            "accept",
            "Origin",
            "Access-Control-Request-Method",
            "Access-Control-Request-Headers",
            "Authorization"
          ]
        },
        "exposed": {
          "headers": [
            "Access-Control-Allow-Origin", "Access-Control-Allow-Credentials"
          ]
        },
        "preflight_max_age": 600
      }
    },

@takb takb added this to To do in ors general Apr 12, 2023
@MichaelsJP MichaelsJP linked an issue Apr 12, 2023 that may be closed by this pull request
@MichaelsJP MichaelsJP changed the title feat(AppConfig): allow AppConfig to read string-lists from path Fix the cors issues Apr 12, 2023
@MichaelsJP MichaelsJP force-pushed the feature/#994-adjustable-cors-settings branch 4 times, most recently from d39a269 to 4d3917e Compare April 13, 2023 17:11
it was previously not possible to read in list of strings from the
config file.
This can now be done in a generic way.
@MichaelsJP MichaelsJP force-pushed the feature/#994-adjustable-cors-settings branch from 4d3917e to c054aac Compare April 14, 2023 08:03
@MichaelsJP
Copy link
Member Author

@TheGreatRefrigerator Should we put some default values for the CORS settings?
Many users will use their old config and still have problems. We could avoid that by settings a wildcard by default?

@TheGreatRefrigerator
Copy link
Contributor

@TheGreatRefrigerator Should we put some default values for the CORS settings? Many users will use their old config and still have problems. We could avoid that by settings a wildcard by default?

Yes, the default values should mirror those we set in the sample config i guess.

@MichaelsJP MichaelsJP linked an issue Apr 14, 2023 that may be closed by this pull request
TheGreatRefrigerator and others added 7 commits April 14, 2023 18:43
there have been CORS issues when using our map clients with local
ors instances during development due to Problems with what headers are
accepted and exposed by the backend.
With the spring-boot update and the new run configurations using spring,
setting these options properly is now possible for local instances using
`spring-boot:run` or mvn version of it.

However, the docker approach is still using catalina + tomcat where
the settings would be passed differently, so they are not respected in
a local backend running with docker.

- add addCorsMappings function to ApiConfig class
- extent ors-config-sample.json with default cors settings working with
  all origins and accepting `Authorization` headers.
- if the api settings are missing from the ors config file,
 default values are used that correspond with the entries
 in the ors-config-sample.json
test all available api_settings as well as predefined defaults
for HTTP methods

- adjust ors-config.json for CORS tests
- switch up the api_settings values to avoid testing defaults
The web.xml is overwriting the native spring-boot CORS
settings when applied.
Removing them is enough to fall back to the spring-boot applied configurations.
This way CORS can be centrally controlled with the normal ors config files.

- change OpenRouteService to openrouteservice
These tests check the tomcat + war file deployment used in Docker,
which cannot be checked via the integration or unit tests.
The tests check with the wildcard from the default config first
and then replace the wildcard with https://example.org.
With that set, it checks for correct CORS issues by requesting
with origin https://example.com.
@TheGreatRefrigerator TheGreatRefrigerator force-pushed the feature/#994-adjustable-cors-settings branch from c054aac to b509221 Compare April 14, 2023 16:49
@TheGreatRefrigerator
Copy link
Contributor

Did some squashing and dropped the @CrossOrigin parts as they were overwriting the main CORS config, and resulted in further CORS Errors when pairing with the map clients.
Let's see if the new tests run through.

@TheGreatRefrigerator TheGreatRefrigerator marked this pull request as ready for review April 14, 2023 16:52
@takb takb moved this from To do to Review in ors general Apr 17, 2023
@takb takb self-requested a review April 17, 2023 08:36
Copy link
Contributor

@takb takb left a comment

Choose a reason for hiding this comment

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

LGTM. Checked locally, seems to work both in docker and run directly.

@MichaelsJP MichaelsJP merged commit 583ffb4 into master Apr 17, 2023
ors general automation moved this from Review to Awaiting release Apr 17, 2023
@MichaelsJP MichaelsJP deleted the feature/#994-adjustable-cors-settings branch April 17, 2023 11:18
@MichaelsJP MichaelsJP restored the feature/#994-adjustable-cors-settings branch April 17, 2023 13:09
@MichaelsJP MichaelsJP deleted the feature/#994-adjustable-cors-settings branch April 17, 2023 13:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
ors general
  
Awaiting release
Development

Successfully merging this pull request may close these issues.

CORS check for backend tomcat deployment Spring-boot cors configuration
3 participants