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

Update OPTIONS, Allow, Accept-* #931

Closed
csarven opened this issue Nov 13, 2018 · 21 comments
Closed

Update OPTIONS, Allow, Accept-* #931

csarven opened this issue Nov 13, 2018 · 21 comments

Comments

@csarven
Copy link
Member

csarven commented Nov 13, 2018

re:

curl -iX OPTIONS https://csarven.solid.community/

Vary: Accept, Authorization, Origin
Access-Control-Allow-Methods: OPTIONS,HEAD,GET,PATCH,POST,PUT,DELETE
Allow: GET,HEAD
Accept-Patch: application/sparql-update
  • Allow should include what's really "allowed" eg. OPTIONS is a good start.
  • If Accept-Patch is of any indication, add PATCH to Allow.
  • If PUT, POST are also allowed add to Allow, and would be good to include Accept-Put and Accept-Post respectively (note: Accept-Put is not really spec'd/standardised but just completeness sake)

Just to be clear, that's for a public resource. I understand that GET, HEAD, and OPTIONS may be the only methods allowed, then Accept-Patch probably shouldn't be advertised. No major harm but still. Note different treatment to https://csarven.solid.community/public/ , which appears to be correct (authenticated).

Aside: There may have been a special case for the storage root (I recall some people having issues with overwriting their homepage or whatever), so perhaps that's why PATCH is not listed there now (even if authenticated if I'm not mistaken). I don't see why PATCH would be allowed but not PUT or POST, and others on storage root, but ok.

@kidehen
Copy link

kidehen commented Nov 13, 2018

@RubenVerborgh ,

Your thoughts on this matter? Fundamentally, we need to sort this out so that the following dokie.li features are demonstrable against a Solid Pod:

  1. Reviews Storage
  2. Activities Viewing

/cc @csarven

@kjetilk kjetilk added the triage Issues that need team review label Nov 13, 2018
@RubenVerborgh
Copy link
Contributor

I'm trying to gauge the urgency of this issue. I get the eventual necessity, but what is broken right now that will be fixed when we do this?

There are more urgent blockers for dokieli, notably #661 (which is currently scheduled with a higher priority).

We have a limited team of developers, an internal deadline of November for v5.0, and more issues than this team can tackle by that date. So it's a matter of the right prioritization.

@dmitrizagidulin
Copy link
Contributor

@csarven Is this functionality not covered by the WAC-Allow header?

@kidehen
Copy link

kidehen commented Nov 13, 2018

@RubenVerborgh,

What's broken right now is the basis of this issue. To reiterate, the following dokie.li functionality is unusable with Solid Pod deployed via the current Node Solid Server (NSS):

  1. Reviews Storage -- reviews are stored in a place chosen by the end-user
  2. Activity Tracking on a Document Reviewed by various folks identified by their respective WebIDs -- transclusion of reviews across end-users for a single document.

Fixing this problem makes the functionality in question usable and demonstrable i.e., powerful end-user app for demonstrating virtues of loosely-coupling:

  1. Identity
  2. Identification
  3. Authentication
  4. Authorization
  5. Storage

Fundamentally, this is yet another killer-demo regarding how Solid enables a Read-Write Web without implicit Data Silos and end-user privacy compromise :)

/cc @csarven

@RubenVerborgh
Copy link
Contributor

But is #931 the reason why 1 and 2 are broken? I think the reason is #661.

@kidehen
Copy link

kidehen commented Nov 14, 2018

@RubenVerborgh,

The issue is simply a case of dokie.li being able to discern the fact that RDF-Turtle is preferred by the Solid Pod deployed via NSS.

If the correct metadata is returned via HTTP response headers dokie.li can make accurate deductions.

I hope this clarifies the issue at hand so that we can move forward on this matter. Right now, we have a quite artificial hold-up that is blocking demonstration of the complete repetoire of capabilities that dokie.li brings to the table.

@RubenVerborgh
Copy link
Contributor

The issue is simply a case of dokie.li being able to discern the fact that RDF-Turtle is preferred by the Solid Pod deployed via NSS.

I don't think that's the issue: NSS does not have such a preference.

The problem is rather that NSS ignores Content-Type headers of request bodies (#925, #413) and instead relies on the file extension. If that file extension is missing, it (incorrectly) assumes Turtle. This is why we have #643, #661, #662. As of today, @rubensworks is working on #661, which makes #662 trivial and will fix this problem once and for all.

If the correct metadata is returned via HTTP response headers dokie.li can make accurate deductions.

The correct metadata is that we accept everything, perhaps with a preference for Turtle. But dokieli has good reasons to prefer HTML/RDFa, and we support that (pending the content-type fixes).

we have a quite artificial hold-up

I love the "just works" experience as much as you do, and dokieli is one of the greatest demonstrators of decentralized Linked Data on the Web. But it's important that we fix things the right way. Shortcuts have been taken in the past, resulting in the difficult-to-maintain NSS codebase we currently have. Implementing the above headers would make things work in the short term, but they are not the right solution in the long term. Now that we have several "just works" experiences, it's important to shift focus to a long-term "keeps on working" experience IMHO.

@kjetilk
Copy link
Member

kjetilk commented Nov 14, 2018

OK, I don't quite understand what dokie.li needs, but the current situation is this: #661 is being worked on as we speak, and with that in place #662 can be fixed shortly thereafter. That fixes what I understand is a significant holdup for dokie.li. So, the question is if this would fix the two problems cited above. From the surface, it seems like it could, since it addresses how Turtle is treated.

If so, that's great. If not, then yes, we are could bump the priority of this issue. It is not certain that we would get to it, as the deadline for prioritization is today and we only have one week to go, but it would be very helpful to know if #661 and #662 are likely to fix the issue.

Nevertheless, I'm labelling it for inclusion in the development queue.

@kjetilk kjetilk added inrupt-planning and removed triage Issues that need team review labels Nov 14, 2018
@csarven
Copy link
Member Author

csarven commented Nov 14, 2018

Hang on folks. There sems to be some miscommunication here. Allow me try to clarify :)

The original issue is not related to 661/662, and it is not specific to dokieli either. It is something that NSS should do properly for OPTIONS requests - unless there are some undocumented heuristics in place for special treatment of /.

dokieli is first checking what the server is capable of and prefers through OPTIONS, if relevant (eg. Accept-Post and one of the mimetypes dokieli can handle) information is not found, it proceeds to do its LDN and WAP (which are JSON-LD based) out of the box. dokieli is fully capable of sending the payload in Turtle, JSON-LD, and HTML+RDFa. It just wants to know which methods it allows and mimetypes it prefers. At the moment it is looking out for information relevant for POST because the interactions around LDN and WAP use it. But, if POST is not possible to that target, that's okay too. The client should be able to see if PUT or PATCH are allowed for instance. At the moment, that doesn't appear to be the case either - Accept-Patch sort of hints that is possible what's intended but, to be specific, it only says what it prefers for PATCH. dokieli can send a PATCH too, so I suggest to at least include that in the Allow for starters.

So:

  1. If agent is authenticated, shouldn't OPTIONS return something like Allow: OPTIONS, HEAD, GET, PATCH, POST, PUT, DELETE at /?
  2. If Accept-Post or maybe even Accept-Put is possible, they should be included. We can put Accept-Put aside for now even as no one is really using that - although dokieli is prepared to do that - and will do (minor, unrelated to the core of the issue)

I understand that there may be good reasons for why storage / returns Allow: GET, HEAD, Accept-Patch: application/sparql-update, but apparently fine for another container eg /public/. That's part of the investigation.

Note that, dokieli happens to not find what it is looking for at / and ends up falling back to JSON-LD POST - I actually think dokieli shouldn't follow through in this case - and NSS seems to be okay with it (when the dokieli browser extension is used, and sending to pim:storage location), 201 Created, and 200 on the Location.

Did I add more confusion or make it clear? Let's put dokieli aside for now and doublecheck what the server should be doing.

Aside: yes, 661/662 are still quite important for dokieli but that is a different matter eg. 'Save'/'Save As' operations for HTML+RDFa articles use PUT (and the user names the target... and we don't want ugly .html). So, LDN/WAP expect the server to operate with JSON-LD, so this HTML stuff between dokieli and NSS is not relevant. dokieli has been doing fine with whatever non-HTML+RDFa NSS prefers through OPTIONS.

@kjetilk
Copy link
Member

kjetilk commented Nov 14, 2018

Right, so yeah, I figured these issues were rather separate. Just trying to find what to prioritize.

Anyway, on the topic of Allow, I have never fully understood what solid tries to achieve with it. It seems to me it is a adding it rather indiscriminately, i.e. not what you can do with a resource, but what you could do if everything was just right, authn and authz and all that... :-)

This looks to me to create a situation where you can't really rely on it, and quite a lot of logic around it would be needed to improve that. Is that part of the problem, @csarven , is it orthogonal to it, or am I just wrong in how Solid works?

@csarven
Copy link
Member Author

csarven commented Nov 14, 2018

Identify and resolve the diff between / and eg /public/.

Needless to say, the server has to be consistently accurate, otherwise, clients are going to be built arbitrarily (out of "spec") because the server has arbitrary behaviour - at least in this particular case.

I have no comment or direct influence on prioritization of things. Just raising issues/QA stuff as usual that appears to require closer inspection or further development. IMO, the set of interactions that kickoff from OPTIONS are vital for improving interop because 1) there are existing standards that clients need to pick through, conform to, and concert with capable servers, and 2) closing gaps on what's unspecified with reliable fallbacks.

@RubenVerborgh
Copy link
Contributor

RubenVerborgh commented Nov 14, 2018

  • If agent is authenticated, shouldn't OPTIONS return something like Allow: OPTIONS, HEAD, GET, PATCH, POST, PUT, DELETE at /?

Yes. #934

  • If Accept-Post or maybe even Accept-Put is possible, they should be included.

They are just * though; or perhaps with a preference for Turtle if that is possible. #935

Let's follow up in the above two separate issues.

@kidehen
Copy link

kidehen commented Nov 14, 2018

@RubenVerborgh ,

I love the "just works" experience as much as you do, and dokieli is one of the greatest demonstrators of decentralized Linked Data on the Web. But it's important that we fix things the right way. Shortcuts have been taken in the past, resulting in the difficult-to-maintain NSS codebase we currently have. Implementing the above headers would make things work in the short term, but they are not the right solution in the long term. Now that we have several "just works" experiences, it's important to shift focus to a long-term "keeps on working" experience IMHO.

But you are kinda misunderstanding my intentions and how I work.

Please note:

  • I am not a fan of shortcut hacks.
  • I try my best to get conflicts resolved between parties especially problems that are riddle-like in nature.

I pushed this thread because I see and sense unecessary inertia, especially as dokie.li and Solid aren't tools that were created in the last few months -- these efforts have years behind them, and they should work much better than they do currently i.e., they should be clear working examples of the core value proposition espoused by the Solid Project.

@RubenVerborgh
Copy link
Contributor

* I am not a fan of shortcut hacks.

Apologies, I certainly did not mean to imply that. I know you favor good work.

What I meant to say is that, in this context and in my opinion, changing the Accept headers to say Turtle would be a shortcut, because it is not the truth.

I see and sense unecessary inertia

Rest assured this is not how we work 🙂
The number of issues is simply larger than the number of available hands. We're doing the best we can.

@kidehen
Copy link

kidehen commented Nov 14, 2018

@RubenVerborgh ,

Here is what I meant to state and say:
NSS should use response metadata to inform user agents (e.g., dokie.li) about its preferences. That's it.

What I describe above isn't happening. Thus, the only way for dokie.li to work properly with NSS is to drop down from a good deductive pattern (that currently exists) to a hack.

This is an example of what I call a problem that's riddle-like in nature :)

Resoure wise, if a problem reaches he point where resources are the problem on either side, we've historically stepped in (be it NSS or dokie.li) and made a contribution.

/cc @csarven

@RubenVerborgh
Copy link
Contributor

NSS should use response metadata to inform user agents (e.g., dokie.li) about its preferences. That's it.

Check, thanks, following up on the two kinds of metadata in #934 and #935.

@kjetilk
Copy link
Member

kjetilk commented Nov 14, 2018

NSS should use response metadata to inform user agents (e.g., dokie.li) about its preferences. That's it.

Very, very much agreed. HATEOAS and all that. :-) I hope we get the chance to make this right soon.

@dmitrizagidulin
Copy link
Contributor

@csarven @kjetilk Couple more details about the Allow: header, to clear up confusion.

The Allow: header is there to return what the /server/ is capable of. In general. Across all resources. (This doesn’t seem very useful to me, in our case, but those are its semantics.) That is why it’s always returned, regardless of user authentication status.

The WAC-Allow: header (returned on http HEAD requests), on the other hand, is about what the /user/ is capable of. It does depend on authentication status. So Dokieli needs to look to it, not Allow-Patch (which is also general case).

Does that make more sense?

@kjetilk
Copy link
Member

kjetilk commented Nov 14, 2018

It does, thanks a lot, @dmitrizagidulin !

@csarven
Copy link
Member Author

csarven commented Nov 14, 2018

@dmitrizagidulin The semantics of Allow is for a given resource, not a general announcement for servers capabilities (possibly for "all resources"): https://tools.ietf.org/html/rfc7231#section-7.4.1 . So, the server should signal what it really can for resource in question.

I think WAC-Allow is orthogonal. It doesn't (and shouldn't) override Allow.

@RubenVerborgh
Copy link
Contributor

Indeed, please see #934 and #935, which have all of this info 🙂

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

No branches or pull requests

5 participants