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

Add rack-cors and configuration #40

Merged
merged 2 commits into from
May 20, 2024
Merged

Add rack-cors and configuration #40

merged 2 commits into from
May 20, 2024

Conversation

matt-bernhardt
Copy link
Member

@matt-bernhardt matt-bernhardt commented May 17, 2024

This adds the rack-cors gem, with the recommended default config file,
to enable the application to receive communication from our various
search applications.

To confirm that this is solving the CORS problem, I've been connecting to the Rails console in the review app, doing searches in the related Bento PR build, and confirming that I can see them appearing via the console.

Developer

Tickets

  • No ENV changes are needed
  • No a11y testing is needed for this change
  • No stakeholder approval is needed yet

Requires database migrations?

NO

Includes new or updated dependencies?

YES

Code Reviewer

  • The commit message is clear and follows our guidelines
    (not just this pull request message)
  • There are appropriate tests covering any new functionality
  • The documentation has been updated or is unnecessary
  • The changes have been verified
  • New dependencies are appropriate or there were no changes

@mitlib mitlib temporarily deployed to tacos-api-pipeline-pr-40 May 17, 2024 16:16 Inactive
@JPrevost JPrevost self-assigned this May 17, 2024
** Why are these changes being introduced:

* The application needs to accept communications from other domains in
  order to do its job.

** Relevant ticket(s):

* https://mitlibraries.atlassian.net/browse/engx-266

** How does this address that need:

* This adds the rack-cors gem, with the recommended default config file,
  to enable the application to receive communication from our various
  search applications.

* A new required ENV is defined, to give control over which systems
  can connect via CORS. By default, no external connections are
  allowed.

** Document any side effects to this change:

* None
Copy link
Member

@JPrevost JPrevost left a comment

Choose a reason for hiding this comment

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

Looks good. I added a note to consider updating the docs to reference a possible local development confusion around port numbers that may not be immediately obvious for developers unfamiliar with CORS.

README.md Outdated
@@ -6,6 +6,8 @@

`LINKRESOLVER_BASEURL`: base url for our link resolver. `https://mit.primo.exlibrisgroup.com/discovery/openurl?institution=01MIT_INST&rfr_id=info:sid/mit.tacos.api&vid=01MIT_INST:MIT` is probably the best value unless you are doing something interesting.

`ORIGINS`: comma-separated list of domains allowed to connect to (and thus query or contribute to) the application. If not defined, no external connections will be permitted.
Copy link
Member

Choose a reason for hiding this comment

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

Maybe add a note that non-standard ports also need to be included (i.e. localhost will not work when running on localhost:5000, etc)

Update env description based on code review feedback
@mitlib mitlib temporarily deployed to tacos-api-pipeline-pr-40 May 20, 2024 19:49 Inactive
@matt-bernhardt
Copy link
Member Author

Good call - I had to work out the port number thing myself, so that's probably good to have in the readme. I've pushed a commit to update the description there.

@matt-bernhardt matt-bernhardt merged commit d977979 into main May 20, 2024
2 checks passed
@matt-bernhardt matt-bernhardt deleted the engx-266-cors branch May 20, 2024 19:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants