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

feat: Add CORS capability to HTTP API #467

Merged
merged 3 commits into from
Jun 8, 2022

Conversation

fredcarle
Copy link
Collaborator

RELEVANT ISSUE(S)

Resolves #408

DESCRIPTION

This PR adds CORS capability to the HTTP API. In situations where the database will be accessed from a different domain than that of the Defra host, the HTTP API needs to communicate to the browser the allowed origins and handle preflight requests.

HOW HAS THIS BEEN TESTED?

Unit tests and curl with allowedOrigins set to http://test.com

# this will result in an unsuccessful Preflight request
curl -i http://localhost:9181/api/v1/ping -H "Origin: http://no.com" -H "Access-Control-Request-Method: GET" -X OPTIONS

# this will result in an successful Preflight request
curl -i http://localhost:9181/api/v1/ping -H "Origin: http://test.com" -H "Access-Control-Request-Method: GET" -X OPTIONS

CHECKLIST:

  • I have commented the code, particularly in hard-to-understand areas.
  • I have made corresponding changes to the repo-held documentation.
  • I have made sure that the PR title adheres to the conventional commit style (subset of the ones we use can be found under: tools/configs/chglog/config.yml

ENVIRONMENT / OS THIS WAS TESTED ON?

Please specify which of the following was this tested on (remove or add your own):

  • Arch Linux
  • Debian Linux
  • MacOS
  • Windows

@fredcarle fredcarle added feature New feature or request area/api Related to the external API component action/no-benchmark Skips the action that runs the benchmark. labels May 24, 2022
@fredcarle fredcarle requested a review from a team May 24, 2022 04:24
api/http/server.go Outdated Show resolved Hide resolved
@fredcarle fredcarle changed the title Feat: Add CORS capability to HTTP API feat: Add CORS capability to HTTP API May 24, 2022
@fredcarle fredcarle added this to the DefraDB v0.3 milestone May 24, 2022
Copy link
Contributor

@AndrewSisley AndrewSisley left a comment

Choose a reason for hiding this comment

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

Approved with a couple of non-blocking comments (mostly on your comments/questions) :)

api/http/handler.go Outdated Show resolved Hide resolved
api/http/server.go Outdated Show resolved Hide resolved
api/http/handler.go Outdated Show resolved Hide resolved
@AndrewSisley
Copy link
Contributor

Re-approving via comment after the latest changes. Looks good :)

@fredcarle fredcarle force-pushed the fredcarle/feat/I408-http-api-cors branch from 0c7a7ef to 945730e Compare May 24, 2022 19:31
@orpheuslummis
Copy link
Contributor

After #389 merges (if that is the case), I suggest that the CORS configuration should then be part of top-level config package.

@jsimnz
Copy link
Member

jsimnz commented May 25, 2022

After #389 merges (if that is the case), I suggest that the CORS configuration should then be part of top-level config package.

Yupp, was going to mention this. We should find a way to track PRs that need to make changes to the config stuff (once its merged)

@fredcarle fredcarle force-pushed the fredcarle/feat/I408-http-api-cors branch from 945730e to 6862080 Compare June 2, 2022 21:07
Copy link
Member

@shahzadlone shahzadlone left a comment

Choose a reason for hiding this comment

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

Did you get a chance to test this with Apolo Studio? I tried (maybe I am doing something wrong) and it gives me a 404, or asks me Are CORS headers present?.

@fredcarle
Copy link
Collaborator Author

Did you get a chance to test this with Apolo Studio? I tried (maybe I am doing something wrong) and it gives me a 404, or asks me Are CORS headers present?.

Could you give me the details of your request please? (Method, URL, Body, Headers). Apollo is giving me some problems with local requests so I'll have to try something different to test it out.

@fredcarle fredcarle force-pushed the fredcarle/feat/I408-http-api-cors branch from 6862080 to d38df73 Compare June 6, 2022 21:26
@shahzadlone
Copy link
Member

Could you give me the details of your request please? (Method, URL, Body, Headers). Apollo is giving me some problems with local requests so I'll have to try something different to test it out.

So, I just go to https://www.apollographql.com and login and try to get it set up with defradb. Idk what Method, URL, Body and Headers it's request contains.

@fredcarle
Copy link
Collaborator Author

So, I just go to https://www.apollographql.com and login and try to get it set up with defradb. Idk what Method, URL, Body and Headers it's request contains.

I made it work. Because the config PR hasn't been merged, there is still no way to set the allowedOrigins from the cli. You have to set it up manually in the code. Go to cli/defradb/cmd/start.go at line 184 and http.WithAllowedOrigins:

s := http.NewServer(db, http.WithAddress(config.Database.Address), http.WithAllowedOrigins("https://studio.apollographql.com"))

@shahzadlone
Copy link
Member

So, I just go to https://www.apollographql.com and login and try to get it set up with defradb. Idk what Method, URL, Body and Headers it's request contains.

I made it work. Because the config PR hasn't been merged, there is still no way to set the allowedOrigins from the cli. You have to set it up manually in the code. Go to cli/defradb/cmd/start.go at line 184 and http.WithAllowedOrigins:

s := http.NewServer(db, http.WithAddress(config.Database.Address), http.WithAllowedOrigins("https://studio.apollographql.com"))

Awesome. Cheers!

api/http/router.go Show resolved Hide resolved
api/http/server_test.go Show resolved Hide resolved
api/http/router.go Show resolved Hide resolved
tweek strucs to have options field instead of embedded serverOption struct

change CORS to only apply if allowedOrigins is explicitely set
@codecov
Copy link

codecov bot commented Jun 8, 2022

Codecov Report

Merging #467 (31a5179) into develop (707c124) will increase coverage by 0.09%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #467      +/-   ##
===========================================
+ Coverage    52.46%   52.55%   +0.09%     
===========================================
  Files           97       97              
  Lines        13144    13170      +26     
===========================================
+ Hits          6896     6922      +26     
  Misses        5574     5574              
  Partials       674      674              
Impacted Files Coverage Δ
api/http/handler.go 100.00% <100.00%> (ø)
api/http/router.go 100.00% <100.00%> (ø)
api/http/server.go 100.00% <100.00%> (ø)

@fredcarle fredcarle merged commit d200cb2 into develop Jun 8, 2022
@fredcarle fredcarle deleted the fredcarle/feat/I408-http-api-cors branch June 8, 2022 13:09
shahzadlone pushed a commit to shahzadlone/defradb that referenced this pull request Feb 23, 2024
RELEVANT ISSUE(S)
Resolves sourcenetwork#408

DESCRIPTION
This PR adds CORS capability to the HTTP API. In situations where the database will be accessed from a different domain than that of the Defra host, the HTTP API needs to communicate to the browser the allowed origins and handle preflight requests.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
action/no-benchmark Skips the action that runs the benchmark. area/api Related to the external API component feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add CORS support to the HTTP GraphQL API endpoint
5 participants