petterreinholdtsen / noark5-tester

Test Noark 5 Core REST API
3 stars 3 forks source link

Allow header expected in responses to GET requests when RFC7231 has no such requirement #27

Open ivaylomitrev opened 1 year ago

ivaylomitrev commented 1 year ago

It seems that tester tool expects the "Allow" header to be present in responses to GET requests when RFC 7231#7.4.1 states:

An origin server MUST generate an Allow field in a 405 (Method Not Allowed) response and MAY do so in any other response.

The particular test is encountered in runtest#L375 as of the current version of the tool:

self.verify(allow is not None, "GET header should include Allow for %s" % url)

Is there another reason for the test that is outside of the known requirements for the "Allow" header?

petterreinholdtsen commented 1 year ago

[ivaylomitrev]

Is there another reason for the test that is outside of the known requirements for the "Allow" header?

I do not remember where this originated from. I see from the git history the test was added to runtest in 2017, and commits around the same time were added tests for OPTIONS related stuff, so I suspect it is related to CORS and OPTIONS. Will try to read up on the topic and get back to you when I checked the spesification and references a bit more. It is a long time since I read the CORS spesification. :)

When that is said, my understanding of the Allow header is that it provide instructions for the clients about the operations that work for a given URL, which seem useful for a REST server to provide.

-- Happy hacking Petter Reinholdtsen

ivaylomitrev commented 1 year ago

Thanks!

I did glance at the CORS specification and it has no reference to the Allow header from what I could see.

And, judging by RFC7231, the only place where Allow is required is 405 Method Not Allowed responses. It is only recommended to be provided in responses to OPTIONS. There is no mention of it with regard to GET requests.

tsodring commented 1 year ago

Looking at this again, I agree the ALLOWS header is a little strange. The ALLOWS header was likely introduced from a discussion on Access-Control-Allow-Headers from CORS.

https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Access-Control-Allow-Methods

_The Access-Control-Allow-Methods response header specifies one or more methods allowed when accessing a resource in response to a preflight request._

An example of the header is shown below:

OPTIONS https://n5.example.com/api/arkivstruktur/mappe/7f000101-8987-1467-8189-878cf131018a
Access-Control-Request-Method: GET, PATCH, PUT, DELETE
Access-Control-Request-Headers: origin, x-requested-with

The intention is that the client knows what it can do with the object. My understanding is that the client could use this e.g., in a user interface to toggle various user interface elements. In the above example the client can retrieve, update and delete the object, and the user interface can automatically switch on buttons to show this to the user.

So in many ways it is about exposing authorization. Somewhere the actual meaning of this seems to have gotten lost. The point, I believe, was to use the ALLOWS header to explicitly expose what the client/user is authorized to do with the object, without baking it in / hiding it in CORS.

The question then is what we should do about it. I think it should be stated explicitly in the API specification. Currently, the publication of authorization details are left to inspecting the Access-Control-Allow-Methods header.

I agree that it does not make sense to have ALLOWS everywhere. I think we should bring the issue up in the next API spec meeting and decide if we explicitly use ALLOWS header for this or not. I don't think it is a good idea to "steal" the ALLOWS header for the API specification. If it is not clear that the client should know what it can do with an object from the CORS header, then we should consider adding a header.

petterreinholdtsen commented 3 months ago

The handling of OPTIONS and Allow is runtest has been adjusted today. Is it now closer to your understanding on how this should work?

-- Happy ahcking Petter Reinholdtsen