validateHmac implementation missed
user-az opened this issue · 5 comments
Hi,
I used composer to install library
"robby-bugatti/shopify-php-sdk": "^1.0" //1.0.3
I use strict mode, as shopify API Guide recomends. Trying to get access token I've got an error "Call to undefined function validateHmac".
It's because the call
/** @var \Shopify\AccessToken $token */
$token = \Shopify\Auth::accessToken($nonce);
in strict mode is chained into implementation
/**
* Fetch an access token
* @param string $nonce
* @return \Shopfiy\AccessToken
*/
public static function accessToken( $nonce = NULL )
{
// Do any strict checking for our OAuth requests
if(\Shopify\Shopify::strict())
{
if( !self::checkNonce( $nonce ) ) throw new \Exception("Strict API execution requires a nonce for Authentication requests");
if( !\Shopify\Shopify::validateHmac() ) throw new Exception\ApiException("Strict API execution requires a valid HMAC signature");
}
return \Shopify\AccessToken::createFromCode($_GET['code']);
}
and there is no any implementation of \Shopify\Shopify::validateHmac().
Such a dirty thing.
I attempted to reproduce in one of our other projects, and it was working for me. Are you using the latest version?
On a side note, the authentication mechanism does need to be refactored. There's no use case I can see where 'strict' execution should actually be disabled. I don't think looking past an invalid HMAC is the worlds greatest idea. The only tricky part is nonce management. We could just chuck it in the session, but we were using a multi-server environment. I am looking at stateless nonce possibilites, but until then, we might just place it in session
Well, It seems composer's major version for ^1.0.x (1.0.3) is broken: here it calls
https://github.com/RobbyBugatti/shopify-php-sdk/blob/v1.0.3/lib/Auth.php
and thereis no implementation here
https://github.com/RobbyBugatti/shopify-php-sdk/blob/v1.0.3/lib/Shopify.php
This was a default installation by composer, I guess you should make 1.0.4 release, so making default composer installation to be fixed.
Now I've made
$ composer require robby-bugatti/shopify-php-sdk:dev-master
and get the last version. I'll try it, thanks.
On a side note, the authentication mechanism does need to be refactored
) I think a lot of the SDK code should be refactored.
For instance phpDocs notations tell IDE @return SomeTypeOfObject , but instance of splObject really is coming back from call. It slows down development process, because developers awaiting that declared interface is right and IDE gets right autocomplition of available classes/objects/methods. Not fatal but not good at all: having to read sources, looking ugly print_r transitional parts, etc. There is no @throws sections also.
As for nonce and transparent storing/restoring it from session, I think it's a good idea. I suggest you should inject some implementation of IStorage interface into you service. Your implementation can use php session, and other developers can implement IStorage interface by their own vision (db, files, redis, pigeons etc). Less couple, much more abstract )
I think also this is not best way to operate $_GET or any other globals inside SDK. I think this is a controller's layer. I prefer sdk takes params from controller/cli starter/test kit. For example you don't know the way, how request get into controller, there could be added $_GET params for routing/analytics/smthng else during nginx/apache pass. So your implementation of validateHmac fail then. Of couse this is mutually fail in that case.
One more thing - static. SDK suppose it's singleton. I think it's better when developer can resolve with Resource Locator or DI be it a singleton or another instance. Right now developer should bear in mind, that thereis only one instance, initialized with THIS concrete shop. So having a bunch of shops inside cli or daemon developer should care about which one of them use SDK at the moment.
So, it was just my point of view. In any case thanks for you code, it seems it works ) and this is good.
Thanks.
P.S.
Guide https://help.shopify.com/api/guides/authentication/oauth says
The characters & and % are replaced with %26 and %25 respectively in keys and values. Additionally the = character is replaced with %3D in keys.
I suppose
$params[$param] = "{$param}={$value}";
should be replaced with somthing like this
$params[$param] = str_replace(['=', '%', '&'], ['%3D', '%25', '%26'], $param) . '=' . str_replace(['%', '&'], ['%25', '%26'], $value);
I definitely agree with you on most counts. A few of the decisions were made specifically for our use case, which obviously wont work for everybody.
I suggest you should inject some implementation of IStorage interface into you service.
Would you happen to have any code samples for an implementation. I am all for giving devs the opportunity to choose, as environments differ so greatly.
Looking at Facebook's Authentication, it defaults to session, but you can obviously override it. We could open an interface allowing set() and get() methods, and devs can plug in any functionality they want, if required, Facebook.php
I think also this is not best way to operate $_GET or any other globals inside SDK
What do you think about parsing $_SERVER['REQUEST_URI'] to create the SDK's own source for $_GET? This way frameworks can do whatever they want, and our SDK won't care. Is there any case that 'REQUEST_URI' would be different? And if there is, I'm not sure how much we can do about it.
Outside of that, did switching to dev-master resolve the issue? I'll try and push a new release sometime today.
Thanks for the feedback.
Outside of that, did switching to dev-master resolve the issue?
Yes, thanks, it now works for me and also with switched security option.
What do you think about parsing $_SERVER['REQUEST_URI'] to create the SDK's own source for $_GET?
I think you are right, it'll be better way to parse $_SERVER['REQUEST_URI'] for params as a native signed request from Shopify.
As for me, SDK can declare interface a-la FrontEnd-role with single method "getParams" and make mostly perfect implementation of the one with $_SERVER['REQUEST_URI'].