Propagate permissions when adding users & streams to projects
didimitrie opened this issue · 10 comments
Step 0:
- I've read the contribution guidelines!
Expected Behaviour
- When adding a user to a project (canRead, canWrite) propagate that to all the streams in the project.
- Likewise when adding a stream to a project, add to its permissions all canRead/canWrite users.
Proposed Solution (if any)
For now, the end clients can update this, but it's not elegant :)
What's the expected behaviour when removing a stream from a project? Should it go back to the permissions it had before it was added? Or have project's users' permissions blanket removed from it?
Adding an inherit flag and canNotRead and canNotWrite fields on projects, streams, and objects, might help. But I'm not sure how well that plays with the possibility of streams belonging to multiple projects, or objects belonging to multiple streams. And I have no idea how you'd present a sane UI for it..
I don't really have any good suggestions, but just thought I'd share those thoughts.
Hah, good question. Let's forget the ui and nested stuff for now - it's hairy as it is; see below.
Talking out loud about it:
Permissions are actually just read < write < owner/admin. Ref: project schema permissions for users in projects are stored in there, not in the project permissions per se. Project permissions per se just allow for editing the project itself.
I remember some vague prior plans about this, it was supposed to be much more of a cosmetic step!
Off the top of my head:
// add stream to project
streamsToAdd.forEach( stream => {
project.permissions.canRead.forEach( userId => stream.canRead.push( userId ) )
project.permissions.canWrite.forEach( userId => stream.canWrite.push( userId ) )
} )
// remove stream
streamsToRemove.forEach( stream => {
project.permissions.canRead.forEach( userId => stream.canRead.splice( stream.canRead.indexOf(userId), 1)
project.permissions.canWrite.forEach( userId => stream.canWrite.splice( stream.canWrite.indexOf(userId), 1)
}}Similar for adding/removing users. When a stream is removed, basically users will access it based on its default levels. Only breaker is if stream A was previously shared with user X before it was added to the project, but we can display a warning saying this will reset permissions on a stream.
Clashes? Yes. Stream P in project A and B, user X in A and B, you remove user X from project B, user X will not be able to access stream A, even though he should be able to since he's still in project A, and so is stream P. Or we do potentially[0] expensive db query to see if user X is by any chance in any other project together with stream A on the same permission level. This stinks of bad schema design 🤔
Or we don't allow streams to be in more than one project 💯 🥇 ?
[0] only way this is not expensive is we set indexes on all those fields for a project
Or we don't allow streams to be in more than one project 💯 🥇 ?
I'd be keen for this, unless anyone is actually using streams in multiple projects.
Similarly, could you also not allow objects to be in more than one stream? That seems a bit more likely to be useful..
I don't think anyone is using projects so far as they've not been implemented anywhere!
Re objects, they "need" to be able to be in more than one stream, as it's proven to have been effective (not storing the same fat mesh surroundings mesh twice saves space), so would keep them unless good reason not to (curious where you see things though)!
To be honest, i'm regretting more and more being lazy & not going for a recursive data structure, as this would have been easier and more elegantly solved right now.
A stream is just a SpeckleObject, that can contain more SpeckleObjects. A project then is just another friggin SpeckleObject, but with more project-like fields that can be added and all differentiation is actually happening in ui/client space. Then we would have 1/3rd of the api points, and a simple to reason about permission system, where we could say things are decided on top level perms. 2.x.x anyone?
Existential questions aside, let's go for just the cosmetic approach for now :)
I wouldn't even enforce one project per stream only, and just - in the future ui - and just ask 'hey, this will reset stream permissions! do you want to do so?' Yes - remove users from canRead, etc. No - just remove the stream from the project, leave permissions intact
what happens with permissions when you merge 2 streams and they have different permissions for 1 user for example, with
- stream A : user X can read
- stream B : user X is admin
hey @radumg! stream merges are not in the server logic - and will most probably not be, as defining what a conflict is takes more than clash detection imho (some good old design heuristic skill is needed). offtopic-ish, but essentially it's a client authenticated stream creation act at the end of the day, which means you'll decide what permissions to set (as a dev, in the client implementation of the merge itself, or as a user via an amazing ui)
coolio, thanks @didimitrie ! Figured it might be a bit offtopic (sorry!) but any future merge code would likely be calling these endpoints, so wanted to bring it up 😁
Yes, no worries. I'm just mulling wether to not do any work on projects until we have clean solution (which would require a jump to 2.x.x).
The cosmetic approach is easy-ish to implement server side, but ui wise it will work that might get lost ⚖️
Actually, for 1.x.x, clients will be responsible to do this. Otherwise two custom endpoints are needed:
- ProjectAddUsers
- ProjectAddStreams
I'm not keen on adding them, as workload wise it will be easier to do this in the admin ui. If anyone feels up for the job, feel free to reopen.