diff --git a/core/server/auth/authenticate.js b/core/server/auth/authenticate.js index 1f76e94400..bbe3f8b7e5 100644 --- a/core/server/auth/authenticate.js +++ b/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(); } diff --git a/core/server/auth/oauth.js b/core/server/auth/oauth.js index 0380386854..3dbae9aa62 100644 --- a/core/server/auth/oauth.js +++ b/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); }); } diff --git a/core/server/auth/utils.js b/core/server/auth/utils.js index c952f46c68..5e0d31051d 100644 --- a/core/server/auth/utils.js +++ b/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; +}; diff --git a/core/test/unit/auth/oauth_spec.js b/core/test/unit/auth/oauth_spec.js index d8e2379fcb..377209f8b8 100644 --- a/core/test/unit/auth/oauth_spec.js +++ b/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