Change normalizeIncludeTotals() in GenericResource to have sane defaults

[kler]

Changes

• Change default param include_totals to be false instead of null
  The endpoint GET /api/v2/users/{id}/roles accepts a param called include_totals, which is expected to be a boolean. However, the default in GenericResource::normalizeIncludeTotals() is null.

I argue that false should be the default, or that the default should be removed all along (normalizeIncludeTotals() unconditionally adds the param, which I believe is wrong, however in this PR I only changed the default though).

On submitting include_totals=null this is what the API will respond:

POST https://xyz.auth0.com/api/v2/users/abc/roles` resulted in a `400 Bad Request` response:
{"statusCode":400,"error":"Bad Request","message":"Payload validation error: 'Expected type string but found type null'  (truncated...)

On top of this the function normalizeIncludeTotals() had an unnecessary check:

if (isset( $params['include_totals'] )) {

The key include_totals in $params will always be set, because either the user provides it, or if not it's added in the statement above. I removed the check whether it's set or not.

Checklist

[x] I have read the Auth0 general contribution guidelines.

[x] I have read the Auth0 Code of Conduct.

[ ] All existing and new tests complete without errors.
=> No, since the testing tenant needs to be precisely tailored for the test.
Getting for instance:

POST https://xyz.auth0.com/dbconnections/signup` resulted in a `400 Bad Request` response:
{"error":"public signup is disabled"}

And test doesn't contain necessary backoff algorithm, resulting in too_many_requests

[x] The correct base branch is being used.

[joshcanhelp]

@kler - I appreciate the contribution here but I think that this is all working as expected.

The first isset check looks for an existing value (other than null) and, if nothing is present, sets it as the method parameter. The second check again makes sure we have a value other than null and makes sure it's a boolean. If include_totals leaves this method with a null value and is passed to where the URL is built, it will be included as an empty string:

https://github.com/auth0/auth0-PHP/blob/master/src/API/Helpers/RequestBuilder.php#L199

Do you have a use case where this is failing or was this just a review of the code that was there? If we've got a bug, I'm happy to take a closer look.

Thank you!

[kler]

Do you have a use case where this is failing or was this just a review of the code that was there?

I got a case where it's failing, it's the one I posted above in the PR.
This one:

On submitting include_totals=null this is what the API will respond:

POST https://xyz.auth0.com/api/v2/users/abc/roles` resulted in a `400 Bad Request` response:
{"statusCode":400,"error":"Bad Request","message":"Payload validation error: 'Expected type string but found type null'  (truncated...)

In fact I just realised that the second if(isset()) is not only a cosmetic problem, it's in fact the culprit. It's the bug.

I'll walk through the code.

    // => Let's assume $params is an empty array: $params = []
    protected function normalizeIncludeTotals(array $params, $include_totals = null)
    {
        // => This if() will be TRUE, since no include_totals in $params
        // User parameter include_totals if params does not have the key.
        if (! isset( $params['include_totals'] )) {
            // => include_totals will now be NULL, since no second parameter provided
            $params['include_totals'] = $include_totals;
        }

        // => This if() will be FALSE, since include_totals in $params is NULL
        // If include_totals is set (not null), then make sure we have a boolean.
        if (isset( $params['include_totals'] )) {
            $params['include_totals'] = boolval( $params['include_totals'] );
        }

        // => At this point $params['include_totals'] = NULL, which is not accepted by the API, since the API is expecting a bool

        return $params;
    }

Let's put it like this: Since the API expects the include_totals to be a boolean, what reason do we have to not perform the boolval($params['include_totals']) ?
I still argue that this should always happen, not only for cosmetic reasons, but because it's a bug otherwise; NULL values will slip through to the API.

[kler]

There are two possible fixes

• Always make sure include_totals is a boolean
• Remove the include_totals entry if it's not a boolean

[joshcanhelp]

@kler - Can you send me the complete code you're using to cause this error?

I see what you're saying here but the final processing of the URL converts null to a blank string so the URL sent is https://tenant.auth0.com/api/v2/roles?include_filters=. That processor should probably just strip the parameter altogether but the point of null converting to no value is to keep the default API value if nothing is passed in.

I just wrote a test to try this out and I get results back just fine:

$api = new Management('API_TOKEN', 'auth0-php.auth0.com');
$response = $api->roles->getAll( [ 'include_filters' => null ] );
echo '<pre>' . print_r( $response, TRUE ) . '</pre>'; die();

I'm more worried about the URL param convertor not doing it's job (which it looks, to me, like it is).

[kler]

Here's a simple curl which shows that an empty include_totals is interpreted as NULL by the API.

Will WORK since include_totals is omitted

curl \
-H 'Authorization: ...' \
-H 'Auth0-Client: ...' \
"https://subdomain.auth0.com/api/v2/users/<user_id>/roles"

Will FAIL since include_totals is interpreted as NULL

curl \
-H 'Authorization: ...' \
-H 'Auth0-Client: ...' \
"https://subdomain.auth0.com/api/v2/users/<user_id>/roles?include_totals="

{"statusCode":400,"error":"Bad Request","message":"Query validation error: 'Expected type boolean but found type string' on property include_totals (<code>True</code> if a query summary must be included in the result, <code>False</code> otherwise. Default <code>False</code>).","errorCode":"invalid_query_string"}

The fix needs to be one of:

1. Remove include_totals if it's not a boolean
2. Always convert include_totals to a boolean
3. Throw an Exception if it's not a boolean

1+2 are both "lazy" and "silent" - i.e. users of the library may use it wrong and don't understand it.
3 is somewhat annoying for such an "unimportant thing".

Personally I think I would have resorted to 2, which is also exactly this PR :)

[joshcanhelp]

@kler - You're correct, a direct call to that API with "null" as a value will fail but the SDK never sends null, it's send as blank, which just takes the API default.

Do you have a code sample using the SDK that's failing?

[kler]

return $auth0Api->users->getRoles($userId);  <-- Will FAIL, should be able to use w/o second param
return $auth0Api->users->getRoles($userId, ['include_totals' => false]); <-- Will WORK

[joshcanhelp]

@kler - Thank you for that, I can reproduce the issue now. The example I posted above uses a method that does not use normalizeIncludeTotals() so it was working fine.

So, it looks like the problem is not that "null" is sent, it's that a blank value for include_totals fails because it's evaluated as a blank string (a little confusing because "true" and "false" are also both string but not much you can do in a URL).

The question is ... how best to guide people here? I don't think converting everything to a boolean is the right way to do this:

❯ php -r 'echo (bool) "FALSE" ? "I am TRUE" : "I am FALSE";'
I am TRUE

... and I want to keep the API default if nothing is passed in. I think the best way to handle this is to check whether the key exists and check if it's boolean, then unset if not. I'll fix the URL query handler to avoid this issue in a separate PR (remove the param if the value is empty).

Does that sound like a good approach? Thank you for sticking with this!

[kler]

Use filter_var() with FILTER_VALIDATE_BOOLEAN instead. That will provide sane boolean conversions.

(On cell, so didn't bother to format commen or provide links).

[joshcanhelp]

@kler - TIL, I didn't know FILTER_VALIDATE_BOOLEAN worked like that!

As long as null is retained (so the API default is used when nothing is provided) then I'm 👍 for that. Keep the null default and I'll address how that's converted to a param in the query string.

[kler]

Both $include_totals = NULL or $include_totals = false will yield the same result given that filter_var() is used (both NULL and false will result in false). But I will still argue that the default should be false, not NULL.
Reason: NULL is not a valid option to the API.

If anything, it should be checked that the user doesn't provide NULL as a param, because that's a complete misuse of the lib and only indicates the user doesn't understand it.

But I won't make that change though - but I strongly believe the param default should be something that the API accepts - or do you have any good argument why the default should be something that the API doesn't accept?

[kler]

Quick change - I've changed the default to NULL, and unsetting the key if it's NULL in change
f6b2720.
How about that?

[joshcanhelp]

You can remove this check. I'll submit a PR to exclude any parameters set to null in the parameter builder. That way we can avoid these checks in other "normalize params" methods we might add in the future. If you add FILTER_NULL_ON_FAILURE as a third param, then we can handle all irrational arguments the same way.

[kler]

Like this then @joshcanhelp ?

[joshcanhelp]

This will keep null as the default (which we'll handle in the param builder) and convert anything that's not filterable to null as well.

Appreciate you sticking with this ... apologies if my feedback has not been clear.

[joshcanhelp]

Thank you once again for the PR and working through it with me 🎉

[joshcanhelp]

Closing and reopening to trigger CI

[joshcanhelp]

Sorry for all the notifications 😄

[github-actions]

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.