From 5866493cae0f2877398e5bedfb4261dfefbdf7dd Mon Sep 17 00:00:00 2001 From: Zach Copley Date: Tue, 19 Oct 2010 12:07:59 -0700 Subject: [PATCH] OAuth - better log messages --- actions/apioauthaccesstoken.php | 26 +++++++------ actions/apioauthauthorize.php | 32 +++++++++++----- actions/apioauthrequesttoken.php | 2 +- lib/apiauth.php | 65 +++++++++++++++++++++----------- lib/apioauthstore.php | 9 ++++- 5 files changed, 89 insertions(+), 45 deletions(-) diff --git a/actions/apioauthaccesstoken.php b/actions/apioauthaccesstoken.php index 663a7a2bb6..6b36d1919e 100644 --- a/actions/apioauthaccesstoken.php +++ b/actions/apioauthaccesstoken.php @@ -67,7 +67,7 @@ class ApiOauthAccessTokenAction extends ApiOauthAction $server->add_signature_method($hmac_method); - $atok = null; + $atok = $app = null; // XXX: Insist that oauth_token and oauth_verifier be populated? // Spec doesn't say they MUST be. @@ -78,7 +78,7 @@ class ApiOauthAccessTokenAction extends ApiOauthAction $this->reqToken = $req->get_parameter('oauth_token'); $this->verifier = $req->get_parameter('oauth_verifier'); - + $app = $datastore->getAppByRequestToken($this->reqToken); $atok = $server->fetch_access_token($req); } catch (OAuthException $e) { @@ -92,22 +92,26 @@ class ApiOauthAccessTokenAction extends ApiOauthAction // Token exchange failed -- log it - list($proxy, $ip) = common_client_ip(); - $msg = sprintf( - 'API OAuth - Failure exchanging request token for access token, ' - . 'request token = %s, verifier = %s, IP = %s, proxy = %s', + 'API OAuth - Failure exchanging OAuth request token for access token, ' + . 'request token = %s, verifier = %s', $this->reqToken, - $this->verifier, - $ip, - $proxy + $this->verifier ); - common_log(LOG_WARNING, $msg); - + common_log(LOG_WARNIGN, $msg); $this->clientError(_("Invalid request token or verifier.", 400, 'text')); } else { + common_log( + LOG_INFO, + sprintf( + "Issued now access token '%s' for application %d (%s).", + $atok->key, + $app->id, + $app->name + ) + ); $this->showAccessToken($atok); } } diff --git a/actions/apioauthauthorize.php b/actions/apioauthauthorize.php index ea5c30c2ac..eb1000e252 100644 --- a/actions/apioauthauthorize.php +++ b/actions/apioauthauthorize.php @@ -113,14 +113,12 @@ class ApiOauthAuthorizeAction extends Action $this->reqToken = $this->store->getTokenByKey($this->oauthTokenParam); if (empty($this->reqToken)) { - $this->serverError( - _('Invalid request token.') - ); + $this->clientError(_('Invalid request token.')); } else { // Check to make sure we haven't already authorized the token if ($this->reqToken->state != 0) { - $this->clientError("Invalid request token."); + $this->clientError(_("Invalid request token.")); } } } @@ -240,15 +238,31 @@ class ApiOauthAuthorizeAction extends Action // Redirect the user to the provided OAuth callback common_redirect($targetUrl, 303); - } else { + } elseif ($this->app->type == 2) { + + // Strangely, a web application seems to want to do the OOB + // workflow. Because no callback was specified anywhere. common_log( - LOG_INFO, - "No oauth_callback parameter provided for application ID " - . $this->app->id - . " when authorizing request token." + LOG_WARNING, + sprintf( + "API OAuth - No callback provided for OAuth web client ID %s (%s) " + . "during authorization step. Falling back to OOB workflow.", + $this->app->id, + $this->app->name + ) ); } + common_log( + LOG_INFO, + sprintf( + "The request token '%s' for OAuth application %s (%s) has been authorized.", + $this->oauthTokenParam, + $this->app->id, + $this->app->name + ) + ); + // Otherwise, inform the user that the rt was authorized $this->showAuthorized(); diff --git a/actions/apioauthrequesttoken.php b/actions/apioauthrequesttoken.php index 478d2dbfc4..7def1aa95c 100644 --- a/actions/apioauthrequesttoken.php +++ b/actions/apioauthrequesttoken.php @@ -146,7 +146,7 @@ class ApiOauthRequestTokenAction extends ApiOauthAction function verifyCallback($callback) { if ($callback == "oob") { - common_debug("OAuth request token requested for out of bounds client."); + common_debug("OAuth request token requested for out of band client."); // XXX: Should we throw an error if a client is registered as a // web application but requests the pin based workflow? For now I'm diff --git a/lib/apiauth.php b/lib/apiauth.php index 8b0a3da17b..a1c698bba9 100644 --- a/lib/apiauth.php +++ b/lib/apiauth.php @@ -168,9 +168,11 @@ class ApiAuthAction extends ApiAction $app = Oauth_application::getByConsumerKey($consumer); if (empty($app)) { - common_log(LOG_WARNING, - 'Couldn\'t find the OAuth app for consumer key: ' . - $consumer); + common_log( + LOG_WARNING, + 'API OAuth - Couldn\'t find the OAuth app for consumer key: ' . + $consumer + ); // TRANS: OAuth exception thrown when no application is found for a given consumer key. throw new OAuthException(_('No application for that consumer key.')); } @@ -197,16 +199,19 @@ class ApiAuthAction extends ApiAction } $msg = "API OAuth authentication for user '%s' (id: %d) on behalf of " . - "application '%s' (id: %d) with %s access."; + "application '%s' (id: %d) with %s access."; - common_log(LOG_INFO, sprintf($msg, - $this->auth_user->nickname, - $this->auth_user->id, - $app->name, - $app->id, - ($this->access = self::READ_WRITE) ? - 'read-write' : 'read-only' - )); + common_log( + LOG_INFO, + sprintf( + $msg, + $this->auth_user->nickname, + $this->auth_user->id, + $app->name, + $app->id, + ($this->access = self::READ_WRITE) ? 'read-write' : 'read-only' + ) + ); } else { // TRANS: OAuth exception given when an incorrect access token was given for a user. throw new OAuthException(_('Bad access token.')); @@ -218,6 +223,7 @@ class ApiAuthAction extends ApiAction } } catch (OAuthException $e) { + $this->logAuthFailure($e->getMessage()); common_log(LOG_WARNING, 'API OAuthException - ' . $e->getMessage()); $this->clientError($e->getMessage(), 401, $this->format); exit; @@ -276,16 +282,11 @@ class ApiAuthAction extends ApiAction $this->access = self::READ_WRITE; if (empty($this->auth_user) && ($required || isset($_SERVER['PHP_AUTH_USER']))) { - - // basic authentication failed - list($proxy, $ip) = common_client_ip(); - - $msg = sprintf( 'Failed API auth attempt, nickname = %1$s, ' . - 'proxy = %2$s, ip = %3$s', - $this->auth_user_nickname, - $proxy, - $ip); - common_log(LOG_WARNING, $msg); + $msg = sprintf( + "basic auth nickname = %s", + $this->auth_user_nickname + ); + $this->logAuthFailure($msg); // TRANS: Client error thrown when authentication fails. $this->clientError(_("Could not authenticate you."), 401, $this->format); exit; @@ -332,4 +333,24 @@ class ApiAuthAction extends ApiAction } } } + + /** + * Log an API authentication failer. Collect the proxy and IP + * and log them + * + * @param string $logMsg additional log message + */ + + function logAuthFailure($logMsg) + { + list($proxy, $ip) = common_client_ip(); + + $msg = sprintf( + 'API auth failure (proxy = %1$s, ip = %2$s) - ', + $proxy, + $ip + ); + + common_log(LOG_WARNING, $msg . $logMsg); + } } diff --git a/lib/apioauthstore.php b/lib/apioauthstore.php index f3bf0b8576..6e0039bdd9 100644 --- a/lib/apioauthstore.php +++ b/lib/apioauthstore.php @@ -74,8 +74,13 @@ class ApiStatusNetOAuthDataStore extends StatusNetOAuthDataStore function new_access_token($token, $consumer, $verifier) { common_debug( - 'new_access_token("' . $token->key . '","' . $consumer->key. '","' . $verifier . '")', - __FILE__ + sprintf( + "%s - New access token from request token %s, consumer %s and verifier %s ", + __FILE__, + $token, + $consumer, + $verifier + ) ); $rt = new Token();