Added API Key authentication middleware to v2 API routes #9915

kevinansfield · 2018-09-26T16:18:15Z

refs #9865

add auth.authenticate.authenticateAdminApiKey middleware
- accepts signed JWT in an Authorization: Ghost [token] header
- sets req.api_key if the token is valid
add auth.authenticate.authenticateContentApiKey middleware
- accepts ?content_key= query param, sets req.api_key if it's a known Content API key
add requiresAuthorizedUserOrApiKey authorization middleware
- passes if either req.user or req.api_key exists
update authenticatePrivate middleware stack for v2 admin routes
update authenticatePublic middleware stack for v2 content routes

Split out from #9869 for easier review.

TODO:

remove oauth2 specific Bearer in Authorization header
extract rejectAdminKey and rejectContentKey into separate middlewares
add comment documenting authenticateAdminApiKey flow
add authenticateContentApiKey middleware to v2 content API routes
i18n error messages
remove rejectAdminKey and rejectContentKey middleware
rebase and update to work with session auth

allouis

Looks good! There's a few minor comments and questions 👍

core/server/services/auth/api-key/admin.js

+
+/**
+ * Remove 'Bearer' from raw authorization header and extract the JWT token.
+ * Eg. Authorization: Bearer ${JWT}


core/server/services/auth/api-key/admin.js

+};
+
+const authenticateAdminApiKey = function authenticateAdminApiKey(req, res, next) {
+    if (req.query && req.query.content_key) {


core/server/services/auth/api-key/admin.js

+            }
+
+            // unknown error
+            return next(new common.errors.InternalServerError(err));


core/server/services/auth/api-key/content.js

+} = require('../../../lib/common/errors');
+
+const authenticateContentApiKey = function authenticateContentApiKey(req, res, next) {
+    if (req.headers && req.headers.authorization) {


core/server/services/auth/api-key/index.js

@@ -0,0 +1,4 @@
+module.exports = {
+    admin: require('./admin'),


core/server/web/api/v2/admin/app.js

core/test/unit/services/auth/api-key/admin_spec.js

core/server/data/schema/fixtures/fixtures.json

+                    "description": "Blog Owner"
+                },
+                {
+                    "name": "Admin API Client",


core/server/data/schema/schema.js

+            validations: {isLength: {min: 128, max: 128}}
+        },
+        role_id: {type: 'string', maxlength: 24, nullable: true},
+        integration_id: {type: 'string', maxlength: 24, nullable: true},


core/server/services/auth/api-key/admin.js

core/server/data/migrations/versions/2.2/2-add-integrations-and-api-key-tables.js

@@ -0,0 +1,33 @@
+const commands = require('../../../schema').commands;


core/server/services/auth/api-key/content.js

core/server/services/auth/api-key/admin.js

+    return;
+};
+
+const rejectAdminApiKey = function rejectAdminApiKey(req, res, next) {


kevinansfield · 2018-10-03T12:40:02Z

@kirrg001 ready for review

core/server/services/auth/authenticate.js

+    },
+
+    // ### v2 API auth middleware
+    rejectAdminApiKey: apiKeyAuth.admin.rejectAdminApiKey,


core/server/services/auth/authenticate.js

+    // ### v2 API auth middleware
+    rejectAdminApiKey: apiKeyAuth.admin.rejectAdminApiKey,
+    authenticateAdminApiKey: apiKeyAuth.admin.authenticateAdminApiKey,
+    rejectContentApiKey: apiKeyAuth.content.rejectContentApiKey,


core/test/unit/services/auth/api-key/admin_spec.js

@@ -0,0 +1,212 @@
+const {
+    authenticateAdminApiKey,
+    rejectAdminApiKey


core/test/unit/services/auth/api-key/content_spec.js

+const common = require('../../../../../server/lib/common');
+const {
+    authenticateContentApiKey,
+    rejectContentApiKey


allouis

Looks solid - just those few things need removing

kevinansfield · 2018-10-04T08:41:49Z

Fixed up the poor global-find-delete job when removing the rejectX middleware 😬

allouis · 2018-10-04T09:00:02Z

:D looks good to me

allouis · 2018-10-08T09:51:33Z

@kevinansfield This needs a rebase
@kirrg001 Would you mind taking a look at this when you can?

core/server/services/auth/authenticate.js

@@ -100,7 +101,10 @@ const authenticate = {
        )(req, res, next);
    },

-    authenticateAdminAPI: [session.safeGetSession, session.getUser]
+    // ### v2 API auth middleware
+    authenticateAdminAPI: [session.safeGetSession, session.getUser],


core/server/services/auth/authorize.js

@@ -37,7 +37,17 @@ const authorize = {
        };
    },

-    authorizeAdminAPI: [session.ensureUser]
+    authorizeAdminAPI: [session.ensureUser],


allouis · 2018-10-08T10:51:43Z

This now breaks a test - will add a separate commit for that

refs TryGhost#9865 - add `auth.authenticate.authenticateAdminApiKey` middleware - accepts signed JWT in an `Authorization: Ghost [token]` header - sets `req.api_key` if the token is valid - add `auth.authenticate.authenticateContentApiKey` middleware - accepts `?content_key=` query param, sets `req.api_key` if it's a known Content API key - add `requiresAuthorizedUserOrApiKey` authorization middleware - passes if either `req.user` or `req.api_key` exists - update `authenticatePrivate` middleware stack for v2 admin routes - update `authenticatePublic` middleware stack for v2 content routes

no-issue This can be reimplemented once we have auth helpers for the content API tests.

core/server/web/api/v2/content/middleware.js

-    auth.authenticate.authenticateClient,
-    auth.authenticate.authenticateUser,
-    // This is a labs-enabled middleware
-    auth.authorize.requiresAuthorizedUserPublicAPI,


core/server/translations/en.json

+                "unknownAdminApiKey": "Unknown Admin API Key",
+                "unknownContentApiKey": "Unknown Content API Key",
+                "invalidApiKeyType": "Invalid API Key type",
+                "invalidJwt": "Invalid JWT",


core/server/services/auth/api-key/content.js

core/server/services/auth/api-key/admin.js

+ * - the "Audience" claim should match the requested API path
+ *   https://tools.ietf.org/html/rfc7519#section-4.1.3
+ */
+const authenticateAdminApiKey = function authenticateAdminApiKey(req, res, next) {


core/test/functional/api/v0.1/public_api_spec.js

@@ -77,7 +77,8 @@ describe('Public API', function () {
            });
    });

-    it('browse pages', function (done) {
+    // TODO: move to v2 folder, and don't skip once we have auth helper for content api tests
+    it.skip('browse pages', function (done) {


allouis · 2018-10-13T07:52:36Z

Due to splitting and not wanting to mess with kevins branch too much i've made a new PR #10005 with the content api changes and #10006 with the admin api changes ☺️

kirrg001 · 2018-10-15T08:15:38Z

Due to splitting and not wanting to mess with kevins branch too much i've made a new PR #10005 with the content api changes and #10006 with the admin api changes ☺️

👍

I think this one can be closed now 👍

kevinansfield requested review from kirrg001 and allouis September 26, 2018 16:18

allouis reviewed Sep 27, 2018

View reviewed changes

allouis self-assigned this Sep 27, 2018

kevinansfield force-pushed the api-key/authentication branch from 4d3d90f to 7d5486f Compare September 27, 2018 10:36

kirrg001 reviewed Sep 27, 2018

View reviewed changes

core/server/data/schema/fixtures/fixtures.json Outdated

"description": "Blog Owner"

},

{

"name": "Admin API Client",

This comment was marked as abuse.

Sign in to view

This comment was marked as abuse.

Sign in to view

This comment was marked as abuse.

Sign in to view

kirrg001 reviewed Sep 27, 2018

View reviewed changes

core/server/data/schema/schema.js Outdated Show resolved Hide resolved

kirrg001 reviewed Sep 27, 2018

View reviewed changes

core/server/services/auth/api-key/admin.js Outdated Show resolved Hide resolved

kirrg001 reviewed Sep 27, 2018

View reviewed changes

kirrg001 self-assigned this Sep 27, 2018

kirrg001 changed the title ~~Added API Key authentication middleware to v2 API routes~~ [WIP] Added API Key authentication middleware to v2 API routes Sep 27, 2018

kevinansfield force-pushed the api-key/authentication branch 2 times, most recently from 3577149 to ecca55f Compare September 27, 2018 13:32

kirrg001 changed the title ~~[WIP] Added API Key authentication middleware to v2 API routes~~ [HOLD] Added API Key authentication middleware to v2 API routes Sep 27, 2018

allouis mentioned this pull request Oct 2, 2018

Setup schema and models for API Key authentication #9904

Merged

kevinansfield force-pushed the api-key/authentication branch from ecca55f to cf519ab Compare October 2, 2018 15:19

kevinansfield changed the title ~~[HOLD] Added API Key authentication middleware to v2 API routes~~ [WIP] Added API Key authentication middleware to v2 API routes Oct 2, 2018

kevinansfield force-pushed the api-key/authentication branch from cf519ab to 55af833 Compare October 2, 2018 16:48

allouis reviewed Oct 3, 2018

View reviewed changes

core/server/services/auth/api-key/content.js Show resolved Hide resolved

kevinansfield force-pushed the api-key/authentication branch 2 times, most recently from 4ddbeec to cc00d67 Compare October 3, 2018 10:34

kevinansfield commented Oct 3, 2018

View reviewed changes

core/server/services/auth/api-key/admin.js Outdated

return;

};

const rejectAdminApiKey = function rejectAdminApiKey(req, res, next) {

This comment was marked as abuse.

Sign in to view

This comment was marked as abuse.

Sign in to view

This comment was marked as abuse.

Sign in to view

kevinansfield force-pushed the api-key/authentication branch 3 times, most recently from 35284af to 2cd5738 Compare October 3, 2018 12:39

kevinansfield changed the title ~~[WIP] Added API Key authentication middleware to v2 API routes~~ Added API Key authentication middleware to v2 API routes Oct 3, 2018

allouis reviewed Oct 4, 2018

View reviewed changes

core/server/services/auth/authenticate.js Outdated

},

// ### v2 API auth middleware

rejectAdminApiKey: apiKeyAuth.admin.rejectAdminApiKey,

This comment was marked as abuse.

Sign in to view

allouis reviewed Oct 4, 2018

View reviewed changes

core/test/unit/services/auth/api-key/admin_spec.js Outdated

@@ -0,0 +1,212 @@

const {

authenticateAdminApiKey,

rejectAdminApiKey

This comment was marked as abuse.

Sign in to view

allouis reviewed Oct 4, 2018

View reviewed changes

core/test/unit/services/auth/api-key/content_spec.js Outdated

const common = require('../../../../../server/lib/common');

const {

authenticateContentApiKey,

rejectContentApiKey

This comment was marked as abuse.

Sign in to view

allouis approved these changes Oct 4, 2018

View reviewed changes

kevinansfield force-pushed the api-key/authentication branch from 2cd5738 to 20392e6 Compare October 4, 2018 08:40

kevinansfield changed the title ~~Added API Key authentication middleware to v2 API routes~~ [WIP] Added API Key authentication middleware to v2 API routes Oct 8, 2018

allouis changed the title ~~[WIP] Added API Key authentication middleware to v2 API routes~~ Added API Key authentication middleware to v2 API routes Oct 8, 2018

allouis force-pushed the api-key/authentication branch from 20392e6 to 7e7fc8b Compare October 8, 2018 10:48

allouis reviewed Oct 8, 2018

View reviewed changes

core/server/services/auth/authorize.js

@@ -37,7 +37,17 @@ const authorize = {

};

},

authorizeAdminAPI: [session.ensureUser]

authorizeAdminAPI: [session.ensureUser],

This comment was marked as abuse.

Sign in to view

kevinansfield and others added 2 commits October 8, 2018 18:42

Skipping v2 content api test

6058c05

no-issue This can be reimplemented once we have auth helpers for the content API tests.

allouis added p1:priority [triage] Priority for immediate patch release priority and removed p1:priority [triage] Priority for immediate patch release labels Oct 12, 2018

kirrg001 reviewed Oct 12, 2018

View reviewed changes

core/server/services/auth/api-key/content.js Show resolved Hide resolved

kirrg001 reviewed Oct 12, 2018

View reviewed changes

This was referenced Oct 13, 2018

Added API Key auth middleware to v2 content API #10005

Merged

Added API Key auth middleware to v2 admin API #10006

Merged

allouis closed this Oct 15, 2018

		@@ -0,0 +1,33 @@
		const commands = require('../../../schema').commands;

Added API Key authentication middleware to v2 API routes #9915

Added API Key authentication middleware to v2 API routes #9915

Conversation

kevinansfield commented Sep 26, 2018 • edited by allouis

allouis left a comment

Choose a reason for hiding this comment

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

kevinansfield commented Oct 3, 2018

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

allouis left a comment

Choose a reason for hiding this comment

kevinansfield commented Oct 4, 2018

allouis commented Oct 4, 2018

allouis commented Oct 8, 2018

This comment was marked as abuse.

This comment was marked as abuse.

allouis commented Oct 8, 2018

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

allouis commented Oct 13, 2018

kirrg001 commented Oct 15, 2018

kevinansfield commented Sep 26, 2018 •

edited by allouis