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: Consistently use CORS (Cross-Origin Resource Sharing) headers such a "Access-Control-Allow-Origin" #1136

Closed
pdurbin opened this issue Nov 17, 2014 · 15 comments

Comments

@pdurbin
Copy link
Member

pdurbin commented Nov 17, 2014

An API tester noticed that a couple API endpoints (Access and Meta, per below) add the "Access-Control-Allow-Origin" CORS (Cross-Origin Resource Sharing) header but most do not.

You can see the header like this: curl -v https://apitest.dataverse.org/api/access/datafile/10

In this ticket let's decide if we should be returning these headers in more places. From a comment, it seems like the header was added to support TwoRavens.

See also http://en.wikipedia.org/wiki/Cross-origin_resource_sharing

Here's where we add this header as of c4e786d

murphy:dataverse pdurbin$ ack 'Access-Control'
src/main/java/edu/harvard/iq/dataverse/api/Access.java
137:        /* Provide "Access-Control-Allow-Origin" header:
143:        response.setHeader("Access-Control-Allow-Origin", "*");

src/main/java/edu/harvard/iq/dataverse/api/Meta.java
96:        response.setHeader("Access-Control-Allow-Origin", "*");
133:        response.setHeader("Access-Control-Allow-Origin", "*");
168:        response.setHeader("Access-Control-Allow-Origin", "*");
@pdurbin pdurbin added this to the In Review - Dataverse 4.0 milestone Nov 17, 2014
@scolapasta scolapasta modified the milestones: 4.01, In Review - Dataverse 4.0 Jan 6, 2015
@eaquigley eaquigley changed the title API: Consitently use CORS (Cross-Origin Resource Sharing) headers such a "Access-Control-Allow-Origin" API: Consistently use CORS (Cross-Origin Resource Sharing) headers such a "Access-Control-Allow-Origin" Aug 26, 2015
@scolapasta scolapasta removed this from the Not Assigned to a Release milestone Jan 28, 2016
@landreev
Copy link
Contributor

landreev commented May 2, 2016

This was brought up again, via RT:

https://help.hmdc.harvard.edu/Ticket/Display.html?id=235395

(below is my note on this from the dataverse list)

I really feel [this issue] should be bumped [up] in priority.

For these people, I just made a quick patch - built a version of Search.class that adds Access-Control-Allow-Origin=* to the response.
A proper solution would probably be to do this in the AbstractApiBean; but maybe make it configurable, with a setting?
I can maybe see how some installation owners may not want everybody in the world to create web forms querying their APIs? (though it can be argued that this is not an issue - since everybody in the world can already bombard their APIs with scripted curl calls... - ?)
Perhaps it should be possible to include specific domains in the header via settings.

landreev added a commit that referenced this issue May 11, 2016
@landreev
Copy link
Contributor

This was addressed in the 3089-jsoup branch, as a quick fix - a patch applied to just the Search.java (search api); per a somewhat urgent request, RT 235512.The patch:
30d29
< import javax.servlet.http.HttpServletResponse;
34d32
< import javax.ws.rs.core.Context;
70,71c68
< @QueryParam("show_my_data") boolean showMyData,

< @context HttpServletResponse response

        @QueryParam("show_my_data") boolean showMyData

188d184
< response.setHeader("Access-Control-Allow-Origin", "*");

this dev. ticket should stay open - to resolve this issue more generally.

@michbarsinai
Copy link
Member

The only candidates to consider for CORS are read-only endpoints (so web pages can't change things in Dataverse, only read them).
I think we should enable CORS for each GET endpoint in Datasets, Dataverses, Search, and MetadataBlocks.

Not sure about WorldMapRelatedData... @raprasad, what do you say?

@raprasad
Copy link
Contributor

raprasad commented Jan 3, 2017

@michbarsinai : Agreed re: cors. In the near-future, the worldmap metadata should become part of the Datafile data. e.g. the core DataFile information + sections/JSON blocks of file-specific metadata for FITS, WorldMap, tabular data, etc., etc.

  • The current WorldMap endpoints, even read-only (the GETs), are task-specific and not too useful out of that context.

@raprasad
Copy link
Contributor

raprasad commented Jan 3, 2017

@pdurbin @michbarsinai - Let me know if the Dataverse "metrics" CORS are consistent with your implementation.

(1) Read-only endpoints for Metrics:

(2) Current CORS header:

def send_cors_response(response):
    """Quick hack to allow CORS...."""

    response["Access-Control-Allow-Origin"] = "*"

    return response

@raprasad
Copy link
Contributor

raprasad commented Jan 3, 2017

Seems consistent to me, but this API being for internal use at this point - do we really want to encourage Javascript clients for it?

May have confused things by introducing a new item (Metrics API):

  • WorldMap endpoints
    • Part of core Dataverse code
    • Internal use only
  • Metrics API
    • Not part of core Dataverse code
    • Open to the public
      • Not widely advertised

@michbarsinai
Copy link
Member

michbarsinai commented Jan 3, 2017 via email

@scolapasta scolapasta removed their assignment Jan 3, 2017
@kcondon kcondon closed this as completed Jan 13, 2017
@kcondon kcondon reopened this Jan 13, 2017
@kcondon
Copy link
Contributor

kcondon commented Jan 13, 2017

Michael wrote:
I think we should enable CORS for each GET endpoint in Datasets, Dataverses, Search, and MetadataBlocks.

Dataverses:
GET http://$SERVER/api/dataverses/$id
GET http://$SERVER/api/dataverses/$id/contents
x GET http://$SERVER/api/dataverses/$id/roles?key=$apiKey
GET http://$SERVER/api/dataverses/$id/facets?key=$apiKey
x GET http://$SERVER/api/dataverses/$id/assignments?key=$apiKey
GET http://$SERVER/api/dataverses/$id/metadatablocks?key=$apiKey
x GET http://$SERVER/api/dataverses/$id/metadatablocks/isRoot?key=$apiKey

Datasets:
GET http://$SERVER/api/datasets/$id?key=$apiKey
GET http://$SERVER/api/datasets/$id/versions?key=$apiKey
GET http://$SERVER/api/datasets/$id/versions/$versionNumber?key=$apiKey
GET http://$SERVER/api/datasets/export?exporter=ddi&persistentId=$persistentId
GET http://$SERVER/api/datasets/$id/versions/$versionId/files?key=$apiKey
GET http://$SERVER/api/datasets/$id/versions/$versionId/metadata?key=$apiKey
GET http://$SERVER/api/datasets/$id/versions/$versionId/metadata/$blockname?key=$apiKey
x GET http://$SERVER/api/datasets/$id/assignments?key=$apiKey
x GET http://$SERVER/api/datasets/$id/privateUrl?key=$apiKey
(also try publish with POST and GET)
POST http://$SERVER/api/datasets/$id/actions/:publish?type=$type&key=$apiKey

Metadatablocks:
GET http://$SERVER/api/metadatablocks
GET http://$SERVER/api/metadatablocks/$identifier

Search:
curl -v -X GET "http://localhost:8080/api/search?q=trees&key=$apiKey"
curl -v -X GET "http://localhost:8080/api/search?q=finch&show_relevance=true&show_facets=true&fq=publicationDate:2016&subtree=root&key=$apiKey"

Shibboleth Groups (this had the green CORS tag):
curl -v http://localhost:8080/api/info/version

@kcondon
Copy link
Contributor

kcondon commented Jan 13, 2017

@michbarsinai @pdurbin
I've tested the api GET calls per Michael's suggestion in a previous comment and found a few, marked with x above, that did not have "Access-Control-Allow-Origin: *"

Thoughts?

pdurbin added a commit that referenced this issue Jan 17, 2017
@pdurbin
Copy link
Member Author

pdurbin commented Jan 17, 2017

@kcondon As of 44f4936 I'm seeing Access-Control-Allow-Origin: * in the output for both of the Search API examples marked with an X above. I did find a small typo, which I fixed in fdb7035.

The other thing that strikes me is that that I assume we should be using the green "CORS" labels that @michbarsinai added to the API Guide as the source of truth of which endpoints support CORS. I'll include a screenshot below.

I don't want to step on @michbarsinai 's toes so I'm going to unassign myself but if he's busy or wants me to work on this pull request, please let me know.

screen shot 2017-01-17 at 9 30 52 am

@pdurbin pdurbin removed their assignment Jan 17, 2017
@michbarsinai
Copy link
Member

Update on my initial list:
Dataverses: Endpoints marked with "x" do not make sense for a browser plug in, I thin they could be left as-is.
Datasets: Same as above
Search: Good catch, I'll add this.

@kcondon
Copy link
Contributor

kcondon commented Jan 17, 2017

@michbarsinai Turns out Search was implementing CORS so false alarm.

@kcondon
Copy link
Contributor

kcondon commented Jan 17, 2017

@michbarsinai @pdurbin Regarding the green CORS labels as the source of truth, that's fine by me but I did not see mention of it when I was testing. Plus, they are not on all CORS-enabled endpoints, which I think is what Phil was getting at but not sure. An example: curl -v "http://localhost:8080/api/dataverses/1/facets?key=" I think that is the only missing one.

@michbarsinai It looks like the only remaining issue is adding CORS tag to the doc for
GET http://$SERVER/api/dataverses/$id/facets?key=$apiKey

@kcondon
Copy link
Contributor

kcondon commented Jan 17, 2017

Phil added the remaining CORS tag, merged, closed.

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

No branches or pull requests

9 participants