From 883152ff15695dac5037b742c54ff56e19c70c5b Mon Sep 17 00:00:00 2001
From: Hannah Wolfe <erisds@gmail.com>
Date: Mon, 14 Dec 2015 15:35:19 +0000
Subject: [PATCH] Improvements to client auth error logging

no issue

- If client credentials are missing, or not valid, output a clear message in the server console
- Still defaults to sending the 'access denied to url' error to the frontend
---
 core/server/middleware/auth.js                | 17 +++++++-
 .../unit/middleware/authentication_spec.js    | 40 ++++++++++++++++---
 2 files changed, 51 insertions(+), 6 deletions(-)

diff --git a/core/server/middleware/auth.js b/core/server/middleware/auth.js
index dae694cf5b..6d8b786ef9 100644
--- a/core/server/middleware/auth.js
+++ b/core/server/middleware/auth.js
@@ -56,6 +56,7 @@ function isValidOrigin(origin, client) {
         || origin === configHostname
         || configHostname === 'my-ghost-blog.com'
         || origin === url.parse(config.urlSSL ? config.urlSSL : '').hostname
+        // @TODO do this in dev mode only, once we can auto-configure the url #2240
         || (origin === 'localhost')
     )) {
         return true;
@@ -82,6 +83,11 @@ auth = {
         }
 
         if (!req.body.client_id || !req.body.client_secret) {
+            errors.logError(
+                'Client Authentication Failed',
+                'Client credentials were not provided',
+                'For information on how to fix this, please read http://api.ghost.org/docs/client-authentication'
+            );
             return errors.handleAPIError(new errors.UnauthorizedError('Access denied.'), req, res, next);
         }
 
@@ -101,6 +107,15 @@ auth = {
                 delete req.body.client_id;
                 delete req.body.client_secret;
 
+                if (!client || client.type !== 'ua') {
+                    errors.logError(
+                        'Client Authentication Failed',
+                        'Client credentials were not valid',
+                        'For information on how to fix this, please read http://api.ghost.org/docs/client-authentication'
+                    );
+                    return errors.handleAPIError(new errors.UnauthorizedError('Access denied.'), req, res, next);
+                }
+
                 if (!origin && client && client.type === 'ua') {
                     res.header('Access-Control-Allow-Origin', config.url);
                     req.client = client;
@@ -115,7 +130,7 @@ auth = {
                     error = new errors.UnauthorizedError('Access Denied from url: ' + origin + '. Please use the url configured in config.js.');
                     errors.logError(error,
                         'You have attempted to access your Ghost admin panel from a url that does not appear in config.js.',
-                        'For information on how to fix this, please visit http://support.ghost.org/config/#url.'
+                        'For information on how to fix this, please read http://support.ghost.org/config/#url.'
                     );
                     return errors.handleAPIError(error, req, res, next);
                 }
diff --git a/core/test/unit/middleware/authentication_spec.js b/core/test/unit/middleware/authentication_spec.js
index 9d8a425d56..f9cacf80a7 100644
--- a/core/test/unit/middleware/authentication_spec.js
+++ b/core/test/unit/middleware/authentication_spec.js
@@ -6,6 +6,7 @@ var _                       = require('lodash'),
     passport                = require('passport'),
     rewire                  = require('rewire'),
     config                  = require('../../../server/config'),
+    errors                  = require('../../../server/errors'),
     defaultConfig           = rewire('../../../../config.example')[process.env.NODE_ENV],
     auth                    = rewire('../../../server/middleware/auth'),
     BearerStrategy          = require('passport-http-bearer').Strategy,
@@ -18,7 +19,9 @@ var _                       = require('lodash'),
     client                  = {
         id: 2,
         type: 'ua'
-    };
+    },
+
+    sandbox = sinon.sandbox.create();
 
 should.equal(true, true);
 
@@ -86,13 +89,13 @@ function registerFaultyClientPasswordStrategy() {
 }
 
 describe('Auth', function () {
-    var res, req, next, sandbox;
+    var res, req, next, errorStub;
 
     beforeEach(function () {
-        sandbox = sinon.sandbox.create();
         req = {};
         res = {};
         next = sandbox.spy();
+        errorStub = sandbox.stub(errors, 'logError');
     });
 
     afterEach(function () {
@@ -322,7 +325,7 @@ describe('Auth', function () {
             done();
         });
 
-        it('shouldn\'t authenticate client', function (done) {
+        it('shouldn\'t authenticate without full client credentials', function (done) {
             req.body = {};
             req.body.client_id = testClient;
             res.status = {};
@@ -339,6 +342,33 @@ describe('Auth', function () {
             registerUnsuccessfulClientPasswordStrategy();
             auth.authenticateClient(req, res, next);
             next.called.should.be.false;
+            errorStub.calledTwice.should.be.true;
+            errorStub.getCall(0).args[1].should.eql('Client credentials were not provided');
+
+            done();
+        });
+
+        it('shouldn\'t authenticate invalid/unknown client', function (done) {
+            req.body = {};
+            req.body.client_id = testClient;
+            req.body.client_secret = testSecret;
+            res.status = {};
+
+            sandbox.stub(res, 'status', function (statusCode) {
+                statusCode.should.eql(401);
+                return {
+                    json: function (err) {
+                        err.errors[0].errorType.should.eql('UnauthorizedError');
+                    }
+                };
+            });
+
+            registerUnsuccessfulClientPasswordStrategy();
+            auth.authenticateClient(req, res, next);
+            next.called.should.be.false;
+            errorStub.calledTwice.should.be.true;
+            errorStub.getCall(0).args[1].should.eql('Client credentials were not valid');
+
             done();
         });
 
@@ -365,7 +395,7 @@ describe('Auth', function () {
             done();
         });
 
-        it('should authenticate client', function (done) {
+        it('should authenticate valid/known client', function (done) {
             req.body = {};
             req.body.client_id = testClient;
             req.body.client_secret = testSecret;