Skip to content

Commit

Permalink
Improved error message when attempting to use disabled v0.1 Public API (
Browse files Browse the repository at this point in the history
#10562)

no issue
- trying to use the v0.1 Public API when it was disabled led to a confusing error message, see https://forum.ghost.org/t/403-forbidden-error-on-postman-api-call/6017
- adds an explicit check for the Public API being enabled in the client authentication step and throws a useful error message if client auth is attempted when it's disabled
  • Loading branch information
kevinansfield committed Mar 5, 2019
1 parent e79fc9a commit 39edb76
Show file tree
Hide file tree
Showing 3 changed files with 37 additions and 0 deletions.
9 changes: 9 additions & 0 deletions core/server/services/auth/authenticate.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ const common = require('../../lib/common');
const session = require('./session');
const apiKeyAuth = require('./api-key');
const members = require('./members');
const labs = require('../labs');

const authenticate = {
// ### Authenticate Client Middleware
Expand Down Expand Up @@ -38,6 +39,14 @@ const authenticate = {
req.body.client_secret = req.query.client_secret;
}

if (labs.isSet('publicAPI') !== true) {

This comment has been minimized.

Copy link
@kirrg001

kirrg001 Mar 6, 2019

Contributor

@kevinansfield Just noticed. Doesn't this break v0.1 user/password authentication completely??

This comment has been minimized.

Copy link
@kirrg001

kirrg001 Mar 6, 2019

Contributor

To sum up:

  • password authentication requires client authentication

This comment has been minimized.

Copy link
@kirrg001

kirrg001 Mar 6, 2019

Contributor

If you have disabled public API (which is default) this will no longer work in v0.1.

return next(new common.errors.NoPermissionError({
message: common.i18n.t('errors.middleware.auth.publicAPIDisabled.error'),
context: common.i18n.t('errors.middleware.auth.publicAPIDisabled.context'),
help: common.i18n.t('errors.middleware.auth.forInformationRead', {url: 'https://docs.ghost.org/api/content/'})
}));
}

if (!req.body.client_id || !req.body.client_secret) {
return next(new common.errors.UnauthorizedError({
message: common.i18n.t('errors.middleware.auth.accessDenied'),
Expand Down
4 changes: 4 additions & 0 deletions core/server/translations/en.json
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,10 @@
"versionMismatch": "Client request for {clientVersion} does not match server version {serverVersion}."
},
"auth": {
"publicAPIDisabled": {
"error": "Permission error! Public API is disabled.",
"context": "Please enable the deprecated v0.1 Public API or switch to using the v2 Content API."
},
"clientAuthenticationFailed": "Client Authentication Failed",
"clientCredentialsNotProvided": "Client credentials were not provided",
"clientCredentialsNotValid": "Client credentials were not valid",
Expand Down
24 changes: 24 additions & 0 deletions core/test/unit/services/auth/authenticate_spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ const ClientPasswordStrategy = require('passport-oauth2-client-password').Strate
const auth = require('../../../../server/services/auth');
const common = require('../../../../server/lib/common');
const models = require('../../../../server/models');
const labs = require('../../../../server/services/labs');
const user = {id: 1};
const info = {scope: '*'};
const token = 'test_token';
Expand Down Expand Up @@ -203,6 +204,10 @@ describe('Auth', function () {
});

describe('Client Authentication', function () {
beforeEach(function () {
sinon.stub(labs, 'isSet').withArgs('publicAPI').returns(true);
});

it('shouldn\'t require authorized client with bearer token', function (done) {
req.headers = {};
req.headers.authorization = 'Bearer ' + token;
Expand Down Expand Up @@ -344,6 +349,25 @@ describe('Auth', function () {
done();
});

it('shouldn\'t authenticate when publicAPI is disabled', function (done) {
labs.isSet.restore();
sinon.stub(labs, 'isSet').withArgs('publicAPI').returns(false);

req.body = {};
req.body.client_id = testClient;
req.body.client_secret = testSecret;
req.headers = {};

var next = function next(err) {
err.statusCode.should.eql(403);
(err instanceof common.errors.NoPermissionError).should.eql(true);
done();
};

registerSuccessfulClientPasswordStrategy();
auth.authenticate.authenticateClient(req, res, next);
});

it('shouldn\'t authenticate when error', function (done) {
req.body = {};
req.body.client_id = testClient;
Expand Down

0 comments on commit 39edb76

Please sign in to comment.