Partial fix Adding custom API authentication methods is currently very messy

[Jake B.]

Affected version

2.1.x

A few issues I've noticed with this process:

First:

In \XF\Api\App::start when $user is returned as falsey by using the app_api_validate_request event listener it does the following:

return $this->getApiErrorResponse(\XF::phrase($error), $code);

This is resulting in something along the lines of:

{
  "errors": [
    {
      "code": "Some error message",
      "message": "Some error message",
      "params": []
    }
  ]
}

There is currently no way to return any sort of custom response with additional details (or an actual error code (invalid_token, invalid_client, token_revoked, etc) that can be programmatically used to determine the specific error rather than relying on just the HTTP response code - which could potentially mean several different things). Ideally there should be some way for me to pass the entire response object so it could be tailored to standards in place for things like OAuth

Second, there isn't a clean way to specify the allowed scopes without creating a faux API key object with the following:

/** @var ApiKey $fauxApiKey */
$fauxApiKey = \XF::app()->em()->create('XF:ApiKey');
$fauxApiKey->bulkSet([
    'scopes' => $allowedScopes,
]);

\XF::setApiKey($fauxApiKey);

Would be much cleaner if I could simply do something along the lines of:

\XF::setApiScopes($allowedScopes);

 

[Jake B.]

Here's a rough summary of my recommended changes so far:

diff --git a/src/XF.php b/src/XF.php
index d745b5cf5..2e1d14e58 100644
--- a/src/XF.php
+++ b/src/XF.php
@@ -53,6 +53,12 @@ class XF
     */
    protected static $apiKey = null;
 
+   /** @var string[] */
+   protected static $apiScopes = [];
+
+   /** @var bool */
+   protected static $apiSuperUser = false;
+
    /**
     * @var bool
     */
@@ -517,11 +523,45 @@ class XF
        return self::$apiKey;
    }
 
+    /**
+     * @return string[]
+     */
+   public static function apiScopes()
+    {
+        return self::$apiScopes;
+    }
+
+    public static function apiHasScope($scope)
+    {
+        return \XF::apiSuperUser() || in_array(self::apiScopes(), $scope);
+    }
+
+    /**
+     * @return bool
+     */
+    public static function apiSuperUser()
+    {
+        return self::$apiSuperUser;
+    }
+
    public static function setApiKey(\XF\Entity\ApiKey $key = null)
    {
        self::$apiKey = $key;
+
+       self::setApiScopes($key->scopes);
+       self::setApiSuperUser($key->is_super_user);
    }
 
+   public static function setApiScopes(array $scopes = [])
+    {
+        self::$apiScopes = $scopes;
+    }
+
+    public static function setApiSuperUser($apiSuperUser = true)
+    {
+        self::$apiSuperUser = $apiSuperUser;
+    }
+
    /**
     * True if the API has been set to bypass permissions for the current request.
     * This is only possible if a super user key is being used.
diff --git a/src/XF/Api/App.php b/src/XF/Api/App.php
index 2493c341e..e93b079ff 100644
--- a/src/XF/Api/App.php
+++ b/src/XF/Api/App.php
@@ -76,9 +76,12 @@ class App extends \XF\App
 
        $this->fire('app_api_start_begin', [$this]);
 
-       $user = $this->validateUserFromApiHeader($error, $code);
+       $user = $this->validateUserFromApiHeader($error, $code, $rawError);
        if (!$user)
        {
+           if ($rawError) {
+               return $this->getRawApiErrorResponse($rawError, $code);
+            }
            return $this->getApiErrorResponse(\XF::phrase($error), $code);
        }
        \XF::setVisitor($user);
@@ -87,7 +90,7 @@ class App extends \XF\App
        $language->setTimeZone($user->timezone);
        \XF::setLanguage($language);
 
-       if ($this->request()->filter('api_bypass_permissions', 'bool') && \XF::apiKey()->is_super_user)
+       if ($this->request()->filter('api_bypass_permissions', 'bool') && \XF::apiSuperUser())
        {
            \XF::setApiBypassPermissions(true);
        }
@@ -133,7 +136,7 @@ class App extends \XF\App
        return $response;
    }
 
-   protected function validateUserFromApiHeader(&$error = '', &$code = null)
+   protected function validateUserFromApiHeader(&$error = '', &$code = null, &$rawError = null)
    {
        $request = $this->request();
 
diff --git a/src/XF/Api/Controller/AbstractController.php b/src/XF/Api/Controller/AbstractController.php
index a3d34d426..acdc67690 100644
--- a/src/XF/Api/Controller/AbstractController.php
+++ b/src/XF/Api/Controller/AbstractController.php
@@ -76,9 +76,7 @@ abstract class AbstractController extends Controller
 
    public function assertApiScope($scope)
    {
-       $key = \XF::apiKey();
-
-       if (!$key->hasScope($scope))
+       if (!\XF::apiHasScope($scope))
        {
            throw $this->exception(
                $this->error(\XF::phrase('api_error.missing_scope', ['scope' => $scope]), 403)
@@ -192,8 +190,7 @@ abstract class AbstractController extends Controller
 
    public function assertSuperUserKey()
    {
-       $key = \XF::apiKey();
-       if ($key->key_type !== 'super')
+       if (\XF::apiSuperUser())
        {
            throw $this->exception($this->noPermission());
        }
diff --git a/src/XF/Api/Controller/Index.php b/src/XF/Api/Controller/Index.php
index d67f3096e..c1c306db4 100644
--- a/src/XF/Api/Controller/Index.php
+++ b/src/XF/Api/Controller/Index.php
@@ -32,6 +32,7 @@ class Index extends AbstractController
            'site_title' => $options->boardTitle,
            'base_url' => $options->boardUrl,
            'api_url' => $this->buildLink('canonical:index'),
+           /* All of this needs handled in a way that works without an ApiKey object */
            'key' => [
                'type' => $key->key_type,
                'user_id' => $key->key_type == 'user' ? $key->user_id : null,
diff --git a/src/XF/Api/Controller/Me.php b/src/XF/Api/Controller/Me.php
index 8c579c612..a4b4edb26 100644
--- a/src/XF/Api/Controller/Me.php
+++ b/src/XF/Api/Controller/Me.php
@@ -26,7 +26,7 @@ class Me extends AbstractController
     */
    public function actionGet()
    {
-       $verbosity = \XF::apiKey()->hasScope('user:read') ? Entity::VERBOSITY_VERBOSE : Entity::VERBOSITY_QUIET;
+       $verbosity = \XF::apiHasScope('user:read') ? Entity::VERBOSITY_VERBOSE : Entity::VERBOSITY_QUIET;
 
        return $this->apiResult([
            'me' => \XF::visitor()->toApiResult($verbosity)

 

[Mike]

I am making some tweaks here. However, I don't necessarily agree with a few of the characterizations here so we're not making any fundamental adjustments.

There is currently no way to return any sort of custom response with additional details (or an actual error code (invalid_token, invalid_client, token_revoked, etc) that can be programmatically used to determine the specific error rather than relying on just the HTTP response code - which could potentially mean several different things). Ideally there should be some way for me to pass the entire response object so it could be tailored to standards in place for things like OAuth

The tweak being made in the next 2.2 release is that you can now also return a XF\Http\Response object and that will be output as is. This is pretty low level, so it should only be used if you really need it.

You also comment that we can't return an actual error code like invalid_token, but in the context of the XF error response system, you can. It's derived from the phrase name, so it's not user changable, unlike the message. As an example, we have this code:

$error = 'api_error.api_key_not_found';
$code = 401;
return false;

Which triggers this output:

{
    "errors": [
        {
            "code": "api_key_not_found",
            "message": "API key provided in request was not found.",
            "params": []
        }
    ]
}

You can see the "code" part is from that (with the api_error phrase group stripped off). So if you did a phrase like api_error.invalid_token you would get invalid_token as the code. (This applies to phrases in errors throughout the API; the api_error part is a special case phrase group removal.)

Second, there isn't a clean way to specify the allowed scopes without creating a faux API key object with the following:

I mean, that's exactly the approach we'd expect to be taken at this point. We take this approach in the core via XF\Repository\Api::getFallbackApiKey. It's a bit verbose, but there shouldn't really be any reasons not to do this. (This is also how guests have worked throughout XF2.)

In the future we could consider moving to a slightly more abstracted API key object, though it'd probably function quite similarly to now. I wouldn't foresee us adding various static methods to the XF object to control things like scopes, etc.

 

[Jake B.]

You also comment that we can't return an actual error code like invalid_token, but in the context of the XF error response system, you can. It's derived from the phrase name, so it's not user changable, unlike the message. As an example, we have this code:

Yeah I ended up finding a couple days after I made this thread that I could return just the phrase ID and it'd use that as the error code, and then render the phrase as the message, I was returning a phrase object which resulted in the issue I had mentioned