🐛 Fixed error for password authentication with Bearer Token (#9227)

refs #8613, refs #9228

- if you send a request to /authentication/token with `grant_type:password` and a Bearer token, Ghost was not able to handle this combination
- because it skipped the client authentication, see https://github.com/TryGhost/Ghost/blob/1.17.0/core/server/auth/authenticate.js#L13
- and OAuth detects the `grant_type: password` and jumps in the target implementation
- the target implementation for password authentication **again** tried to fetch the client and failed, because it relied on the previous client authentication
- see https://github.com/TryGhost/Ghost/blob/1.17.0/core/server/auth/oauth.js#L40 (client.slug is undefined if client authentication is skipped)
- ^ so this is the bug
- we **can** skip client authentication for requests to the API to fetch data for example e.g. GET /posts (including Bearer)
- so when is a client authentication required?
- RFC (https://tools.ietf.org/html/rfc6749#page-38) differentiates between confidential and public clients, Ghost has no implementation for this at the moment
  - so in theory, public clients don't have to be authenticated, only if the credentials are included
- to not invent a breaking change, i decided to only make the client authentication required for password authentication
- we could change this in Ghost 2.0

I have removed the extra client request to the database for the password authentication, this is not needed. We already do client password authentication [here](https://github.com/TryGhost/Ghost/blob/1.17.0/core/server/auth/auth-strategies.js#L19);
If a Bearer token is present and you have not send a `grant_type` (which signalises OAuth to do authentication), you can skip the client authentication.

core/server/auth/authenticate.js

@@ -9,8 +9,24 @@ var passport = require('passport'),
 authenticate = {
     // ### Authenticate Client Middleware
     authenticateClient: function authenticateClient(req, res, next) {
-        // skip client authentication if bearer token is present
-        if (authUtils.getBearerAutorizationToken(req)) {
+        /**
+         * In theory, client authentication is not required for public clients, only for confidential clients.
+         * See e.g. https://tools.ietf.org/html/rfc6749#page-38. Ghost has no differentiation for this at the moment.
+         * See also See https://tools.ietf.org/html/rfc6749#section-2.1.
+         *
+         * Ghost requires client authentication for `grant_type: password`, because we have to ensure that
+         * we tie a client to a new access token. That means `grant_type: refresh_token` does not require
+         * client authentication, because binding a client already happened.
+         *
+         * To sum up:
+         *   - password authentication requires client authentication
+         *   - refreshing a token does not require client authentication
+         *   - public API requires client authentication
+         *      - as soon as you send an access token in the header or via query
+         *      - we deny public API access
+         *   - API access with a Bearer does not require client authentication
+         */
+        if (authUtils.getBearerAutorizationToken(req) && !authUtils.hasGrantType(req, 'password')) {
             return next();
         }
 

core/server/auth/oauth.js

@@ -37,29 +37,26 @@ function exchangeRefreshToken(client, refreshToken, scope, body, authInfo, done)
 }
 // We are required to pass in authInfo in order to reset spam counter for user login
 function exchangePassword(client, username, password, scope, body, authInfo, done) {
-    models.Client.findOne({slug: client.slug})
-        .then(function then(client) {
-            if (!client) {
-                return done(new errors.NoPermissionError({
-                    message: i18n.t('errors.middleware.oauth.invalidClient')
-                }), false);
-            }
+    if (!client || !client.id) {
+        return done(new errors.UnauthorizedError({
+            message: i18n.t('errors.middleware.auth.clientCredentialsNotProvided')
+        }), false);
+    }
 
-            // Validate the user
-            return models.User.check({email: username, password: password})
-                .then(function then(user) {
-                    return authUtils.createTokens({
-                        clientId: client.id,
-                        userId: user.id
-                    });
-                })
-                .then(function then(response) {
-                    spamPrevention.userLogin().reset(authInfo.ip, username + 'login');
-                    return done(null, response.access_token, response.refresh_token, {expires_in: response.expires_in});
-                });
+    // Validate the user
+    return models.User.check({email: username, password: password})
+        .then(function then(user) {
+            return authUtils.createTokens({
+                clientId: client.id,
+                userId: user.id
+            });
         })
-        .catch(function handleError(error) {
-            return done(error, false);
+        .then(function then(response) {
+            spamPrevention.userLogin().reset(authInfo.ip, username + 'login');
+            return done(null, response.access_token, response.refresh_token, {expires_in: response.expires_in});
+        })
+        .catch(function (err) {
+            done(err, false);
         });
 }
 

core/server/auth/utils.js

@@ -115,3 +115,8 @@ module.exports.getBearerAutorizationToken = function (req) {
 
     return token;
 };
+
+module.exports.hasGrantType = function hasGrantType(req, type) {
+    return req.body && req.body.hasOwnProperty('grant_type') && req.body.grant_type === type
+        || req.query && req.query.hasOwnProperty('grant_type') && req.query.grant_type === type;
+};

core/test/unit/auth/oauth_spec.js

@@ -50,14 +50,12 @@ describe('OAuth', function () {
             req.body.grant_type = 'password';
             req.body.username = 'username';
             req.body.password = 'password';
+            req.client = {
+                id: 1
+            };
             res.setHeader = {};
             res.end = {};
 
-            sandbox.stub(models.Client, 'findOne')
-                .withArgs({slug: 'test'}).returns(new Promise.resolve({
-                id: 1
-            }));
-
             sandbox.stub(models.User, 'check')
                 .withArgs({email: 'username', password: 'password'}).returns(new Promise.resolve({
                 id: 1
@@ -105,11 +103,8 @@ describe('OAuth', function () {
             res.setHeader = {};
             res.end = {};
 
-            sandbox.stub(models.Client, 'findOne')
-                .withArgs({slug: 'test'}).returns(new Promise.resolve());
-
             oAuth.generateAccessToken(req, res, function (err) {
-                err.errorType.should.eql('NoPermissionError');
+                err.errorType.should.eql('UnauthorizedError');
                 done();
             });
         });
@@ -124,14 +119,12 @@ describe('OAuth', function () {
             req.body.grant_type = 'password';
             req.body.username = 'username';
             req.body.password = 'password';
+            req.client = {
+                id: 1
+            };
             res.setHeader = {};
             res.end = {};
 
-            sandbox.stub(models.Client, 'findOne')
-                .withArgs({slug: 'test'}).returns(new Promise.resolve({
-                id: 1
-            }));
-
             sandbox.stub(models.User, 'check')
                 .withArgs({email: 'username', password: 'password'}).returns(new Promise.resolve({
                 id: 1