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

[Ask Purpose of Access Control on P2P Topic] #16

Closed
riandyrn opened this issue Aug 2, 2017 · 25 comments
Closed

[Ask Purpose of Access Control on P2P Topic] #16

riandyrn opened this issue Aug 2, 2017 · 25 comments

Comments

@riandyrn
Copy link
Contributor

riandyrn commented Aug 2, 2017

Hello, Gene

I have question regarding purpose of access control on P2P topic. So what is the purpose of it anyway?

The reason why I ask this question is because I found that the purpose stated in documentation is somewhat contradict with the implementation written on code. So with this question I would like to validate what I found because I think it is very fundamental.

In documentation (see here) I found that the purpose of it is for managing presence + banning user from initiating or continuing P2P conversation. So if there are user1 & user2 which have P2P connection, then after sometime user2 would like to block message from user1, then user2 would be just only need to remove “W” permission from user1.

But based on current implementation it is not possible to achieve that purpose. The reason is in current implementation it is not possible to change others permission on P2P topic, it is just for setting user own permission for filtering the data retrieved by user itself (which I personally think is somewhat useless on this context).

So in above case, user2 would only able to manage its own permission, so does with user1, so they cannot block each other in case they want to do it.

So which purpose is true? Managing the P2P connection or just for filtering data retrieved from the context? Or maybe you have another design decision?

Thanks a lot, Gene.

@or-else
Copy link
Contributor

or-else commented Aug 3, 2017

Well, yes, the implementation is different from the documentation. And yes, implementation is not great. The only permission which works as intended is 'P'. The rest should be fixed.

@or-else
Copy link
Contributor

or-else commented Aug 3, 2017

Permissions have two sides: 'given' and 'want'. With P2P the original idea was for peers to set each other's 'given' permissions. But the mechanism to set the 'given' side for P2P topics is not implemented. Basically, it should work by user1 sending setMeta for user2 and vice versa.

@or-else
Copy link
Contributor

or-else commented Aug 3, 2017

The key question is if it needs to be fixed now.

@riandyrn
Copy link
Contributor Author

riandyrn commented Aug 3, 2017

Hmm, I see, so the original idea was each user in P2P topic could set other user given permission.

But I think we could make it simpler. What do you think if we just treat the permission in P2P topic as communicating directly to other user rather than to own channel which connect to other user?

So for example there are user1 & user2, user1 have P2P default access ”JWRP” & user2 have P2P default access ”JRP”.

In current implementation, if we create P2P topic for user1 & user2, the access to P2P topic for user1 would be ”JWRP” & user2 would be ”JRP” since in current implementation we treat P2P topic as our own topic which connect to another user. The consequence of this configuration is user2 blocking itself for communicating with user1 (cannot send message to user1), but user1 could still send message to user2. But user2 then could send message to user1 if itself add ”W” permission to its own P2P topic which connect to user1.

But if we treat P2P topic permission as directly communicating to other user, user1 would give “JWRP” permission to user2 & user2 would give "JRP" permission to user1. Hence, the access to P2P topic for user1 would be ”JRP” & user2 would be ”JWRP”.

So in this case, user1 won’t be able to send user2 message since by default user2 doesn’t want to receive message from any stranger. But notice that since the one which control user1 permission is user2, then if it want to allow user1 to send message to it, then it just need to change user1 permission from “JRP” to “JWRP”. If somehow in the future user2 would like block message from user1 again, it just need to remove ”W” bit from user1 P2P permission. So does with user1 if it somehow want to block message from user2.

What do you think?

@riandyrn
Copy link
Contributor Author

riandyrn commented Aug 3, 2017

The key question is if it needs to be fixed now.

Hmm…, I think it depends on your roadmap, Gene. But I found that this issue won’t affect the system basic functionality as long as both users in P2P topic having “JWRP” permission. But for advance use case, this issue need to be addressed correctly.

Btw, in current implementation I found that the default value for P2P default access for newly created user is ”JRPD” not ”JWRPS”. This will cause issue in P2P topic permission setting since the resulted P2P mode for user which having “JRPD” permission would be “JRP”, in which means it won’t be able to send message to another user. So when we are creating new user, we need to tell server explicitly that we want to create user which have “JRWPS” permission as P2P default access, by setting acc.desc.defacs to ”JWRPS”.

If you need some extra hand, I would gladly to participate 😃👍

@or-else
Copy link
Contributor

or-else commented Aug 3, 2017

In current implementation,

Yes, that's the correct description of the current incorrect implementation. P2P's given is currently set to JRWP for all users. A user configures a filter by setting want permissions. Thats not right, I agree.

But if we treat P2P topic permission as directly communicating to other user

I think there is a legitimate need for two sets of permissions, wanted and given. For instance, user1 may not want to accept connections from other users. So, user2 has to ask for access first: "user1, please give me permissions want". Then user1 would "approve" the request by setting given permissions.

The P permission is truly a filter, user may need to set it on the receiving end.

Btw, in current implementation I found that the default value for P2P default access for newly created user is ”JRPD” not ”JWRPS”

It should be JRWP for P2P topics https://github.com/tinode/chat/blob/master/server/store/types/types.go#L290

@or-else
Copy link
Contributor

or-else commented Aug 3, 2017

If it creates problems then maybe I should just fix it. Since you are using it I would greatly appreciate your help with debugging.

@riandyrn
Copy link
Contributor Author

riandyrn commented Aug 3, 2017

I think there is a legitimate need for two sets of permissions, wanted and given. For instance, user1 may not want to accept connections from other users…

Hoo…, ok (y)

So how does system supposed know that user1 doesn't want to accept connections from other users while maybe another users by default want to accept connections from anyone? Is it by looking at user1 default access or maybe something else?

The reason why I'm asking this is because in current implementation, user’s default access define 'want' permission for this user to other user P2P topic, right? Not define 'given' permission for other user to access this user.

It should be JRWP for P2P topics

Hmm…, ok, I will try to debug it (y)

If it creates problems then maybe I should just fix it. Since you are using it I would greatly appreciate your help with debugging.

Sure, it’s an honor to be able to help you 😎👍

@or-else
Copy link
Contributor

or-else commented Aug 4, 2017

Is it by looking at user1 default access or maybe something else?

Yes, use1's default access should define given for P2P topics with this user (it's does not work like this right now and it needs fixing)

The reason why I'm asking this is because in current implementation, user’s default access define 'want' permission for this user to other user P2P topic, right? Not define 'given' permission for other user to access this user.

Yes, that's how it's implemented now and that's not right. It should define given for the other user instead. Default want should be just the maximum possible value, for instance equal to given.

I'll look into it over the weekend.

@or-else
Copy link
Contributor

or-else commented Aug 6, 2017

JFYI, I'm working on this. It's just taking a bit longer than expected.

@or-else
Copy link
Contributor

or-else commented Aug 9, 2017

Please take a look: #20

@or-else
Copy link
Contributor

or-else commented Aug 9, 2017

And tinode/webapp#7

@riandyrn
Copy link
Contributor Author

riandyrn commented Aug 9, 2017

Hello, Gene

I noticed that you change ModeCP2P (Default P2P Access Mode) value from "JRWP" to "JRWPA" (see here). Why?

I thought that in this new version we will be using default P2P access mode as permission given to other user which trying to connect with this user. But when you add the "A" permission into it, it’s become weird. Here is the reason why I think it's become weird:

  • J => other user, you may join this p2p topic
  • R => other user, you may receive broadcast from this p2p topic
  • W => other user, you may write to this p2p topic
  • P => other user, you may receive presence updates from this p2p topic
  • A => other user, you may approve request to join this p2p topic (???)

Why would current user give access to approve the newly established p2p connection to another user rather than itself? It should be current user which have the permission to approve the connection, right?

Or maybe I misunderstood the permissions meaning?

Thanks.

@or-else
Copy link
Contributor

or-else commented Aug 9, 2017

Hi Riandy,

A is intended as a topic administrator's permission. A is for Administrator. It's a permission which allows the A user to manage permissions of other subscribers of the given topic (P2P or Group). It seems appropriate for the case of P2P topics where each user manages permissions of the other user.

The two participants of a P2P topic cannot invite other users to the topic because of the way the name of a P2P topic is composed. It's based on a concatenation of the IDs of the two participants. So the A permission will only affect the two members of the P2P topic.

@or-else
Copy link
Contributor

or-else commented Aug 9, 2017

If I don't hear any objections in the next few hours I'm going to merge it to devel

@riandyrn
Copy link
Contributor Author

riandyrn commented Aug 9, 2017

Hmm…, I see…

But wouldn’t it better if "A" permission appended directly to each users in P2P topic rather than inferred from user's default access?

Notice following scenario:

For example there are user A & B which have "JRWP" & "JRWPA" default access respectively. At some point, user B would like to connect with user A. Since in current system the "A" permission doesn't directly appended to each user, thus user A got "JRWPA" mode for user B topic, & user B got "JRWP" mode for user A topic.

Since user B doesn't have "A" permission, thus it cannot control user A topic. But in another side, since user A have "A" permission over user B, thus it has complete control over user B. So if somehow user A would like to spam user B, user A could do it freely without any resistance, right?

@or-else
Copy link
Contributor

or-else commented Aug 10, 2017

But wouldn’t it better if "A" permission appended directly to each users in P2P topic rather than inferred from user's default access?

It is appended. Here: https://github.com/tinode/chat/blob/permissions/server/store/types/types.go#L291
And then I check that it cannot be removed, for instance here https://github.com/tinode/chat/blob/permissions/server/store/types/types.go#L291 and here https://github.com/tinode/chat/blob/permissions/server/topic.go#L749

For example there are user A & B which have "JRWP" & "JRWPA"

That's not possible because user A will always have A permission

@riandyrn
Copy link
Contributor Author

riandyrn commented Aug 10, 2017

Hmm..., here are the steps which I'm using to generate above scenario where user could remove "A" permission from its P2P default access:

  1. login with any account (I recommend to use newly created account so it has "JRWPA" permission by default)
  2. subscribe to me topic
{
  "sub": {
    "id": "make online presence",
    "topic": "me"
  }
}
  1. set the P2P default permission to "JRWP"
{
  "set": {
    "id": "set me permission",
    "topic": "me",
    "desc": {
      "defacs": {
        "auth": "JRWP",
        "anon": "N"
      }
    }
  }
}

So yes, it cannot remove "A" permission from existing P2P topic. But for incoming created topics, it can.

Also notice that the "JRWPA" default access would only owned by newly created user, for the old ones, they will still have "JRWP" default access.

It is appended. Here:

Hmm..., what I meant with append was we append the "A" permission in newly created P2P topic for both sides if the resulted mode for any of them is not having "A" permission on it.

Notice that if the user having P2P default access "JRWP" the resulted permission for user in other side is "JRWP" even though the given permission from server is "JRWPA".

@or-else
Copy link
Contributor

or-else commented Aug 10, 2017

here are the steps which I'm using to generate above scenario where user could remove "A" permission from its P2P default access

Good point, thanks. I'll fix that.

Also notice that the "JRWPA" default access would only owned by newly created user

I'm still not sure what you mean here. The database has to be either updated or regenerated with the new permissions.

what I meant with append was we append the "A" permission in newly created P2P topic for both sides if the resulted mode for any of them is not having "A" permission on it.

Yes, that's what I mean too. It should always have an A in it unless it's an N.

Notice that if the user having P2P default access "JRWP" the resulted permission for user in other side is "JRWP" even though the given permission from server is "JRWPA".

Correct, but the default access without an A is not supposed to happen.

@riandyrn
Copy link
Contributor Author

I'm still not sure what you mean here. The database has to be either updated or regenerated with the new permissions.

Ok, never mind for this one. Actually I got hint for this issue because I was using the old database which still contain user with previous permission model without updating it 😅.

Good point, thanks. I'll fix that.

Another scenario is when user specify acc.desc.defacs when it create new account:

{
  "acc": {
    "id": "create new user",
    "user": "new",
    "scheme": "basic",
    "secret": "d3JvbTp3cm9t",
    "tags": [
      "email:[email protected]"
    ],
    "desc": {
      "defacs": {
        "auth": "JRWP",
        "anon": "N"
      },
      "public": {
        "fn": "wrom"
      }
    }
  }
}

@or-else
Copy link
Contributor

or-else commented Aug 10, 2017

Thanks. Updated

@or-else
Copy link
Contributor

or-else commented Aug 11, 2017

Merged into devel

@or-else or-else closed this as completed Aug 11, 2017
@sanmiguel2019
Copy link

sanmiguel2019 commented Dec 7, 2020

I have a similar question about users permissions in users table. Ok, I understand the purpose but at the same time - why they are needed if root user is not allowed to change them later? All the users have the same permissions and normally permissions are intended to be granted or revoked. In Tinode users permissions once granted can not be revoked anymore. For example, I want to block some user for creating new topics. I would set {"Auth":"N","Anon":"N"} but it's not possible,

As documentation says desc / defacs of the acc message is only used to create a new account. But how do I change them later? Changing them in the database only takes affect after restarting Tinode.

@or-else
Copy link
Contributor

or-else commented Dec 7, 2020

why they are needed if root user is not allowed to change them later

A root user can change anything for anyone that a normal users can change for themselves.

In Tinode users permissions once granted can not be revoked anymore

That is untrue.

@or-else
Copy link
Contributor

or-else commented Dec 7, 2020

Please do not comment on old issues. If you have a question, please use the forum.

@tinode tinode locked as off-topic and limited conversation to collaborators Dec 7, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants