In documentation for the resources definition there is part with key 'actions' that allows defining own calls.

But unfortunately they can be only POST calls but not GET. Because of:

    // Detect action requests
    else if ($pc==1 && $method=='POST') {
      return $resource['actions'][$path[0]];
    }

in RESTServer.inc function resolveController.

Is there any special reason for that or it is a bug that should be changed to:

    // Detect action requests.
    else if ($pc==1 && isset($resource['actions'][$path[0]])) {
      return $resource['actions'][$path[0]];
    }

Please let advise.

CommentFileSizeAuthor
#1 services-rest-actions-get-987180.patch1.99 KBygerasimov

Comments

ygerasimov’s picture

ygerasimov commented
Status: Active » Needs review
StatusFileSize
new services-rest-actions-get-987180.patch1.99 KB

Sorry. Above mentioned changes not enough. Please review attached patch to allow non POST actions callbacks to be called.

voxpelli’s picture

voxpelli commented
Title: REST server resource actions. » Allow GET-requests for resource actions in the REST server
Component: Documentation » Code
Category: bug » feature

Actions are arbitrary RPC-style function calls and all RPC-style mechanisms must use the POST-method because that's the only method that safely and reliably can execute such functions - all other HTTP methods are idempotent and most RPC-style functions isn't. In addition to being idempotent the GET-method in particular is also defined as "safe" which means it may not change anything on the server - most RPC-style actions will change things and allowing GET-requests for them might then open up XSS-vulnerabilities and similar.

All GET-style methods should be implemented as the "retrieve" or "index" methods on resources.

I would say this is a won't fix.

You can read more about HTTP methods at eg. http://en.wikipedia.org/wiki/Hypertext_Transfer_Protocol#Request_methods

ygerasimov’s picture

ygerasimov commented
Status: Needs review » Active

@voxpelli I completely on the opinion that only retrieval actions should be accessible with GET method.

But from developer perspective it is very inconvenient to create separate resources for having his GET call to be implemented. Nice example is file_resource. There is action nodeFiles that does only retrieval, so there should be possibility to make possible to use it with GET method.

Maybe there should be possibility in REST server to choose what methods are allowed for any particular method. By default only POST method is checked, but admin can also allow some others.

What do you think about this?

kylebrowning’s picture

kylebrowning commented

Maybe nodeFIles need to not be an action, its more like an index. Hrmm ill think about this a bit.

voxpelli’s picture

voxpelli commented

Relationships are GET:s as well - forgot them. "nodeFiles" should probably be a "files" relationship on the node resource - that would solve it?

Looking at the resources I think we still have a few improvements left to do on them. Especially the file one, but also eg. the comment resource. Will try and get Hugo to take a look at them and otherwise try to get some time myself.

Do we have target date for stable release? Would love to do a review prior to stable release, but am short of time due to trying to get OAuth released (which would kind of be needed for a stable release of Services for D7 as well).

gdd’s picture

gdd
he/him
Primary language English
Location Portland, OR
commented

My only date for release is 'sooner than later'. We have already missed our D7CX pledge so the one hard date I had is gone. I am concerned about waiting too long though. This just opens up the possibility that people will get impatient and fork or start doing production work with the betas or RCs, and it looks like we're talking about implementing more API changes still (this is also true in the Session Auth issue.) Again, I'm not completely against this, we only have 90 users right now, but its something to be aware of.

That said, I want it to be as strong and usable a release as I can and voxpelli is probably the one who has used this codebase the most in real life, so any suggestions are welcome.

gdd’s picture

gdd
he/him
Primary language English
Location Portland, OR
commented

As far as this issue, I concur that it seems like any retrieve-only methods should ideally be accessible through GET, so if we can figure out a way to do that it would be great.

kylebrowning’s picture

kylebrowning commented
Status: Active » Fixed

I tested this patch, ran it through all tests, and it appears to be working, thanks ygerasimov,

voxpelli’s picture

voxpelli commented
Title: Allow GET-requests for resource actions in the REST server » Don't allow GET-requests for resource actions in the REST server
Category: feature » bug
Priority: Normal » Critical
Status: Fixed » Active

So there's no point in me arguing against a patch? As long as it applies you will commit it anyway? Should I prepare my e-mail to the security team so that it's ready in time for the stable release?

Quite disappointed at the outcome here - I don't have time to roll patches myself as I need to focus fully on releasing a stable version of OAuth for D6 and port it to D7 - which is something that benefits Services as well. All I have time to do is to point out how I think things should be solved. It would be okay if you committed this patch because you could argue that my standpoint is wrong - but if you just ignore my objections? Not okay… :/

SimmeLj’s picture

SimmeLj commented

I'm with @voxpelli here.
Actions are API calls that modify the resource. GET requests should never modify anything. If you do need to have additional endpoints accessible via GET you should use a relationship. Relationships are there to allow you to retrieve information related to the resource.

If you as a developer really want to expose data modifying endpoints accessible via GET go ahead, but at least use a relationship. Then it will be your code that's incorrect REST and not the server module. Or roll your own server implementation that will let you do anything.

Committing this patch will break the REST philosophy and if a valid reason for committing it can't be provided I, as voxpelli, will strongly object to it.

ygerasimov’s picture

ygerasimov commented

Guys, I am quite agree with voxpelli and on #7. Yes GET calls should not change anything. Maybe we should make some settings for REST server to provide selection with radios what methods each action can be called with. In this way we will move responsibility of making decision to admin. That would be very nice feature.

@kylebrowning I would like to request to rollback this patch and wait for proper solution with making settings.

@SimmeLj Please advise what do you mean by relationship?

ygerasimov’s picture

ygerasimov commented

thanks to @voxpelli I got the idea about relations. For the idea about them you can review http://drupal.org/project/rest_server . I think we will need to change resources to use relations properly instead of allowing actions.

kylebrowning’s picture

kylebrowning commented
Priority: Critical » Major

Ok, rolling back the patch.

Voxpelli, I misunderstood what heyrocker had said, so please dont assume Im going to commit patches just b/c they are good.

I think the proper approach to this is just roll back and have documentation explaining whats going on.

One issue I can see is that we dont have any examples of relationships, and the bigger problem might be that things like file/nodeFiles needs to be come a relationship with node.

This retrieve only method needs a home, and requiring POST to it, doesnt really make sense when you are not changing any data.

Ive rolled this patch back, so im no longer marking this as critical, but a major.

SimmeLj’s picture

SimmeLj commented

I think the proper solution to this problem would be to allow developers to define "sub resources". That is, a resource that can be accessed via an URL relative to a parent resource.

For example, if you have an endpoint called "api" and a resource called "node" you might get a node by calling
GET /api/node/1

And a relationship to access it's files
GET /api/node/1/files

And then the new sub resource, it's it's own resource, but in the definition you put 'parent' => 'node', or something. So for example, a sub resource to list all files attached to nodes. Accessed via:
GET /api/node/files

So this last example would call the index method on the files resource.

Just a thought.

voxpelli’s picture

voxpelli commented

@SimmeLJ: I think I would prefer #1042674: Add non-targeted relationships.

kylebrowning’s picture

kylebrowning commented
Status: Active » Closed (duplicate)

So im closing this issue and #1042674: Add non-targeted relationships will solve all of this.

gdd’s picture

gdd
he/him
Primary language English
Location Portland, OR
commented

See #1051158: Convert files/nodeFiles action to node/x/files relationship for more followup