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

Move Transfer Ownership to own API endpoint #3426

Closed
ErisDS opened this issue Jul 28, 2014 · 0 comments · Fixed by #3457
Closed

Move Transfer Ownership to own API endpoint #3426

ErisDS opened this issue Jul 28, 2014 · 0 comments · Fixed by #3457
Assignees
Labels
affects:api Affects the Ghost API

Comments

@ErisDS
Copy link
Member

ErisDS commented Jul 28, 2014

As I've been working on writing tests for #3196 and also getting the user permissions correct and tested for #3096 I've hit a roadblock whereby it's impossible to make all of the conditions pass.

Essentially, I have what I believe is a full suite of tests, where one test always fails. Every time I fix the permissions so that the broken test passes, that change breaks another test.

There are a couple of underlying problems with the permissions model which is causing this:

For user edit and add we now have 2 permissions checks:

canThis(context).add/edit.user(userId) and canThis(context).assign.role(roleId)

This is a bit restrictive in that:

  1. We can't ask if the context can assign a role to a specific user, essentially the code should probably be:

canThis(context).add.roleUser(roleId, userId)

But joinTables aren't treated as models and canThis isn't really setup for testing the join tables. canThis(context).add.role(roleId, userId) or canThis(context).add.role(roleId).to.user(userId) would be great, but will require extensive reworking of canThis.

The side effect of this is that the check which prevents modifying your own role has to be outside of the permissions system: https://github.com/TryGhost/Ghost/pull/3395/files#diff-e1c52a506c4dfb4202439ef6db5f2de0R111

This is not so bad, and we'll likely have an issue to extend canThis and fix this later.

  1. Inside of testing for assign permissions, we don't know whether we're assigning to a new or existing user.

The rules for assigning to an existing user, and assigning to a brand new user are subtly different:

  • no one can assign the owner role to a new user
  • owners can assign the owner role to an existing admin user

This could be solved by having 2 permissions: assignToNew and assignToExisting, but the above latter rule is actually just one very special case purely for transferring ownership. When the owner wants to transfer their ownership to another user, they can do so only if the other user exists and is already an admin. No one else is ever allowed to perform this action, and this action is already considered to be different to a standard edit in the UI (there's a different control),

Furthermore, it seems odd to me that in a certain case, calling edit on a user model can cause more than one user to be updated.

Finally this would resolve both my problems with the permissions model and #3403 in one fell swoop. Therefore I believe, making an endpoint just for this change makes the most sense.

That just leaves what to call it.. I'm looking at:

router.put('/users/owner', api.http(api.users.transferOwnership));

@ErisDS ErisDS added this to the 0.5 Multi-user milestone Jul 28, 2014
sebgie added a commit to sebgie/Ghost that referenced this issue Jul 30, 2014
closes TryGhost#3426
- added transfer ownership endpoint
- added owner to roles.permissible
- manually removed owner from roles.browse
- removed hard coded author role
- fixed tests that were passing due to hard coded author role
- added testUtils.setup(‚roles‘)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
affects:api Affects the Ghost API
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants