Skip to content

Add app permission checking to canThis#2199

Closed
jgable wants to merge 1 commit intoTryGhost:masterfrom
jgable:canThisApp
Closed

Add app permission checking to canThis#2199
jgable wants to merge 1 commit intoTryGhost:masterfrom
jgable:canThisApp

Conversation

@jgable
Copy link
Contributor

@jgable jgable commented Feb 15, 2014

Progress on #2059

  • Create apps table and App model (Increments schema to 003)
  • Pass permissions loading to buildObjectTypeHandlers to eliminate shared state
  • Load both app and user permissions to check in canThisResult
  • Check app permissions if present after user checks.
  • Move effectiveUserPermissions to permissions/effective.js so logic is one place.
  • Change permissable interface to take context; has user and app and could contain more in future.
  • Add unit tests for app canThis checks and effective permissions

Not really sure if I'm doing the schema stuff correctly, so feel free to review that.

In order to start using this #2179 will need merged, and then we'll need to push the loaded permissions into the database during app loading. I thought this was big enough by itself without adding that stuff in.

- Pass permissions loading to buildObjectTypeHandlers to eliminate
shared state
- Load both app and user permissions to check
- Check app permissions if present
- Create apps table and App model
- Move effectiveUserPermissions to permissions/effective
- Change permissable interface to take context; user and app.
- Add unit tests for app canThis checks and effective permissions

This comment was marked as abuse.

@halfdan
Copy link
Contributor

halfdan commented Feb 15, 2014

We seem to have some weird testing failures again: https://travis-ci.org/TryGhost/Ghost/builds/18947660

@ErisDS
Copy link
Member

ErisDS commented Feb 15, 2014

The 500 error is due to the handoff between 2 sets of tests I believe, the 'should' error I'm not sure about - I try to track them in #1702. They have certainly become more common.

@sebgie
Copy link
Contributor

sebgie commented Feb 19, 2014

👍 good to merge.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

@ErisDS
Copy link
Member

ErisDS commented Feb 19, 2014

I'm planning to merge this out onto a branch temporarily whilst we wait for #2102.

I don't want to leave it on a branch for long because it'll get hard to merge, equally, merging into master would mean either doing 2 data version bumps or having a rough ride in master for a while and it would be good to avoid those things. At least if it's on a branch rather than in a PR it's my job to keep it rebased and not yours :)

@jgable
Copy link
Contributor Author

jgable commented Feb 19, 2014

Sounds good. Just let me know if you run into any snags.

ErisDS added a commit that referenced this pull request Feb 20, 2014
ErisDS added a commit that referenced this pull request Feb 20, 2014
@ErisDS
Copy link
Member

ErisDS commented Mar 1, 2014

This is merged on the 003-data-updates branch - which we are now definitely using so closing this.

@ErisDS ErisDS closed this Mar 1, 2014
@halfdan halfdan mentioned this pull request Mar 6, 2014
@jgable jgable deleted the canThisApp branch March 17, 2014 14:49
ErisDS added a commit that referenced this pull request Apr 2, 2014
@ErisDS
Copy link
Member

ErisDS commented Apr 3, 2014

This has been remerged onto the apps-perms branch which is basically master with all the data changes on it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants