Setup schema and models for API Key authentication #9904

kevinansfield · 2018-09-25T10:54:25Z

schema migration
- adds integrations and api_keys tables
- inserts Admin API Client role and permissions
adds Integration model
adds ApiKey model
- creates default secret if not given
- hardcodes associated role based on key type
  - admin = Admin API Client
  - content = no role

Split out from #9869 to make review easier.

kirrg001 · 2018-09-25T11:40:50Z

@kevinansfield When do you need a review on this? Let me know if it's blocking you.

kevinansfield · 2018-09-25T12:14:05Z

@kirrg001 as soon as you have a moment would be 👍 I'm splitting more PRs out from #9869 but they all build on top of this one

core/server/data/schema/schema.js

kirrg001 · 2018-09-25T12:50:33Z

I am getting an error when running the migration

TypeError: Cannot read property 'name' of undefined
    at Object.addApiKeyRole (/Users/kirrg001/dev/ghost/ghost-workspace2/core/server/data/migrations/versions/2.2/2-add-integrations-and-api-key-tables.js:39:50)
    at _private.addIntegrationsTable.then.then (/Users/kirrg001/dev/ghost/ghost-workspace2/core/server/data/migrations/versions/2.2/2-add-integrations-and-api-key-tables.js:79:30)

🤷🏻‍♀️

kevinansfield · 2018-09-25T13:03:32Z

Oops, I pulled the role fixtures out for the tests but missed the relevant schema fixture. Should be fixed now 😅

kirrg001

I did a first review round. Left some first comments. I need to continue with my tasks, will try to review again later :)

ES6: I know you are used to liberal let 😁but server uses constantly const. If we do two different stylings, it's confusing.

core/test/unit/models/api-key_spec.js

+            };
+
+            return models.ApiKey.add(attrs).then((api_key) => {
+                return models.ApiKey.where({id: api_key.id}).fetch({withRelated: ['role']})


core/test/unit/models/api-key_spec.js

+                        api_key.get('type').should.eql('admin');
+
+                        // defaults
+                        api_key.related('role').should.exist;


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

+    });
+};
+
+module.exports.up = function setupIntegrations(options) {


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

+            logging.info(message);
+        }
+
+        logging.warn(`(${result.done}/${result.expected}) ${message}`);


core/server/models/api-key.js

+    add(data, unfilteredOptions) {
+        const options = ApiKey.filterOptions(unfilteredOptions, 'add');
+
+        return ghostBookshelf.Model.add.call(this, data, options);


core/server/models/api-key.js

kevinansfield · 2018-09-25T17:08:15Z

Updated, and ready for review again 😄

allouis

Looks good, few minor tiny comments

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

core/server/models/api-key.js

+        // - content key = no role
+        if (this.hasChanged('type')) {
+            if (this.get('type') === 'admin') {
+                tasks.setAdminRole = Role.findOne({name: 'Admin API Client'}, {columns: ['id']})


core/server/models/api-key.js

core/server/models/integration.js

+    api_keys: function apiKeys() {
+        return this.hasMany('ApiKey');
+    }
+}, {


core/server/data/schema/schema.js

kevinansfield · 2018-09-27T10:28:32Z

Rebased and resolved conflicts

core/server/data/migrations/versions/2.2/3-insert-admin-api-client-role.js

+                .invokeThen('destroy', options)
+                .then(() => role.destroy())
+                .then(() => {
+                    logging.info(message);


core/server/models/role.js

@@ -10,6 +10,12 @@ Role = ghostBookshelf.Model.extend({

    tableName: 'roles',

+    relationships: ['permissions'],


allouis

1 comment and 1 question 👍

core/test/unit/models/role_spec.js

+const should = require('should');
+
+describe('Unit: models/role', function () {
+    before(function () {


core/test/unit/models/role_spec.js

+        models.init();
+    });
+
+    before(testUtils.teardown);


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

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


core/server/data/migrations/versions/2.2/3-insert-admin-api-client-role.js

+        from: relations.from,
+        to: relations.to,
+        entries: {
+            'Admin API Client': relations.entries['Admin API Client']


kevinansfield · 2018-10-02T11:51:27Z

@kirrg001 do you have any insight on the integration test that has started to fail here? https://travis-ci.org/TryGhost/Ghost/jobs/436072806#L4875

It seems as though using bookshelf-relations is changing the toJSON output after destroy is called. I don't know have enough context to know if that's fine and the test should be fixed or if I need to find a way to get the output to match what the test is expecting.

core/server/data/schema/schema.js

core/test/utils/fixtures/data-generator.js

+        {
+            id: ObjectId.generate(),
+            type: 'admin'
+            // integration_id: DataGenerator.Content.integrations[0].id


kirrg001 · 2018-10-02T12:04:17Z

@kirrg001 do you have any insight on the integration test that has started to fail here? https://travis-ci.org/TryGhost/Ghost/jobs/436072806#L4875
It seems as though using bookshelf-relations is changing the toJSON output after destroy is called. I don't know have enough context to know if that's fine and the test should be fixed or if I need to find a way to get the output to match what the test is expecting.

Bookshelf relation does not change the JSON output, it modifies the model.
e.g. if you edit relations e.g. tags, you expect to get the relation endresult back. I agree, for deleting it's maybe a little unexpected to receive role.permissions = [], but it proofs that no more relations exist. The destroy API endpoints return null anyway. Soo i would ignore this for now, because it's an internal behaviour?

kevinansfield · 2018-10-02T12:12:13Z

Soo i would ignore this for now, because it's an internal behaviour?

👍 ok, I'll update the test to match the internal behaviour we expect in this case. Thanks!

kirrg001 · 2018-10-02T14:02:11Z

@kevinansfield Pls let me know when the PR is ready for final test

refs TryGhost#9865 - schema migrations - adds `integrations` and `api_keys` tables - inserts `integration` and `api_key` permissions and Administrator role relationships - inserts `Admin Integration` role and permissions - adds `Integration` model - adds `ApiKey` model - creates default secret if not given - hardcodes associated role based on key type - `admin` = `Admin API Client` - `content` = no role - updates `Role` model to use bookshelf-relationships for auto cleanup of relationships on destroy

kevinansfield · 2018-10-02T14:19:19Z

@kirrg001 good for final test 🙂

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

+                    "integration": "all",
+                    "api_key": "all"
+                },
+                "Admin Integration": {


kirrg001

@kevinansfield Feel free to self merge 👍

kevinansfield requested review from kirrg001 and allouis September 25, 2018 10:54

kevinansfield force-pushed the api-key/schema branch from 4ed3a8d to a5a63b7 Compare September 25, 2018 11:13

kirrg001 assigned kirrg001 and allouis Sep 25, 2018

kevinansfield force-pushed the api-key/schema branch from a5a63b7 to 0a42eb4 Compare September 25, 2018 12:21

kirrg001 reviewed Sep 25, 2018

View reviewed changes

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

kevinansfield force-pushed the api-key/schema branch 2 times, most recently from ca559e8 to 7d626e1 Compare September 25, 2018 13:02

kirrg001 reviewed Sep 25, 2018

View reviewed changes

allouis reviewed Sep 26, 2018

View reviewed changes

kevinansfield force-pushed the api-key/schema branch 2 times, most recently from 1ea8e8e to b7026bb Compare September 26, 2018 15:52

kevinansfield mentioned this pull request Sep 26, 2018

Added API Key authentication middleware to v2 API routes #9915

Closed

7 tasks

allouis reviewed Sep 27, 2018

View reviewed changes

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

kevinansfield force-pushed the api-key/schema branch from b7026bb to 61ee9ce Compare September 27, 2018 10:27

allouis approved these changes Sep 27, 2018

View reviewed changes

kevinansfield force-pushed the api-key/schema branch from 61ee9ce to a1455bb Compare September 27, 2018 10:35

kirrg001 reviewed Sep 27, 2018

View reviewed changes

kevinansfield force-pushed the api-key/schema branch 2 times, most recently from a8cc2c0 to cb983bb Compare October 2, 2018 10:32

allouis reviewed Oct 2, 2018

View reviewed changes

core/server/models/role.js

@@ -10,6 +10,12 @@ Role = ghostBookshelf.Model.extend({

tableName: 'roles',

relationships: ['permissions'],

This comment was marked as abuse.

Sign in to view

allouis reviewed Oct 2, 2018

View reviewed changes

kevinansfield force-pushed the api-key/schema branch from cb983bb to df2833c Compare October 2, 2018 10:43

kirrg001 reviewed Oct 2, 2018

View reviewed changes

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

kirrg001 reviewed Oct 2, 2018

View reviewed changes

kevinansfield force-pushed the api-key/schema branch from 657ec59 to 8bc2add Compare October 2, 2018 14:07

kirrg001 mentioned this pull request Oct 2, 2018

Added migration script for new mobiledoc_revisions table #9934

Merged

kirrg001 reviewed Oct 2, 2018

View reviewed changes

kevinansfield merged commit 1db3aef into TryGhost:master Oct 2, 2018

kevinansfield deleted the api-key/schema branch October 2, 2018 16:46

		@@ -10,6 +10,12 @@ Role = ghostBookshelf.Model.extend({

		tableName: 'roles',

		relationships: ['permissions'],

Setup schema and models for API Key authentication #9904

Setup schema and models for API Key authentication #9904

Conversation

kevinansfield commented Sep 25, 2018 • edited Loading

kirrg001 commented Sep 25, 2018

kevinansfield commented Sep 25, 2018

kirrg001 commented Sep 25, 2018

kevinansfield commented Sep 25, 2018

kirrg001 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.

kevinansfield commented Sep 25, 2018

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.

kevinansfield commented Sep 27, 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.

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.

kevinansfield commented Oct 2, 2018

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

kirrg001 commented Oct 2, 2018 • edited Loading

kevinansfield commented Oct 2, 2018

kirrg001 commented Oct 2, 2018

kevinansfield commented Oct 2, 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.

kirrg001 left a comment

Choose a reason for hiding this comment

kevinansfield commented Sep 25, 2018 •

edited

Loading

kirrg001 commented Oct 2, 2018 •

edited

Loading