bigcommerce / bigcommerce-api-php

Connect PHP applications with the Bigcommerce Platform
https://developer.bigcommerce.com
MIT License
144 stars 186 forks source link

`Bigcommerce::getTime()` doesn't validate store credentials and returns garbage #298

Closed luke8086 closed 4 months ago

luke8086 commented 4 months ago

Expected behavior

\Bigcommerce\Api\Client::getTime() should fail on invalid store credentials, as documented

Actual behavior

\Bigcommerce\Api\Client::getTime() uses $api_url instead of $api_path so it effectively requests https://api.bigcommerce.com/time, which doesn't check for any store credentials.

On top of that, that endpoint returns timestamp in milliseconds, and getTime() parses is as seconds, so it returns bogus DateTime object

Steps to reproduce behavior

Run without any prior setup:

>>> \Bigcommerce\Api\Client::getTime()
=>  DateTime @1718280499756 {#5814
      date: 56420-02-21 18:49:16.0 +00:00,
    }
TomA-R commented 4 months ago

Thank you for reporting this issue, @luke8086! I don't believe we have plans to add authentication to the /time endpoint but you're absolutely correct that \Bigcommerce\Api\Client::getTime() returns garbage. I've opened https://github.com/bigcommerce/bigcommerce-api-php/pull/299 to address this.

luke8086 commented 4 months ago

Thank you @TomA-R for the quick response!

I can see you already have an authenticated /time endpoint - https://developer.bigcommerce.com/docs/rest-management/store-information#get-system-timestamp

You just need to switch getTime() from $api_url to $api_path to use it 🙂

>>> $url = \Bigcommerce\Api\Client::$api_path . "/time"
=>  "https://api.bigcommerce.com/stores/6jvqbmuz8y/v2/time"

>>> $resp = \Bigcommerce\Api\Client::getConnection()->get($url)
=>  {#5817
      +"time": 1718286038,
    }

>>> new DateTime("@{$resp->time}")
=>  DateTime @1718286038 {#5799
      date: 2024-06-13 13:40:38.0 +00:00,
    }
TomA-R commented 4 months ago

Thank you @TomA-R for the quick response!

I can see you already have an authenticated /time endpoint - https://developer.bigcommerce.com/docs/rest-management/store-information#get-system-timestamp

You just need to switch getTime() from $api_url to $api_path to use it 🙂

>>> $url = \Bigcommerce\Api\Client::$api_path . "/time"
=>  "https://api.bigcommerce.com/stores/6jvqbmuz8y/v2/time"

>>> $resp = \Bigcommerce\Api\Client::getConnection()->get($url)
=>  {#5817
      +"time": 1718286038,
    }

>>> new DateTime("@{$resp->time}")
=>  DateTime @1718286038 {#5799
      date: 2024-06-13 13:40:38.0 +00:00,
    }

Thank you @luke8086, that's a valid point. Since that would change the behaviour and prerequisites of the call, I imagine that changing this might cause some frustration for people who are using this to simply check API connectivity. So perhaps introducing a ::getStoreTime() method would make sense here, what do you think?

luke8086 commented 4 months ago

@TomA-R Yep, makes sense! I agree about adding a new method to avoid breaking it for existing users.

Note though that both the docstring and the README say that getTime() tests connection to the store, not just the API, so they'll need to be updated.

TomA-R commented 4 months ago

Thanks again for reporting! The fix has been merged and released in version 3.3.7.