Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

APIv4 - AJAX errors should say *something* useful #19526

Merged
merged 1 commit into from
Feb 4, 2021

Conversation

totten
Copy link
Member

@totten totten commented Feb 4, 2021

When calling APIv4 via AJAX, you may sometimes encounter an error. What response do you get?

Before

You are likely to get a completely blank response (status=500, body=[]). There is no information in any of the logs (Apache, PHP, CiviCRM, etc). You have no way to tell what's gone wrong.

Of course, if you're logged in as a full administrator, then you may have permission view debug output, in which case there might be something useful. But this won't help if you're using a less privileged user.

After

For the administrator (view debug output), you still get a detailed error response.

For less privileged users, the error is logged. The response provides a generic message along with an
"Error ID". You can use the "Error ID" to locate information in the log.

Screen Shot 2021-02-03 at 8 47 00 PM

Also, if the error is an UnauthorizedException, then the response code will be a semantic 403 instead of a generic 500.

When calling APIv4 via AJAX, you may sometimes encounter an error. What response do you get?

Before
------

You are likely to get a completely blank response (`status=500, body=[]`).
There is no information in any of the logs (Apache, PHP, CiviCRM, etc).  You
have no way to tell what's gone wrong.

Of course, if you're logged in as a full administrator, then you may have
permission `view debug output`, in which case there might be something
useful.  But this won't help if you're using a less privileged user.

After
-----

For the administrator (`view debug output`), you still get a detailed error response.

For less privileged users, the error is logged. The response provides a generic message along with an
"Error ID". You can use the "Error ID" to locate information in the log.

Also, if the error is an `UnauthorizedException`, then the response code will be a semantic 403 instead of a generic 500.
@civibot
Copy link

civibot bot commented Feb 4, 2021

(Standard links)

@civibot civibot bot added the master label Feb 4, 2021
@seamuslee001
Copy link
Contributor

This seems fine to me MOP

@seamuslee001 seamuslee001 merged commit 53095c4 into civicrm:master Feb 4, 2021
$statusMap = [
\Civi\API\Exception\UnauthorizedException::class => 403,
];
http_response_code($statusMap[get_class($e) ?? 500]);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@totten this code looks incorrect - it is defaulting to the array key 500 but the array does not contain that key.
I think it should be

Suggested change
http_response_code($statusMap[get_class($e) ?? 500]);
http_response_code($statusMap[get_class($e)] ?? 500);

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doh. Thanks @colemanw. Fix: #19533

totten added a commit to totten/civicrm-core that referenced this pull request Feb 4, 2021
This is a follow-up to civicrm#19526 which addresses a typo that causes a misbehavior in reporting the HTTP status code.

Before
------

If an API request encounters an exception, then it always returns HTTP 403.

After
-----

If an API request encounters an exception, then:

* It may return HTTP 403 (for an authorization exception)
* It may return HTTP 500 (for any other/unrecognized exception)
@totten totten deleted the master-api4-error branch February 4, 2021 21:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants