googleads / google-ads-php

Google Ads API Client Library for PHP
https://developers.google.com/google-ads/api/docs/client-libs/php
Apache License 2.0
291 stars 260 forks source link

Catchable Expired Tokens from Oauth #803

Closed necrogami closed 2 years ago

necrogami commented 2 years ago

Your client library and Google Ads API versions:

Your environment:

================= PHP GENERAL INFORMATION
phpinfo()
PHP Version => 8.1.6

System => Linux ip-10-0-2-94.us-west-2.compute.internal 5.10.109-104.500.amzn2.x86_64 #1 SMP Wed Apr 13 20:31:43 UTC 2022 x86_64
Build Date => May 11 2022 01:14:18
Build System => Red Hat Enterprise Linux Server release 7.9 (Maipo)
Build Provider => Remi's RPM repository <https://rpms.remirepo.net/>
Compiler => gcc (GCC) 10.2.1 20210130 (Red Hat 10.2.1-11)
Architecture => x86_64
Server API => Command Line Interface
Virtual Directory Support => disabled
Configuration File (php.ini) Path => /etc/opt/remi/php81
Loaded Configuration File => /etc/opt/remi/php81/php.ini
Scan this dir for additional .ini files => /etc/opt/remi/php81/php.d
Additional .ini files parsed => /etc/opt/remi/php81/php.d/10-opcache.ini,
/etc/opt/remi/php81/php.d/20-bcmath.ini,
/etc/opt/remi/php81/php.d/20-bz2.ini,
/etc/opt/remi/php81/php.d/20-calendar.ini,
/etc/opt/remi/php81/php.d/20-ctype.ini,
/etc/opt/remi/php81/php.d/20-curl.ini,
/etc/opt/remi/php81/php.d/20-dom.ini,
/etc/opt/remi/php81/php.d/20-exif.ini,
/etc/opt/remi/php81/php.d/20-fileinfo.ini,
/etc/opt/remi/php81/php.d/20-ftp.ini,
/etc/opt/remi/php81/php.d/20-gd.ini,
/etc/opt/remi/php81/php.d/20-gettext.ini,
/etc/opt/remi/php81/php.d/20-iconv.ini,
/etc/opt/remi/php81/php.d/20-ldap.ini,
/etc/opt/remi/php81/php.d/20-mbstring.ini,
/etc/opt/remi/php81/php.d/20-mysqlnd.ini,
/etc/opt/remi/php81/php.d/20-pdo.ini,
/etc/opt/remi/php81/php.d/20-phar.ini,
/etc/opt/remi/php81/php.d/20-posix.ini,
/etc/opt/remi/php81/php.d/20-shmop.ini,
/etc/opt/remi/php81/php.d/20-simplexml.ini,
/etc/opt/remi/php81/php.d/20-sockets.ini,
/etc/opt/remi/php81/php.d/20-sqlite3.ini,
/etc/opt/remi/php81/php.d/20-sysvmsg.ini,
/etc/opt/remi/php81/php.d/20-sysvsem.ini,
/etc/opt/remi/php81/php.d/20-sysvshm.ini,
/etc/opt/remi/php81/php.d/20-tokenizer.ini,
/etc/opt/remi/php81/php.d/20-xml.ini,
/etc/opt/remi/php81/php.d/20-xmlwriter.ini,
/etc/opt/remi/php81/php.d/20-xsl.ini,
/etc/opt/remi/php81/php.d/30-mysqli.ini,
/etc/opt/remi/php81/php.d/30-pdo_mysql.ini,
/etc/opt/remi/php81/php.d/30-pdo_sqlite.ini,
/etc/opt/remi/php81/php.d/30-xmlreader.ini,
/etc/opt/remi/php81/php.d/40-grpc.ini,
/etc/opt/remi/php81/php.d/40-igbinary.ini,
/etc/opt/remi/php81/php.d/40-msgpack.ini,
/etc/opt/remi/php81/php.d/40-protobuf.ini,
/etc/opt/remi/php81/php.d/50-redis.ini

PHP API => 20210902
PHP Extension => 20210902
Zend Extension => 420210902
Zend Extension Build => API420210902,NTS
PHP Extension Build => API20210902,NTS
Debug Build => no
Thread Safety => disabled
Zend Signal Handling => enabled
Zend Memory Manager => enabled
Zend Multibyte Support => provided by mbstring
IPv6 Support => enabled
DTrace Support => available, disabled

Registered PHP Streams => https, ftps, compress.zlib, php, file, glob, data, http, ftp, compress.bzip2, phar
Registered Stream Socket Transports => tcp, udp, unix, udg, ssl, sslv3, tls, tlsv1.0, tlsv1.1, tlsv1.2
Registered Stream Filters => zlib.*, string.rot13, string.toupper, string.tolower, convert.*, consumed, dechunk, bzip2.*, convert.iconv.*

This program makes use of the Zend Scripting Language Engine:
Zend Engine v4.1.6, Copyright (c) Zend Technologies
    with Zend OPcache v8.1.6, Copyright (c), by Zend Technologies
================= PHP EXTENSION INFORMATION
The PHP Extension grpc is installed: 1.45.0
The PHP Extension protobuf is installed: 3.20.1

Description of the bug: We have a need to be able to catch, and handle when a refresh token comes back as invalid or expired. We need to be able to send a message off to our customers to refresh their tokens. Currently i'm not seeing a way to handle this within the initiator.

Steps to reproduce:

        try {
            // Generate a refreshable OAuth2 credential for authentication.
            $oAuth2Credential = (new OAuth2TokenBuilder())
                ->withClientId(env('GOOGLE_ADS_CLIENT_ID'))
                ->withClientSecret(env('GOOGLE_ADS_CLIENT_SECRET'))
                ->withRefreshToken($refreshToken)
                ->build();
        } catch (\Throwable $e) {
            return $this->job->fail();
        }

        try {
            // Construct a Google Ads client configured from a properties file and the
            // OAuth2 credentials above.
            $googleAdsClient = (new GoogleAdsClientBuilder())
                ->withOAuth2Credential($oAuth2Credential)
                ->withDeveloperToken(env('GOOGLE_ADS_DEVELOPER_TOKEN'))
                ->withLoginCustomerId($customerID)
                ->withTransport('rest')
                ->build();
        } catch (\Throwable $e) {
            return $this->job->fail();
        }

with the code above passing in an invalid or expired token causes guzzle to throw this error into logs, it's not catchable even with catching \Throwable

Message
Client error: POST https://oauth2.googleapis.com/token resulted in a 400 Bad Request response:
{
 "error": "invalid_grant",
 "error_description": "Token has been expired or revoked."
}
Level
ERROR
Exception
{
    "class": "GuzzleHttp\\Exception\\ClientException",
    "message": "Client error: `POST https://oauth2.googleapis.com/token` resulted in a `400 Bad Request` response:\n{\n  \"error\": \"invalid_grant\",\n  \"error_description\": \"Token has been expired or revoked.\"\n}\n",
    "code": 400,
    "file": "/usr/local/bigtop/vendor/guzzlehttp/guzzle/src/Exception/RequestException.php:113",
    "trace": [
        "/usr/local/bigtop/vendor/guzzlehttp/guzzle/src/Middleware.php:69",
        "/usr/local/bigtop/vendor/guzzlehttp/promises/src/Promise.php:204",
        "/usr/local/bigtop/vendor/guzzlehttp/promises/src/Promise.php:153",
        "/usr/local/bigtop/vendor/guzzlehttp/promises/src/TaskQueue.php:48",
        "/usr/local/bigtop/vendor/guzzlehttp/promises/src/Promise.php:248",
        "/usr/local/bigtop/vendor/guzzlehttp/promises/src/Promise.php:224",
        "/usr/local/bigtop/vendor/guzzlehttp/promises/src/Promise.php:269",
        "/usr/local/bigtop/vendor/guzzlehttp/promises/src/Promise.php:226",
        "/usr/local/bigtop/vendor/guzzlehttp/promises/src/Promise.php:62",
        "/usr/local/bigtop/vendor/guzzlehttp/guzzle/src/Client.php:123",
        "/usr/local/bigtop/vendor/google/auth/src/HttpHandler/Guzzle6HttpHandler.php:47",
        "/usr/local/bigtop/vendor/google/auth/src/OAuth2.php:544",
        "/usr/local/bigtop/vendor/google/auth/src/Credentials/UserRefreshCredentials.php:114",
        "/usr/local/bigtop/vendor/google/auth/src/CredentialsLoader.php:214",
        "/usr/local/bigtop/vendor/google/gax/src/CredentialsWrapper.php:208",
        "/usr/local/bigtop/vendor/google/gax/src/Transport/HttpUnaryTransportTrait.php:111",
        "/usr/local/bigtop/vendor/google/gax/src/Transport/RestTransport.php:110",
        "/usr/local/bigtop/vendor/google/gax/src/GapicClientTrait.php:608",
        "/usr/local/bigtop/vendor/google/gax/src/Middleware/CredentialsWrapperMiddleware.php:61",
        "/usr/local/bigtop/vendor/google/gax/src/Middleware/FixedH

Expected behavior: A catchable exception that we can handle Request/Response Logs:

Anything else we should know about your project / environment:

fiboknacky commented 2 years ago

Do you mean it logs the error when trying to create a new UserRefreshCredentials in this line, right?

If so, that code is inside the google-auth-library-php. Could you please file a bug there instead?

necrogami commented 2 years ago

It is on that line, It exits, It doesn't log the error it doesn't throw a throwable error or exception it catches it internally and exits. And because of how it is caught internally i can't catch and handle to be able to tell customers that their token is invalid or expired.

necrogami commented 2 years ago

Also i have a ticket open with that team as well and have received no response.

https://github.com/googleapis/google-auth-library-php/issues/404

fiboknacky commented 2 years ago

Could you please try pinging the issue agin? I'm afraid I cannot do anything much here, as it's outside this repository.

necrogami commented 2 years ago

@fiboknacky now they are telling me that this needs to be handled as a middleware in google ads api library? Each side is pointing fingers at the other.

fiboknacky commented 2 years ago

After closely looking into this, it looks like the exception isn't printed out as a log directly.

The message part of ClientException may confuse us, since it beautifully prints out the error message in a JSON format.

However, it's still thrown as a normal exception. That means you can do something like this:

        try {
            // Generate a refreshable OAuth2 credential for authentication.
            $oAuth2Credential = (new OAuth2TokenBuilder())
                ->withClientId(env('GOOGLE_ADS_CLIENT_ID'))
                ->withClientSecret(env('GOOGLE_ADS_CLIENT_SECRET'))
                ->withRefreshToken($refreshToken)
                ->build();
        } catch (ClientException $e) {
            // Do something with $e.
        }

Could you give it a try?

necrogami commented 2 years ago

Except it's not, if you notice my code above, I'm catching \Throwable which is under \Error or \Exception the root of all throwable errors in php and it's still not being caught, this is because something upstream is catching the error before it comes to the client.

fiboknacky commented 2 years ago

I just tried yesterday and it worked perfectly. I don't think we have any layers that would catch ClientException. Do you mean it still doesn't work even if you specify an \Exception or ClientException like I mentioned?

necrogami commented 2 years ago

If you look at the code in the first post, it should just fail silently, It doesn't matter if i catch ClientException which extends Exception Which extends Throwable It doesn't matter which i try and catch, none catch the above refresh token error nor any authentication based error. Of which were currently seeing 3.

fiboknacky commented 2 years ago

If you look at the code in the first post, it should just fail silently, It doesn't matter if i catch ClientException which extends Exception Which extends Throwable none catch the above refresh token error nor any authentication based error.

That's the part I don't understand because as I mentioned in my previous reply, I just tried the solution I shared above and it successfully catches the exact exception you mentioned in the first post. And in the error message itself, it says that it is of the GuzzleHttp\Exception\ClientException type, so there is no reason that catch will not work on ClientException.

Correct me if I'm wrong. You're trying to catch this exception and do something with it, right? If it fails silently even if you try to catch like I showed, that may be because of other reasons, which we need to figure out further. But for the basic PHP settings, it should work.

necrogami commented 2 years ago

Thats just it, it's not failing silently, it's being caught earlier upstream. Even though i want to do something with it it's already caught, and handled and dumped out and exited the code. I'm currently running this in a laravel job queue, this exits the queue worker completely and dumping the error output out as listed above. I'm not asking it to, it just is.

fiboknacky commented 2 years ago

I see. I think the best way to test would be modifying one of our code examples to include catching of ClientException. (That's what I test in this reply) Could you please try that too?

And would it be possible to test using grpc instead of rest? I know that the code in your original post is similar to our code examples, but let's make it more aligned to remove unrelated factors.

fiboknacky commented 2 years ago

Closing due to inactivity. Feel free to reopen this if you still face the issue.