Bug In How getEnrolledFactors Handles the otp_devices API Data

[zachary-lee]

First, I want to thank you for this SDK. It's been quite helpful to our organization.

I discovered a bug in the OneLogin PHP SDK caused by what I think is an API inconsistency related to the GET /users/{user_id}/otp_devices API endpoint endpoint.

The primary issue is in the OneLoginClient.php file within the methods handleDataResponse and getEnrolledFactors. This also affects getFactors in the same way as getEnrolledFactors.

The issue seems to stem from the fact that the otp_devices endpoint is returning its data wrapped in an extra object with the single property '[otp_devices]=>array(...)' instead of just being a flat array of OTP devices similar to how GET /users is a flat array of the users.

This is causing two issues on line 226--if (count($data) == 1 && empty($data[0])) {--in handleDataResponse. First, count($data) is raising a warning because stdClass objects don't implement the countable interface, and second, since the value of $data is an object, empty($data[0]) is evaluated to true since the data itself is actually in $data->otp_devices. The causes the function to always return an empty array regardless of the data returned by otp_devices.

I've added a bandaid to this for now by circumventing the if check by adding

if (is_object($data)) {
  return $data;
}

on line 227. This appears to work in our environment without raising issues elsewhere, but we don't use a majority of the SDK functions, and the test suite doesn't seem to be mocking (or even touching) any of the SDK's API calls.

In full, "handleDataResponse" now looks like

protected function handleDataResponse($response) {
  $data = null;
  $content = json_decode($response->getBody());
  if (property_exists($content, 'data')) {
    $data = $content->data;
  }
  if (is_object($data)) {
    return $data
  }
  if (count($data) == 1 && empty($data[0])) {
    return [];
  }

  return $data;
}

Upon reflection on this, a better solution to this is likely to cast data to an array and then call array_values on the new array to get numerically indexed keys.

Regardless, having resolved this, another issue popped up in the way the library handles creating new OTPDevice objects. The issue stems from the way fields are added to the object. If the resulting otp_device data doesn't include every field expected by the constructor, the library raises an error which is caught by the catch (\Exception $e) on line 2186 (related to issue 11 in the SDK's github -- not opened by me) and the code returns without creating any OTPDevices.

To resolve this, we've added null coalesce operators with empty string defaults like

public function __construct($data) {
  $this->id = isset($data->id)? (int) $data->id : null;
  $this->active = $data->active ?? '';
  $this->default = $data->default ?? '';
  $this->authFactorName = $data->auth_factor_name ?? '';
  $this->phoneNumber = $data->phone_number ?? '';
  $this->typeDisplayName = $data->type_display_name ?? '';
  $this->needsTrigger = $data->needs_trigger ?? '';
  $this->userDisplayName = $data->user_display_name ?? '';
  $this->stateToken = $data->state_token ?? '';
}

but these fields may need to actually have some real default instead of just empty strings. The issues for me were phone_number and state_token, but I went ahead and headed off future issues for us by adding them to every field.

With these changes, it appears that I'm finally getting valid OTP results.

We created a case with the support team about this (CS0267403) and they pointed us here. I'd be happy to open a PR (or 2) for this with my exact changes above, but I'm hesitant to do that. Primarily, I'm not sure that my PHP code fixes are the best solution. There don't seem to be any unit tests for individual functions--just some to check if the library generally is working--so any change I make could adversely affect other functionality. Additionally, I'm not entirely sure if this project is still maintained (I hope so! This has been really helpful!). The last update was nearly a year ago -- comparing this to the python SDK which seems to still be under active development.