OAuth - better log messages

5 files changed, 90 insertions, 46 deletions

diff --git a/actions/apioauthaccesstoken.php b/actions/apioauthaccesstoken.php
index 663a7a2bb..6b36d1919 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 ea5c30c2a..eb1000e25 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 478d2dbfc..7def1aa95 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 8b0a3da17..a1c698bba 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.";
-
-                    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'
-                                                 ));
+                        "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'
+                        )
+                    );
                 } 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 f3bf0b857..6e0039bdd 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();