Schedules controller migration to v2 #10975

naz · 2019-08-02T07:21:00Z

Main changes:

Introduces v2 schedules controller for posts/pages scheduling
Creates internal integration for 'Ghost Scheduler' which is allowed to only access schedules endpoint
Will require migration of existing scheduler clients to use internal integration keys and new way of processing schedulable resources

Todo:

proper authorization for schedules integration
private getScheduledPosts method migration
transactions
migrations

naz · 2019-08-02T15:48:22Z

core/server/api/v2/pages.js

@@ -43,7 +43,10 @@ module.exports = {
            'fields',
            'formats',
            'debug',
-            'absolute_urls'
+            'absolute_urls',
+            // NOTE: only for internal context


These are required so that "frame" correctly processes internal options with transactions. If the context is not internal the options are automatically cleaned here - https://github.com/TryGhost/Ghost/blob/a31ed7c/core/server/api/shared/serializers/input/all.js#L36-L39

naz · 2019-08-02T15:50:02Z

core/server/api/v2/schedules.js

+            }
+        },
+        query(frame) {
+            const resourceType = frame.data.resource;


NOTE: dropped from and to parameters support (they were present in v01 controller) as they were never used.

naz · 2019-08-02T15:52:08Z

core/test/regression/api/v2/admin/utils.js

@@ -102,18 +102,20 @@ module.exports = {
        return testUtils.API.doAuth(`${API_URL}session/`, ...args);
    },

-    getValidAdminToken(endpoint) {
+    getValidAdminToken(endpoint, key) {


Had to be refactored to be able to use custom keys when testing Scheduler Internal Integration

allouis · 2019-08-05T06:39:00Z

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

+                {
+                    "name": "Schedule content",
+                    "action_type": "schedule",
+                    "object_type": "schedule"


Should this object type be post and possibly another one for page?

I'm wondering if this permission should just be "publish" "post" 🤔

Should this object type be post and possibly another one for page?

Was thinking about scheduling in more generic terms of scheduling a "resource" or "content".

I'm wondering if this permission should just be "publish" "post"

My first response was: "nah this is different" 😅 But after looking at it from the higher point all aw allow to do is "publish" here so it 💯 makes sense! The only bit I'm not sure about is the "post"/"page" bit. It's a generic action on anything that is "publishable" and don't see the point of making it that much granular 🤔 On the other hand why invent a new resource type?

After a second look at this, think naming this permission might've been:

{ "name": "Publish resource", "action_type": "publish", "object_type": "publishable" } OR { "name": "Publish post/page", "action_type": "publish", "object_type": "post" / "page" }

Any other ideas? The second approach with "post" / "page" would probably need some more customizations in the "frame" as we only can have one resource per controller at the moment (or we introduce 2 separate controllers 🤔)

@allouis @kevinansfield @rishabhgrg ping (what we talked about on the call)

FWIW I don't think this is a blocker.

We don't have a page object type - and I don't think we need to introduce one.

The permission here seems to be specifically publish: post. By renaming it, we don't change any functionality, but we set some groundwork for fixing the post permissions in future.

As for the scheduler having more permissions in future - this is totally doable, as the scheduler will have the scheduler role - which can have many permissions.

We actively want permissions to be more granular, in the future, starting now isn't a terrible idea IMO.

Again I don't want to block on this - get it merged whatever which way - permissions and roles are modifiable in the future

Permissions are not based on controller methods, they're based on actions on objects which we'd like to restrict to particular users/actors.

IMO we should name them accordingly

Cool, makes sense in that case, guess that will just need minor changes to permissions file to handle ☝️ if we go ahead, will leave it on @gargol's call if its part of this PR :)

The messed up thing about permissions is that controllers depend on them in a way that action and object are being matched with docName and the controller method that is being called. Will try to go with publish and post as new permission for scheduler integration, but if it ends up a bit PITA going with what is there already: schedule / schedule.

@allouis @rishabhgrg Note, the side effect and behavior change that is connected with going with publish/post combination is that other roles will start having access to this endpoint (previously only scheduler had the right to publish).

From a functional perspective, it doesn't change much, because every permission that had post: "all" already could "publish" through edit endpoint. But still thought something worth pointing out.

From a functional perspective, it doesn't change much, because every permission that had post: "all" already could "publish" through edit endpoint. But still thought something worth pointing out.

Yeah its definitely important to keep it in mind and some point later make a note of it to also distinguish it with edit, but looks good for now

allouis · 2019-08-05T06:46:55Z

This all looks good, I'm just thinking about the permissions structure. I think scheduling a post and publishing a post is the same permissions 🤔

Either way, the permission that we use for scheduling should be added to owner, administrator, editor & author roles too, maybe Admin Integration?

core/server/adapters/scheduling/post-scheduling/index.js

naz · 2019-08-06T17:58:56Z

@allouis @rishabhgrg have finished with the migrations and testing. Think the last 7 commits (everything after the last force push) is to be reviewed, especially the migrations as they are a little different to ones introduced for db backup integration (which eye missed to spot in the beginning - we add new permission!)

Would love to merge this beast tomorrow finally to unblock /canary work.

closes TryGhost#10060 - Implemented scheduling for posts and pages - Added cache invalidation when scheduling - Refactored admin token eneration function to accept existing key as parameter in tests - Added Ghost Scheduler Integration fixture - Added fixture for permissions for post publish action - Migrated getScheduled method to v2 - Did not add support for 'from' and 'to' parameters as they were not used by DefaultScheduler - This method needs rethinking in a long run as it's an ugly hack and should rather become proper endpoint that returns JSON data instead of models - Removed unused auth middleware from v2 routes - Added internal scheduler role - Implemetnted transactions in v2 frame - This takes into account scenario mentioned in c93f03b - Specifically: >if two queries happening in a transaction we have to signalise knex/mysql that we select for an update otherwise the following case happens: you fetch posts for an update a user requests comes in and updates the post (e.g. sets title to "X") you update the fetched posts, title would get overriden to the old one

refs TryGhost#10060 - Modification of https://github.com/TryGhost/Ghost/pull/10974/files - Added publish permission migrations for all roles having "post": "all" permission

kirrg001 mentioned this pull request Aug 2, 2019

API V2 complete controller set #10060

Closed

12 tasks

naz force-pushed the schedules-controller-migration-to-v2 branch 2 times, most recently from e74eb89 to 38813e3 Compare August 2, 2019 14:05

naz marked this pull request as ready for review August 2, 2019 15:42

naz requested review from allouis and rishabhgrg August 2, 2019 15:45

naz commented Aug 2, 2019

View reviewed changes

allouis reviewed Aug 5, 2019

View reviewed changes

rishabhgrg reviewed Aug 5, 2019

View reviewed changes

core/server/adapters/scheduling/post-scheduling/index.js Show resolved Hide resolved

naz force-pushed the schedules-controller-migration-to-v2 branch from 2e92030 to c00f04d Compare August 6, 2019 10:03

naz force-pushed the schedules-controller-migration-to-v2 branch 2 times, most recently from 846de5b to b616515 Compare August 7, 2019 12:33

naz added 2 commits August 7, 2019 14:51

Added migrations for scheduler integration, permission and role

532fdb0

refs TryGhost#10060 - Modification of https://github.com/TryGhost/Ghost/pull/10974/files - Added publish permission migrations for all roles having "post": "all" permission

naz force-pushed the schedules-controller-migration-to-v2 branch from b616515 to 532fdb0 Compare August 7, 2019 12:57

naz merged commit 532fdb0 into TryGhost:master Aug 7, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Schedules controller migration to v2 #10975

Schedules controller migration to v2 #10975

naz commented Aug 2, 2019 •

edited

naz Aug 2, 2019

naz Aug 2, 2019

naz Aug 2, 2019 •

edited

allouis Aug 5, 2019

allouis Aug 5, 2019

naz Aug 5, 2019

naz Aug 5, 2019

allouis Aug 5, 2019

allouis Aug 6, 2019

rishabhgrg Aug 6, 2019 •

edited

naz Aug 6, 2019

naz Aug 6, 2019

rishabhgrg Aug 7, 2019 •

edited

allouis commented Aug 5, 2019

naz commented Aug 6, 2019

Schedules controller migration to v2 #10975

Schedules controller migration to v2 #10975

Conversation

naz commented Aug 2, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

naz Aug 2, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rishabhgrg Aug 6, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rishabhgrg Aug 7, 2019 • edited

Choose a reason for hiding this comment

allouis commented Aug 5, 2019

naz commented Aug 6, 2019

naz commented Aug 2, 2019 •

edited

naz Aug 2, 2019 •

edited

rishabhgrg Aug 6, 2019 •

edited

rishabhgrg Aug 7, 2019 •

edited