OAuth: Fix rare problem in which request tokens were sometimes being

returned as access tokens.

actions/apioauthaccesstoken.php

@@ -84,6 +84,7 @@ class ApiOauthAccessTokenAction extends ApiOauthAction
             common_debug(var_export($req, true));
             $code = $e->getCode();
             $this->clientError($e->getMessage(), empty($code) ? 401 : $code, 'text');
+            return;
         }
 
         if (empty($atok)) {

lib/apioauthstore.php

@@ -152,8 +152,11 @@ class ApiStatusNetOAuthDataStore extends StatusNetOAuthDataStore
                 );
             }
 
-            // check to see if we have previously issued an access token for this application
+            // Check to see if we have previously issued an access token for
-            // and profile
+            // this application and profile; if so we can just return the
+            // existing access token. That seems to be the best practice. It
+            // makes it so users only have to authorize the app once per
+            // machine.
 
             $appUser = new Oauth_application_user();
 
@@ -172,6 +175,26 @@ class ApiStatusNetOAuthDataStore extends StatusNetOAuthDataStore
                      )
                 );
 
+                $at = null;
+
+                // Special case: we used to store request tokens in the
+                // Oauth_application_user record, and the access_type would
+                // always be 0 (no access) as a failsafe until an access
+                // token was issued and replaced the request token. There could
+                // be a few old Oauth_application_user records storing request
+                // tokens still around, and we don't want to accidentally
+                // return a useless request token instead of a new access
+                // token. So if we find one, we generate a new access token
+                // and update the existing Oauth_application_user record before
+                // returning the new access token. This should be rare.
+
+                if ($appUser->access_type == 0) {
+
+                    $at = $this->generateNewAccessToken($consumer, $rt, $verifier);
+                    $this->updateAppUser($appUser, $app, $at);
+
+                } else {
+
                     $at = new Token();
 
                     // fetch the full access token
@@ -186,6 +209,7 @@ class ApiStatusNetOAuthDataStore extends StatusNetOAuthDataStore
                             _('Could not issue access token.')
                         );
                     }
+                }
 
                 // Yay, we can re-issue the access token
                 return new OAuthToken($at->tok, $at->secret);
@@ -200,7 +224,40 @@ class ApiStatusNetOAuthDataStore extends StatusNetOAuthDataStore
                      )
                 );
 
-                // make a brand new access token
+                $at = $this->generateNewAccessToken($consumer, $rt, $verifier);
+                $this->newAppUser($tokenAssoc, $app, $at);
+
+                // Okay, good
+                return new OAuthToken($at->tok, $at->secret);
+            }
+
+        } else {
+
+            // the token was not authorized or not verfied
+            common_log(
+                LOG_INFO,
+                sprintf(
+                    "API OAuth - Attempt to exchange unauthorized or unverified request token %s for an access token.",
+                     $rt->tok
+                )
+            );
+            return null;
+        }
+    }
+
+    /*
+     * Generate a new access token and save it to the database
+     *
+     * @param Consumer $consumer the OAuth consumer
+     * @param Token    $rt       the authorized request token
+     * @param string   $verifier the OAuth 1.0a verifier
+     *
+     * @access private
+     *
+     * @return Token   $at       the new access token
+     */
+    private function generateNewAccessToken($consumer, $rt, $verifier)
+    {
         $at = new Token();
 
         $at->consumer_key      = $consumer->key;
@@ -226,7 +283,22 @@ class ApiStatusNetOAuthDataStore extends StatusNetOAuthDataStore
             common_debug('request token "' . $rt->tok . '" updated', __FILE__);
         }
 
-                // insert a new Oauth_application_user record w/access token
+        return $at;
+    }
+
+   /*
+    * Add a new app user (Oauth_application_user) record
+    *
+    * @param Oauth_token_association $tokenAssoc token-to-app association
+    * @param Oauth_application       $app        the OAuth client app
+    * @param Token                   $at         the access token
+    *
+    * @access private
+    *
+    * @return void
+    */
+    private function newAppUser($tokenAssoc, $app, $at)
+    {
         $appUser = new Oauth_application_user();
 
         $appUser->profile_id     = $tokenAssoc->profile_id;
@@ -239,25 +311,39 @@ class ApiStatusNetOAuthDataStore extends StatusNetOAuthDataStore
 
         if (!$result) {
             common_log_db_error($appUser, 'INSERT', __FILE__);
+
             // TRANS: Server error displayed when a database error occurs.
-                    $this->serverError(_('Database error inserting OAuth application user.'));
+            throw new Exception(
-                }
+                _('Database error inserting OAuth application user.')
-
+            );
-                // Okay, good
+        }
-                return new OAuthToken($at->tok, $at->secret);
+    }
-            }
+
-
+   /*
-        } else {
+    * Update an existing app user (Oauth_application_user) record
-
+    *
-            // the token was not authorized or not verfied
+    * @param Oauth_application_user $appUser existing app user rec
-            common_log(
+    * @param Oauth_application      $app     the OAuth client app
-                LOG_INFO,
+    * @param Token                  $at      the access token
-                sprintf(
+    *
-                    "API OAuth - Attempt to exchange unauthorized or unverified request token %s for an access token.",
+    * @access private
-                     $rt->tok
+    *
-                )
+    * @return void
+    */
+    private function updateAppUser($appUser, $app, $at)
+    {
+        $original = clone($appUser);
+        $appUser->access_type = $app->access_type;
+        $appUser->token       = $at->tok;
+
+        $result = $appUser->update($original);
+
+        if (!$result) {
+            common_log_db_error($appUser, 'UPDATE', __FILE__);
+            // TRANS: Server error displayed when a database error occurs.
+            throw new Exception(
+                _('Database error updating OAuth application user.')
             );
-            return null;
         }
     }