Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Permissions / ACL #113

Closed
wants to merge 1 commit into from
Closed

Permissions / ACL #113

wants to merge 1 commit into from

Conversation

jgable
Copy link
Contributor

@jgable jgable commented Jun 5, 2013

Initial implementation of #29

  • Created Role model
  • Created Permission model
  • Linked Users<->Roles with a belongsToMany relationship
  • Linked Permissions to Users and Roles with a belongsToMany relationship
  • Created permissions helper with functions for initializing and
    checking permissions (canThis)
  • Unit tests for lots of things

I think the canThis(...) stuff is probably the most complicated so here is a quick run down.

  • We call permissions.init() which will build the "actionsMap" that the CanThisResult code uses to fill out the properties like edit.post(). The Ghost.init() should be modified to also call permissions.init(). Any time permissions are updated/modified, we should call permissions.refresh(); I think we can do this with a global saving handler on the Permission model, but I'm not sure.
  • Any code that needs to check permissions will require('./core/shared/permissions')
  • Calling permissions.canThis(userModelOrId).edit.post() returns a promise that will resolve or reject based on whether the user can do the action.
  • Underneath the covers the canThis(...) call is loading the users "effectivePermissions" meaning all the users permissions and any roles it has' permissions.

- Created Role model
- Created Permission model
- Linked Users->Roles with a belongsToMany relationship
- Linked Permissions to Users and Roles with a belongsToMany relationship
- Created permissions helper with functions for initializing and
  checking permissions (canThis)
- Unit tests for lots of things
@ErisDS
Copy link
Member

ErisDS commented Jun 5, 2013

This is looking awesome, I've only taken a super quick glance, but a couple of points / questions:

What made you opt to implement your own? Last time we talked you mentioned using enrolement I think... not that there is a problem with rolling our own, but I am very interested to hear what the story / learning was?

The permissions.canThis(userModelOrId).edit.post() syntax is awesome. I think I understand that we can also do permissions.canThis(userModelOrId).edit.post(posModelOrId)? If so, how can we determine which users can edit which posts? This would usually be by the concept of ownership on the model, but not always. There's obviously no opposite nice syntax for creating permissions (completely understandably, not even sure that would be useful), but at first glance I didn't see a way to do this.

@jgable
Copy link
Contributor Author

jgable commented Jun 5, 2013

The Permission model has an object_id that we could use for being more specific about the permission. If no object_id is defined then it assumes you meant "all the things".

I rolled my own because, well, I got a little obsessed with the Object.defineProperty thing to build that dynamic chain of actions to objects. But, really I started looking at the entitlement code and it was really tiny so I kind of based this on it.

You're right about this only being a useful api for checking permissions one way, but I could come up with a different set of helpers for granting permissions and listing them for maybe an admin page. Do we have any mockups for those kind of pages yet? We always have the raw models as well for attaching things because of the relations, there's an example in the permissions_spec test.

@ErisDS
Copy link
Member

ErisDS commented Jun 6, 2013

This means that we will require an individual row in the permissions table for each item a person has access to?

I'm thinking specifically of authors who can only edit the posts they authored. I imagined having a concept of ownership, but then I guess that requires the ACL to be more tightly tied to the models.

@ErisDS
Copy link
Member

ErisDS commented Jun 6, 2013

Have manually merged this.

@ErisDS ErisDS closed this Jun 6, 2013
daniellockyer pushed a commit that referenced this pull request Jul 20, 2022
refs TryGhost/Members#105

- It's a follow up to a series of refactorings in the module mostly discussed in refed PR
- The sendEmailWithMagicLink and destroyStripeSubscriptions were exposed through members API so that Ghost  could call it from the controller level
daniellockyer pushed a commit that referenced this pull request Oct 5, 2022
Co-authored-by: Renovate Bot <bot@renovateapp.com>
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.

2 participants