Page MenuHomePhabricator

OAuth extension - REST - show error (rights restrictions) messages instead of an object '{}'
Closed, ResolvedPublic5 Estimated Story Points

Description

For instance - ".../oauth2/client" [GET] will return (in case email field is restricted for a user):

Expected behavior:

Observed behavior:

Event Timeline

The Idea is just to: make DAOAccessControl::get() to return a string - like "$msg->text()" and not an $msg obj. as now.

Unfortunatelly __toString() will not be called in this case, only typecast can help but this is not an option to add "(string)" averywhere where $cmrAc->getXXXX() is used.

Usually if some field in inaccessible in REST API we just set it to null or omit it instead of setting it to the error message. How would you expect the client to differentiate between a real email and an error message?

The issue that @Pchelolo mentioned applied not only to the client, but also to internal code. If DAOAccessControl::get() always returns a string, internal callers don't have a good way to tell whether the call succeeded.

If I'm understanding Art's concern, in the current code functions like getEmail() return a Message object if the field is not available, which (when passed through all our plumbing for generating responses) end up returning the string "{}". So to omit it with current code, we'd need to test the return value everywhere that a Message object could be returned. That'd be painfully messy if we did it manually in all cases.

If we did actually want to return the error message to the client, I'm not sure why typecasting everywhere is not an option. It'd be a lot of places, but not an insurmountable amount.

If we prefer to omit/null the value (which makes more sense to me based on other things we've done in REST, thanks Petr for mentioning that) what if we wrapped these calls in a helper function in ListClients? Something like:

private function stringOrNull( $field ) {
  return isstring( $field ) ? $field : null;
}

We could probably come up with a better function name, and I'm not sure if it should be private or protected. But you get the idea.

Alternatively, if we choose to omit, we could have DAOAccessControl::get() return null unless an optional flag was set to return the Message object. I haven't checked the code to see what that might break. It might be okay, or it might be really bad.

apaskulin renamed this task from OAuth extension - REST - show error (rights restrictions) messages intead of an object '{}' to OAuth extension - REST - show error (rights restrictions) messages instead of an object '{}'.Nov 16 2020, 6:14 PM
Art.tsymbar set the point value for this task to 5.Nov 17 2020, 6:00 PM
apaskulin lowered the priority of this task from High to Medium.Nov 24 2020, 12:13 AM

Change 645044 had a related patch set uploaded (by Vadim Kovalenko; owner: Vadim Kovalenko):
[mediawiki/extensions/OAuth@master] ListClients.php: Show error message

https://gerrit.wikimedia.org/r/645044

Change 645044 merged by jenkins-bot:
[mediawiki/extensions/OAuth@master] ListClients.php: Filter clients response data

https://gerrit.wikimedia.org/r/645044

Resolving. Thanks, Vlad!