Hi - Terrific and powerful module!

I am wondering why after all these years Services responds with a 401 Unauthorized for missing arguments rather than 400 Bad Request?

Ex. If a route is declared as PUT /soup/:id, and one of the required arguments is data: { temperature: hot }, and that value is not provided, I get a 401 but would expect and want a 400.

Is it just me and my convention, or is a 400 really more appropriate? Note: This occurs bc of the line:

          return services_error(t('Missing required argument @arg', array('@arg' => $argument_info['name'])), 401);

and to change this behavior (as far as I know!) is just updating 401 to 400 in includes/RESTServer.inc:

          return services_error(t('Missing required argument @arg', array('@arg' => $argument_info['name'])), 400);

Comments

texas-bronius created an issue. See original summary.

tyler.frankenstein’s picture

Status: Active » Needs work
StatusFileSize
new898 bytes

Agreed. We'll want to get a change log record mentioned for this, because some clients may be specifically watching for the 401 after all these years ;) - and we can give them a heads up to the change this way.

Kyle, any thoughts/long-term-implications on making a change to this response code?

kylebrowning’s picture

Yeah it could break clients. Not sure how to handle it, but I would push it out ASAP so that it doesnt have to come with a forced update later incase we get anymore SA's.

ON the other hand, im sure someone will miss this and it will break.

Im wondering if we tag a 7.x-4.x for Services.

An alternative oslution, is to self apply the patch themselves. until we do reelase a 4.x

texas-bronius’s picture

Issue summary: View changes
texas-bronius’s picture

Well.. that was certainly fast to get attention and eyes on it: Thank you! I can also appreciate the challenge with maintaining existing implementations. A very-Drupal workaround could be $conf or variable preference, settable to future 400, defaulting to existing 401 behavior. How rigid you make it (fixed 400/401 toggle vs a free form nnn code) is something else to consider.

texas-bronius’s picture

StatusFileSize
new1.21 KB

The attached patch against master (which I believe to be 7.x-3.x-dev?):
* Out of the box responds with 401 Unauthorized (no change in behavior for existing users)
* Provides a $conf['services_deprecated_missing_arg_code'] allowing a concerned user to set to 400 (Bad Request)
* Provides comments in code about this and how to implement

Plan to obsolete this crutch in 7.x-4.x or as you see fit.

kylebrowning’s picture

Status: Needs work » Needs review
texas-bronius’s picture

StatusFileSize
new1.1 KB

Woops - Attached is better. Using variable_get() seems more appropriate and still (as before) allows variable_set() and $conf manipulation in settings.php just the same.

kylebrowning’s picture

I actually prefer $conf, because variable_gets can be a massive overhead on requests.

texas-bronius’s picture

Thanks for your reply @kylebrowning. I think I get what you're saying, and I thought so, too, at first. Then I took a look under the hood to see what's what. The variable_get() function is doing exactly what I did before but without me having to clutter your codebase with a stray global $conf; in the middle of a function call:

function variable_get($name, $default = NULL) {
  global $conf;

  return isset($conf[$name]) ? $conf[$name] : $default;
}

The only overhead with variables occurs when they're first -all- loaded from db into the global $conf array at some point in bootstrap. Recall that we can still set the value for this conf/variable either by vset or from $conf in settings.php. Here's more:
https://drupal.stackexchange.com/a/187670

marcingy’s picture

I like the look of this patch and yes no overhead unless set in db, and even then by this point in time https://api.drupal.org/api/drupal/includes%21bootstrap.inc/function/_dru... will have been called and db values merged into conf values.

marcingy’s picture

Status: Needs review » Reviewed & tested by the community

Forgot to do this!

kylebrowning’s picture

Go ahead, im cool with this.

tyler.frankenstein’s picture

Status: Reviewed & tested by the community » Fixed

The patch didn't apply cleanly to the latest 7.x-3.x, so I touched it up and committed it.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.