- Check for read-only vs. read-write access to protected API resources (OAuth)

- Some cleanup

2 files changed, 74 insertions, 68 deletions

diff --git a/actions/apistatusesupdate.php b/actions/apistatusesupdate.php
index 31c9b20ce..bf367e1e1 100644
--- a/actions/apistatusesupdate.php
+++ b/actions/apistatusesupdate.php
@@ -28,7 +28,7 @@
  * @author    Mike Cochrane <mikec@mikenz.geek.nz>
  * @author    Robin Millette <robin@millette.info>
  * @author    Zach Copley <zach@status.net>
- * @copyright 2009 StatusNet, Inc.
+ * @copyright 2009-2010 StatusNet, Inc.
  * @license   http://www.fsf.org/licensing/licenses/agpl-3.0.html GNU Affero General Public License version 3.0
  * @link      http://status.net/
  */
@@ -79,7 +79,6 @@ class ApiStatusesUpdateAction extends ApiAuthAction
     {
         parent::prepare($args);
 
-        $this->user   = $this->auth_user;
         $this->status = $this->trimmed('status');
         $this->source = $this->trimmed('source');
         $this->lat    = $this->trimmed('lat');
@@ -145,7 +144,7 @@ class ApiStatusesUpdateAction extends ApiAuthAction
             return;
         }
 
-        if (empty($this->user)) {
+        if (empty($this->auth_user)) {
             $this->clientError(_('No such user.'), 404, $this->format);
             return;
         }
@@ -172,7 +171,7 @@ class ApiStatusesUpdateAction extends ApiAuthAction
         // Check for commands
 
         $inter = new CommandInterpreter();
-        $cmd = $inter->handle_command($this->user, $status_shortened);
+        $cmd = $inter->handle_command($this->auth_user, $status_shortened);
 
         if ($cmd) {
 
@@ -184,7 +183,7 @@ class ApiStatusesUpdateAction extends ApiAuthAction
             // And, it returns your last status whether the cmd was successful
             // or not!
 
-            $this->notice = $this->user->getCurrentNotice();
+            $this->notice = $this->auth_user->getCurrentNotice();
 
         } else {
 
@@ -211,7 +210,7 @@ class ApiStatusesUpdateAction extends ApiAuthAction
             $upload = null;
 
             try {
-                $upload = MediaFile::fromUpload('media', $this->user);
+                $upload = MediaFile::fromUpload('media', $this->auth_user);
             } catch (ClientException $ce) {
                 $this->clientError($ce->getMessage());
                 return;
@@ -234,19 +233,19 @@ class ApiStatusesUpdateAction extends ApiAuthAction
 
             $options = array('reply_to' => $reply_to);
 
-            if ($this->user->shareLocation()) {
+            if ($this->auth_user->shareLocation()) {
 
                 $locOptions = Notice::locationOptions($this->lat,
                                                       $this->lon,
                                                       null,
                                                       null,
-                                                      $this->user->getProfile());
+                                                      $this->auth_user->getProfile());
 
                 $options = array_merge($options, $locOptions);
             }
 
             $this->notice =
-              Notice::saveNew($this->user->id,
+              Notice::saveNew($this->auth_user->id,
                               $content,
                               $this->source,
                               $options);
@@ -255,7 +254,6 @@ class ApiStatusesUpdateAction extends ApiAuthAction
                 $upload->attachToNotice($this->notice);
             }
 
-
         }
 
         $this->showNotice();
diff --git a/lib/apiauth.php b/lib/apiauth.php
index 37070d212..ad9651ff2 100644
--- a/lib/apiauth.php
+++ b/lib/apiauth.php
@@ -29,7 +29,7 @@
  * @author    mEDI <medi@milaro.net>
  * @author    Sarven Capadisli <csarven@status.net>
  * @author    Zach Copley <zach@status.net>
- * @copyright 2009 StatusNet, Inc.
+ * @copyright 2009-2010 StatusNet, Inc.
  * @license   http://www.fsf.org/licensing/licenses/agpl-3.0.html GNU Affero General Public License version 3.0
  * @link      http://status.net/
  */
@@ -53,9 +53,11 @@ require_once INSTALLDIR . '/lib/apioauth.php';
 
 class ApiAuthAction extends ApiAction
 {
-    var $access_token;
-    var $oauth_access_type;
-    var $oauth_source;
+    var $auth_user_nickname = null;
+    var $auth_user_password = null;
+    var $access_token       = null;
+    var $oauth_source       = null;
+    var $auth_user          = null;
 
     /**
      * Take arguments for running, and output basic auth header if needed
@@ -70,18 +72,28 @@ class ApiAuthAction extends ApiAction
     {
         parent::prepare($args);
 
-        if ($this->requiresAuth()) {
+        $this->consumer_key = $this->arg('oauth_consumer_key');
+        $this->access_token = $this->arg('oauth_token');
 
-            $this->consumer_key = $this->arg('oauth_consumer_key');
-            $this->access_token = $this->arg('oauth_token');
+        // NOTE: $this->auth_user has to get set in prepare(), not handle(),
+        // because subclasses do stuff with it in their prepares.
 
+        if ($this->requiresAuth()) {
             if (!empty($this->access_token)) {
                 $this->checkOAuthRequest();
             } else {
                 $this->checkBasicAuthUser();
-                // By default, all basic auth users have read and write access
+            }
+
+            // Reject API calls with the wrong access level
 
-                $this->access = self::READ_WRITE;
+            if ($this->isReadOnly($args) == false) {
+                if ($this->access != self::READ_WRITE) {
+                    $msg = 'API resource requires read-write access, ' .
+                      'but you only have read access.';
+                    $this->clientError($msg, 401, $this->format);
+                    exit();
+                }
             }
         }
 
@@ -95,8 +107,6 @@ class ApiAuthAction extends ApiAction
 
     function checkOAuthRequest()
     {
-        common_debug("We have an OAuth request.");
-
         $datastore   = new ApiStatusNetOAuthDataStore();
         $server      = new OAuthServer($datastore);
         $hmac_method = new OAuthSignatureMethod_HMAC_SHA1();
@@ -114,9 +124,10 @@ class ApiAuthAction extends ApiAction
 
             if (empty($app)) {
 
-                // this should really not happen
-                common_log(LOG_WARN,
-                           "Couldn't find the OAuth app for consumer key: $this->consumer_key");
+                // this should probably not happen
+                common_log(LOG_WARNING,
+                           'Couldn\'t find the OAuth app for consumer key: ' .
+                           $this->consumer_key);
 
                 throw new OAuthException('No application for that consumer key.');
             }
@@ -128,20 +139,18 @@ class ApiAuthAction extends ApiAction
             $appUser = Oauth_application_user::staticGet('token',
                                                          $this->access_token);
 
-            // XXX: check that app->id and appUser->application_id and consumer all
+            // XXX: Check that app->id and appUser->application_id and consumer all
             // match?
 
             if (!empty($appUser)) {
 
-                // read or read-write
-                $this->oauth_access_type = $appUser->access_type;
-
                 // If access_type == 0 we have either a request token
                 // or a bad / revoked access token
 
-                if ($this->oauth_access_type != 0) {
+                if ($appUser->access_type != 0) {
+
+                    // Set the access level for the api call
 
-                    // Set the read or read-write access for the api call
                     $this->access = ($appUser->access_type & Oauth_application::$writeAccess)
                       ? self::READ_WRITE : self::READ_ONLY;
 
@@ -151,38 +160,34 @@ class ApiAuthAction extends ApiAction
                     }
 
                     $msg = "API OAuth authentication for user '%s' (id: %d) on behalf of " .
-                      "application '%s' (id: %d).";
+                      "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));
+                                                 $app->id,
+                                                 ($this->access = self::READ_WRITE) ?
+                                                 'read-write' : 'read-only'
+                                                 ));
                     return true;
                 } else {
                     throw new OAuthException('Bad access token.');
                 }
             } else {
 
-                // also should not happen
+                // Also should not happen
+
                 throw new OAuthException('No user for that token.');
-        }
+            }
 
         } catch (OAuthException $e) {
-            common_log(LOG_WARN, 'API OAuthException - ' . $e->getMessage());
-            common_debug(var_export($req, true));
-            $this->showOAuthError($e->getMessage());
-            exit();
+            common_log(LOG_WARNING, 'API OAuthException - ' . $e->getMessage());
+            $this->showAuthError();
+            exit;
         }
     }
 
-    function showOAuthError($msg)
-    {
-        header('HTTP/1.1 401 Unauthorized');
-        header('Content-Type: text/html; charset=utf-8');
-        print $msg . "\n";
-    }
-
     /**
      * Does this API resource require authentication?
      *
@@ -207,39 +212,44 @@ class ApiAuthAction extends ApiAction
 
         $realm = common_config('site', 'name') . ' API';
 
-        if (!isset($this->auth_user)) {
+        if (!isset($this->auth_user_nickname)) {
             header('WWW-Authenticate: Basic realm="' . $realm . '"');
 
             // show error if the user clicks 'cancel'
 
-            $this->showBasicAuthError();
+            $this->showAuthError();
             exit;
 
         } else {
-            $nickname = $this->auth_user;
-            $password = $this->auth_pw;
-            $user = common_check_user($nickname, $password);
+
+            $user = common_check_user($this->auth_user_nickname,
+                                      $this->auth_user_password);
+
             if (Event::handle('StartSetApiUser', array(&$user))) {
                 $this->auth_user = $user;
                 Event::handle('EndSetApiUser', array($user));
             }
 
+            // By default, basic auth users have rw access
+
+            $this->access = self::READ_WRITE;
+
             if (empty($this->auth_user)) {
 
                 // basic authentication failed
 
                 list($proxy, $ip) = common_client_ip();
+
                 common_log(
                     LOG_WARNING,
                     'Failed API auth attempt, nickname = ' .
                     "$nickname, proxy = $proxy, ip = $ip."
                 );
-                $this->showBasicAuthError();
+
+                $this->showAuthError();
                 exit;
             }
         }
-
-        return true;
     }
 
     /**
@@ -253,32 +263,30 @@ class ApiAuthAction extends ApiAction
     {
         if (isset($_SERVER['AUTHORIZATION'])
             || isset($_SERVER['HTTP_AUTHORIZATION'])
-        ) {
-                $authorization_header = isset($_SERVER['HTTP_AUTHORIZATION'])
-                ? $_SERVER['HTTP_AUTHORIZATION'] : $_SERVER['AUTHORIZATION'];
+            ) {
+            $authorization_header = isset($_SERVER['HTTP_AUTHORIZATION'])
+              ? $_SERVER['HTTP_AUTHORIZATION'] : $_SERVER['AUTHORIZATION'];
         }
 
         if (isset($_SERVER['PHP_AUTH_USER'])) {
-            $this->auth_user = $_SERVER['PHP_AUTH_USER'];
-            $this->auth_pw = $_SERVER['PHP_AUTH_PW'];
+            $this->auth_user_nickname = $_SERVER['PHP_AUTH_USER'];
+            $this->auth_user_password = $_SERVER['PHP_AUTH_PW'];
         } elseif (isset($authorization_header)
             && strstr(substr($authorization_header, 0, 5), 'Basic')) {
 
-            // decode the HTTP_AUTHORIZATION header on php-cgi server self
+            // Decode the HTTP_AUTHORIZATION header on php-cgi server self
             // on fcgid server the header name is AUTHORIZATION
 
             $auth_hash = base64_decode(substr($authorization_header, 6));
-            list($this->auth_user, $this->auth_pw) = explode(':', $auth_hash);
+            list($this->auth_user_nickname,
+                 $this->auth_user_password) = explode(':', $auth_hash);
 
-            // set all to null on a empty basic auth request
+            // Set all to null on a empty basic auth request
 
-            if ($this->auth_user == "") {
-                $this->auth_user = null;
-                $this->auth_pw = null;
+            if (empty($this->auth_user_nickname)) {
+                $this->auth_user_nickname = null;
+                $this->auth_password = null;
             }
-        } else {
-            $this->auth_user = null;
-            $this->auth_pw = null;
         }
     }
 
@@ -289,7 +297,7 @@ class ApiAuthAction extends ApiAction
      * @return void
      */
 
-    function showBasicAuthError()
+    function showAuthError()
     {
         header('HTTP/1.1 401 Unauthorized');
         $msg = 'Could not authenticate you.';