Skip to content

Conversation

hkethi002
Copy link
Contributor

@hkethi002 hkethi002 commented Jul 17, 2017

Fixes #601

Changes

  • /groups/{GroupId}/permission?propagate=true will now propagate permission changes down hierarchy
  • POST will add the permission to all of that group's projects that doesn't have that user's permission already, projects that do already have it are updated to the new access
  • PUT and DELETE will update and delete that group's projects that already have the user's permission and won't affect projects without the user's permission.

Review Checklist

  • Tests were added to cover all code changes
  • Documentation was added / updated
  • Code and tests follow standards in CONTRIBUTING.md

@hkethi002 hkethi002 requested a review from nagem July 17, 2017 15:45
@kofalt
Copy link
Contributor

kofalt commented Jul 17, 2017

Can I request that we invert this behavior?

From previous discussion, it sounded to me like this wants to be the new default behavior. I'd request that the flag be renamed isolate or something, and setting it to false or omitting it would not propagate.

@nagem
Copy link
Contributor

nagem commented Jul 17, 2017

@hkethi002 , @kofalt - @gsfr and I were discussing this on Friday, unfortunately it's not this simple. This would do a replace of the projects' permission with that of the group permissions. Instead, the expected behavior is that the specific addition or change would be propagated through the hierarchy.

For example, group permissions are still supposed to be a set of permissions that are applied by default to a new project. After that, permissions might differ wildly from project to project in the same group. If one project decides to add a user to their project's permissions, a change here will wipe their permission to that project. It is more likely they would want to add a new user to all existing projects in a group, or make a user an admin of all existing projects, rather than "reset" all projects to the state of the group's permissions list.

Unfortunately the "correct" behavior for pushing changes is still under discussion.

@kofalt
Copy link
Contributor

kofalt commented Jul 17, 2017

Let's check that my understanding is correct:

  1. This PR's flag, when set to true, makes a change to a group's permission, then overwrites all member projects with the updated group permission set.
  2. Ideally, instead the specific change (add/remove/change 1+ users) could be applied instead. This is left as a possible feature for the future.
  3. It's anticipated that this PR's feature would be used rarely, thus it is opt-in.

A) Is the above list accurate?
B) If so, are sessions / etc affected when you use this PR's flag? How far does it "push"?
C) Should this feature be exposed in the SDK?

@nagem
Copy link
Contributor

nagem commented Jul 17, 2017

A) #2 is inaccurate as this feature as it currently works in this PR is not the requested behavior and is not really a "mid-step" to requested behavior.
B) It would push to all hierarchy levels below it, as session and acquisition permissions are stored on the objects for convenience, their permissions cannot be edited individually
C) Only for completeness. I don't think many people will be doing one-time admin jobs like editing permissions through the SDK/a script using the SDK.

@kofalt
Copy link
Contributor

kofalt commented Jul 17, 2017

Given your response to A, is the intention that this PR will change significantly before being merged? If not, will we use this feature in the web UI, or just leave it as an admin-ish feature?

Re: C, I'll probably omit it for now then :)

@nagem
Copy link
Contributor

nagem commented Jul 17, 2017

It will have to change, yes. Or at least also support only applying the change in the request body. And the web UI is the expected client that would be using this endpoint. The envisioned UI would have a "propagate"* button next to each item in the list of permissions, and maybe a global one at the top that does what this PR currently does.

Just to be clear, on Friday I was originally writing a change to do exactly what's in this PR, but the nuance of not wanting to clear project specific permissions was realized and development was put on hold. @hkethi002 we can discuss the best way to cleanly add the requests above to what is here, that should cover all possible functionality requested for this endpoint.

@hkethi002 hkethi002 force-pushed the group-perm-push branch 2 times, most recently from c0d042f to 2528f32 Compare July 21, 2017 16:22
@nagem
Copy link
Contributor

nagem commented Jul 27, 2017

@hkethi002 mind writing a few example requests and how they would change the underlying projects?

elif oper == 'DELETE':
config.db.projects.update_many({'group': _id, 'permissions._id' : update}, {'$pull' : {'permissions': {'_id': update}}})
else:
raise APIStorageException("Cannot propagate {} operation".format(oper))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you reuse the function below and set the query and update commands based on what you would do here? I'd prefer reusing complicated hierarchy functions when possible.

@hkethi002 hkethi002 force-pushed the group-perm-push branch 3 times, most recently from eda3cca to f1922b4 Compare August 2, 2017 19:24
@nagem
Copy link
Contributor

nagem commented Aug 15, 2017

Changes look good, thanks!

@hkethi002 hkethi002 merged commit c266472 into master Aug 15, 2017
@hkethi002 hkethi002 deleted the group-perm-push branch August 15, 2017 18:58
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.

3 participants