For instance - ".../oauth2/client" [GET] will return (in case email field is restricted for a user):
Expected behavior:
Observed behavior:
| Art.tsymbar | |
| Nov 11 2020, 6:42 PM |
| F33912582: screen_.png | |
| Nov 11 2020, 6:42 PM |
| F33912580: screen (1).png | |
| Nov 11 2020, 6:42 PM |
For instance - ".../oauth2/client" [GET] will return (in case email field is restricted for a user):
Expected behavior:
Observed behavior:
| Subject | Repo | Branch | Lines +/- | |
|---|---|---|---|---|
| ListClients.php: Filter clients response data | mediawiki/extensions/OAuth | master | +10 -5 |
| Status | Subtype | Assigned | Task | ||
|---|---|---|---|---|---|
| Resolved | Art.tsymbar | T264457 Client secret shared between clients | |||
| Resolved | Art.tsymbar | T267755 OAuth extension - REST - show error (rights restrictions) messages instead of an object '{}' |
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.
Change 645044 had a related patch set uploaded (by Vadim Kovalenko; owner: Vadim Kovalenko):
[mediawiki/extensions/OAuth@master] ListClients.php: Show error message
Change 645044 merged by jenkins-bot:
[mediawiki/extensions/OAuth@master] ListClients.php: Filter clients response data