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

CORS support #501

Closed
fgalan opened this Issue Aug 7, 2014 · 21 comments

Comments

Projects
None yet
7 participants
@fgalan
Copy link
Member

fgalan commented Aug 7, 2014

It would be interesting to have a CLI switch (e.g. -cors) so Orion CB behaves in a "CORS friendly" mode, i.e. including the Access-Request-XXXX headers in the HTTP responses.

@paramarco

This comment has been minimized.

Copy link

paramarco commented May 18, 2015

+1

@fgalan

This comment has been minimized.

Copy link
Member

fgalan commented May 19, 2015

CORS support for GET requests implemented in PR #846. CORS support for PUT/POST request requires preflight requests through the OPTIONS method and it is deferred to a next PR.

@fgalan

This comment has been minimized.

@drasko

This comment has been minimized.

Copy link

drasko commented Mar 14, 2016

+1

Freeboard (https://www.fiware.org/2015/07/13/fiware-orion-data-source-now-available-for-freeboard/) will not work without it on localhost, unless proxied.

@cdupont

This comment has been minimized.

Copy link

cdupont commented Jan 31, 2017

Hi there!
What is the status of implementing OPTIONS requests in Orion?

@fgalan

This comment has been minimized.

Copy link
Member

fgalan commented Jan 31, 2017

What is the status of implementing OPTIONS requests in Orion?

The status is "not implemented". If you want to support the implemenation of CORS features in Orion, please use a +1 reaction on this issue.

Thanks!

@cdupont

This comment has been minimized.

Copy link

cdupont commented Jan 31, 2017

+1

@reitsma

This comment has been minimized.

Copy link

reitsma commented Feb 9, 2017

+1
I am using Angular JS running inside a Chrome browser with Orion 1.6.0. The preflight is triggered by the use of the Fiware-specific HTTP headers Fiware-Service and Fiware-ServicePath. I would expect CORS support for preflights because of exactly this reason: Your own product is requiring it!

@cdupont

This comment has been minimized.

Copy link

cdupont commented Feb 9, 2017

In the mean-time, here is a work around: putting Orion behind a proxy.
The proxy should reply with the correct headers and respond to pre-flight.
Here is our HTTPD configuration:
https://github.com/Waziup/Platform/blob/master/identity/identityproxy.conf

<Location /v2>
   # forward requests to Orion
   ProxyPass "http://orion:1026/v2"
   ProxyPassReverse "http://orion:1026/v2"

   # CORS headers including Fiware-ServicePath
   Header always set Access-Control-Allow-Origin "*"
   Header always set Access-Control-Allow-Methods "POST, GET, OPTIONS"
   Header always set Access-Control-Allow-Headers "Origin, X-Requested-With, Content-Type, Accept, Authorization, Fiware-ServicePath, Fiware-Service"

   # Reply to OPTIONS request with empty message (only headers)
   RewriteEngine On 
   RewriteCond %{REQUEST_METHOD} OPTIONS 
   RewriteRule ^(.*)$ $1 [R=200,L]
</Location>
@McMutton

This comment has been minimized.

Copy link
Collaborator

McMutton commented Sep 1, 2017

I am working on this issue, can you please assign it to me? Assignment already done on Jira.

@fgalan

This comment has been minimized.

Copy link
Member

fgalan commented Sep 1, 2017

@McMutton is assigned to this issue

@McMutton

This comment has been minimized.

Copy link
Collaborator

McMutton commented Sep 19, 2017

I have been reading the documentation and experimenting with Orion's service routines and I need some guidance before diving into coding.

So, on CORS friendly mode, if we receive an OPTIONS request, we need to have the headers below in the response:

Header Comment
Access-Control-Allow-Origin This has already been implemented in #846 and almost no further action is required.
Access-Control-Allow-Headers This can be handled the same way in #846 assuming we have a static set of allowed headers.
Access-Control-Max-Age Same as above.
Access-Control-Allow-Methods This is where things get a bit messy. In contextBroker.cpp we have the service vector definitions and for each of them we have a service routine.

For example, this is what I did to handle the Access-Control-Allow-Methods headers for the /v2/entities resource. I added the OPTIONS entry below and implemented the optionsGetPostOnly service routine which pushes the header with value: GET, POST, OPTIONS to the response.

  { "GET",     ENT,          ENT_COMPS_V2,         ENT_COMPS_WORD,          getEntities              }, \
  { "POST",    ENT,          ENT_COMPS_V2,         ENT_COMPS_WORD,          postEntities             }, \
  { "OPTIONS", ENT,          ENT_COMPS_V2,         ENT_COMPS_WORD,          optionsGetPostOnly       }, \
  { "*",       ENT,          ENT_COMPS_V2,         ENT_COMPS_WORD,          badVerbGetPostOnly       }, \

But this means, we need to do the same for ALL the resources. I don’t see any easier way to do this since allowed methods vary for each resource.

So my question is, do you think this is feasible or do we have some other mechanism in Orion which I am not aware of, that would do the same job without touching too many lines of code?

@fgalan

This comment has been minimized.

Copy link
Member

fgalan commented Sep 19, 2017

The restServiceV[] vector defined in contextBroker.cpp (splited in several part using #defines) is the basic routing structure for HTTP requests, as you probably guess. It associates each combination of verb + URL pattern to a given handling function.

Thus, I think you would need to add an "OPTION" entry for each resource to which processing OPTION makes sense (all of them? I'm not sure, I'm not an expert in CORS ;). You are right there are many resources, but adding entries should be quite straighforward.

What I'm not so sure if this each new "OPTION" entry involves creating a new handling function. Maybe you could factorize behaviour in one or a small set of handling functions. Actually, this strategy is used in the current code, look how badVerbPostOnly handling function is used in many entries.

Regarding .test, covering the OPTION case for all the resource could be overhelming (althoug if you want to be exahustive, no problem with it :). My suggestion is to cover a "sample" of them, large enough to be representative, but not too large.

Hoping this feedback helps. Maybe @kzangeli could also give some implementation hint, as he is more familiar with the routing logic than me in the code.

@McMutton

This comment has been minimized.

Copy link
Collaborator

McMutton commented Sep 19, 2017

Thanks for your comment @fgalan.

Yes, that was my thought, a small set of handling functions would be enough as in badVerb... handlers. My concern was with the new service vector definitions which seems to be inevitable :)

And about the tests, I will probably post here again asking about the details of the coverage soon :)

@McMutton

This comment has been minimized.

Copy link
Collaborator

McMutton commented Oct 2, 2017

I have mostly completed the handling of OPTIONS request to the V2 API and the commit (in my fork) is really huge at this point: 21 changed files with 986 additions and 49 deletions. Please note that this does not include the unit and functional tests, so the final commit will include even bigger number of file and lines of code changes.

Do you think is it feasible to review a huge PR like this? Should I divide the work between a few commits to have the PR more organized? Any suggestions?

@fgalan

This comment has been minimized.

Copy link
Member

fgalan commented Oct 2, 2017

Yes, it's pretty big...

Maybe spliting the work in several PRs, each one covering a small number of V2 API operations e2e (i.e. including the test for such samall number of operations)?

@McMutton

This comment has been minimized.

Copy link
Collaborator

McMutton commented Oct 2, 2017

Ok sure, I will divide the work in several PRs.

Now that you mention it for only V2 API operations, I have to ask, are we going to enable CORS for V1 API operations as well?

@fgalan

This comment has been minimized.

Copy link
Member

fgalan commented Oct 2, 2017

Now that you mention it for only V2 API operations, I have to ask, are we going to enable CORS for V1 API operations as well?

Ideally, it should cover both V2 and V1 API. Let's focus in V2 by the moment. Covering also V1 would depend on how much effort you can put on this ;)

@fgalan fgalan added this to the 1.10.0 milestone Oct 19, 2017

@fgalan

This comment has been minimized.

Copy link
Member

fgalan commented Oct 20, 2017

CORS preflight for /v2 and /v2/entities implemented in PR #3018

@fgalan

This comment has been minimized.

Copy link
Member

fgalan commented Nov 6, 2017

Implemented for all the rest of NGSIv2 URLs in PR #3032

@fgalan

This comment has been minimized.

Copy link
Member

fgalan commented Nov 15, 2017

Some spin-off issue is pending (in particular, #3030) but I think we can consider CORS basic support completed and this issue (once that SOF questions has been edited) can be closed.

At the end I think it doesn't worth the time to implement CORS for NGSIv1. NGSIv1 is fine for legacy integrations (although it would be deprecated sooner or later) but for new functionality (including CORS) let's focus on NGSIv2.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment