Closed (fixed)
Project:
Drupal core
Version:
8.0.x-dev
Component:
rest.module
Priority:
Normal
Category:
Task
Assigned:
Issue tags:
Reporter:
Created:
30 Jul 2013 at 23:02 UTC
Updated:
29 Jul 2014 at 22:43 UTC
Jump to comment: Most recent, Most recent file
Comments
Comment #1
juampynr commentedHere it is.
It adds support to the _auth setting when defining REST resources at rest.settings.yml.
It also implements a test where a REST resource is added which only supports HTTP authentication and then we request it anonymously and then using an authenticated user to test it.
I had to copy the method basicAuthGet() from HttpBasicTest into RESTTestBase since I needed it. I am not sure if we should move it up to WebTestBase.
Comment #2
juampynr commentedHere is an updated version where I am removing devel module from the list of modules to enable in the test (doh!).
Comment #3
juampynr commentedThis patch changes the authentication methods in a REST resource to supported_auth, as suggested by @klausi.
Comment #4
Crell commentedTagging. I think this is a necessary addition. Will review later.
Comment #5
juampynr commentedReminder...
Comment #6
juampynr commentedRelated: this module will make use of this piece of logic to define authentication for REST resources:
https://drupal.org/project/restui
Comment #7
klausiInsufficient comment, should be "Check if there are authentication provider restrictions in the configuration and apply them to the route."
In this case I think it is enough to test one entity type.
This line should be removed?
Unclear comments. The first should be "Try to read the resource with session cookie authentication, which is not enabled and should not work."
The second one: "Now read it with the Basic authentication which is enabled and should work."
And we should have a test case here with a not authenticated request (anonymous user), which should also return 403.
Should be "Performs a HTTP request with Basic authentication.
Comment #8
juampynr commentedThanks for the review. Here it is.
Comment #9
klausidescription should be "Tests authentication provider restrictions."
So the foreach loop is not necessary anymore.
Wrong comment: the request is authenticated, but with session cookies.
Comment #10
juampynr commentedHere it is.
Since I removed the loop, the interdiff is not that useful. I am adding it anyway.
Comment #11
klausiNo tests on users and nodes present.
@{inheritdoc} missing.
I think we should call $this->drupalLogout() after the session cookie test, because the basicAuthGet request sends cookies and Basic auth credentials currently.
And we are missing an example in rest.settings.yml, same as we have there for supported formats.
Comment #11.0
juampynr commentedUpdated issue summary.
Comment #12
juampynr commentedDone!
Comment #14
juampynr commented#12: drupal-allow-auth-on-rest-resources-2054187-12.patch queued for re-testing.
Comment #15
Crell commentedWe should probably document here what happens if none are set. (ie, I believe all are acceptable?)
No t() in test classes. WebTests can use String::format() if they need placeholders, but in this case we don't even need that.
This error happens when the request is not AUTHORIZED, but has been authenticated. (That is, we know who you are, we just don't like you.)
As above, this should be "authorized".
Comment #16
klausiNeeds work per #15.
Comment #17
juampynr commentedI have verified that when a route does not define authentication methods, only cookie authentication is available. Let's discuss this at #2064009: Review default authentication logic when no authentication methods are defined.
Made the rest of the suggested changes.
Comment #18
klausiThe ->curlClose() is not really necessary, but it does not hurt either. I think the comment in the YAML file is good enough, it is the same as for supported formats, so it should make sense as it is now.
Comment #19
dries commentedCommitted to 8.x. Let's add a change notification. Keep up the great work.
Comment #20
moshe weitzman commentedNice work... also, please update the docs at https://drupal.org/documentation/modules/rest
Comment #21
alexpottCommitted 4ca797a and 2cfbc18 and pushed to 8.x because a little extra got in with this change.
Comment #22
klausiComment #23
juampynr commentedAdded change record.
I have added an example of use at the REST docs page, but I also want to add there a cURL example to create a user. I need to investigate a little bit more since the following snippet returns "Method not allowed":
Comment #24
klausi"Currently, POSTing is only supported with media type application/hal+json. Enable HAL module to support this media type." https://drupal.org/documentation/modules/rest
Comment #25
linclark commentedjuampy says:
klausi says:
I do not understand how these two statements correspond. Available formats are all enabled by default. Available auth is not.
If we only have two keys in the config file, and they share a patten as 'supported_formats' and 'supported_auth' do, they should at least work in a consistent way. I'm fine with making it so that you have to define your supported formats. In fact, I think that could make it clearer to devs what is going on.
Comment #26
juampynr commented@klausi added an example to post a node using HTTP basic authentication. So the documentation is up to date now.
https://drupal.org/node/1978890
Marking as fixed.
Comment #27
Crell commentedComment #28
klausiFollow-up: #2065193: supported_formats and supported_auth should work in the same way
Comment #29.0
(not verified) commentedUpdated issue summary.