[V2.0] Track Permissions Through Topics #270
Conversation
…/streams into v2.0/test-permissions-in-cursor-store
… v2.0/test-permissions-in-cursor-store
lets/src/id/permission.rs
Outdated
|
|
||
| impl Mask<&Permissioned<&Identifier>> for sizeof::Context { | ||
| fn mask(&mut self, permission: &Permissioned<&Identifier>) -> Result<&mut Self> { | ||
| self.mask(&permission.take()) |
There was a problem hiding this comment.
In order to avoid the take here and 245, move the impl from their respective methods to this one, and use self.mask(&permission.as_ref()) to call this from the other method
There was a problem hiding this comment.
Yes that does make more sense, changed
streams/src/api/user.rs
Outdated
| .is_cursor_tracked(topic, subscriber.identifier()); | ||
| !subscriber.is_readonly() && no_tracked_cursor | ||
| let permission = self.state.cursor_store.get_permission(topic, subscriber.identifier()); | ||
| let tracked_and_equal = permission.is_some() && permission == Some(&subscriber.take()); |
There was a problem hiding this comment.
We could replace the permission == Some() with !(permission.unwrap().as_ref() == subscriber)
streams/src/api/user.rs
Outdated
|
|
||
| // If a branch admin does not include a user in the keyload, any further messages sent by | ||
| // the user will not be received by the others, so remove them from the publisher pool | ||
| let stored_subscribers: Vec<(Topic, Permissioned<Identifier>, usize)> = self |
There was a problem hiding this comment.
Cant we use cursors_by_topic?
There was a problem hiding this comment.
We can and should yes
| { | ||
| self.state | ||
| .cursor_store | ||
| .insert_cursor(&topic, Permissioned::Read(perm.identifier().clone()), cursor); |
There was a problem hiding this comment.
Hmm we store the subscriber in the list if he wasnt added anymore, are we sure we want this behaviour?
There was a problem hiding this comment.
It's either this or removing them, which would reset their cursor standing, so I think it's appropriate to retain the cursor but in a state that it won't be used to iterate new messages unless added again in a later keyload by the Author.
There was a problem hiding this comment.
To me, if youre not included in the keyload yoou shouldnt have access to the data.
I know we said that keyloads carry on through branches, but i wasnt thinking on a additive manner, same keyload or new keyload only. Lets see what @arnauorriols thinks about this
There was a problem hiding this comment.
which would reset their cursor standing
Oh! I also thought it was weird not to remove the cursor, but you are right, we need to keep the cursor around in case the user is added to the branch again.
To me, if youre not included in the keyload yoou shouldnt have access to the data
This does not change. the user that is not included in the keyload cannot decode the message, period. The fact that it remains tracked is to keep the cursor around. It would probably be less confusing if instead of using the permissions, we had a type that expressed this intent explicitly
lets/src/id/permission.rs
Outdated
| } | ||
|
|
||
| impl Permissioned<&Identifier> { | ||
| pub fn take(self) -> Permissioned<Identifier> { |
There was a problem hiding this comment.
Also its not much of a take(), more a clone. could be changed to take an &self and then (*id).clone(). Why not call it clone?
There was a problem hiding this comment.
I was grappling between the two honestly so no problem changing that to clone
There was a problem hiding this comment.
Ah I remember now why I chose take, if it's not an actual clone implementation clippy does not like it
There was a problem hiding this comment.
lets implement a From trait instead then, i think it looks better.
impl From<Permissioned<&Identifier>> for Permissioned<Identifier> {
fn from(permisison: Permissioned<&Identifier>) -> Self {
match permisison {
Permissioned::Read(id) => Permissioned::Read(id.clone()),
Permissioned::ReadWrite(id, duration) => Permissioned::ReadWrite(id.clone(), duration),
Permissioned::Admin(id) => Permissioned::Admin(id.clone()),
}
}
}
| .subscribers() | ||
| .map(|s| { | ||
| if s == permission.identifier() { | ||
| Permissioned::Admin(s.clone()) |
There was a problem hiding this comment.
What is this step for? Just for admin to be able to check his permissions?
There was a problem hiding this comment.
For admin to establish that they will continue to be admin going forward and not replace their permissions with a read/write
| { | ||
| self.state | ||
| .cursor_store | ||
| .insert_cursor(&topic, Permissioned::Read(perm.identifier().clone()), cursor); |
There was a problem hiding this comment.
which would reset their cursor standing
Oh! I also thought it was weird not to remove the cursor, but you are right, we need to keep the cursor around in case the user is added to the branch again.
To me, if youre not included in the keyload yoou shouldnt have access to the data
This does not change. the user that is not included in the keyload cannot decode the message, period. The fact that it remains tracked is to keep the cursor around. It would probably be less confusing if instead of using the permissions, we had a type that expressed this intent explicitly
Description of change
Currently the only place we use
Permissionedis when we are doing checks onKeyloadmessages, but those permissions are not retained in store anywhere, as our cursor store only uses the Identifier. To allow for that permission retention we need to changeCursorStoreto map byPermissioned<Identifier>instead of justIdentifier. This brings a host of other trickling changes along with it.For example, a
Userwill be able to check their permissions on a topic before they attempt to send messages, failing if they are registered asReadby a topic administrator. Additionally administrative privileges can be properly assigned and forwarded through the topics.Requires #262 to be merged
Type of change
How the change has been tested
New section added to end of examples to have
Authorre-subscribeSubscriberAand grant them Admin privileges, confirming that they carry forward into new branches.