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

Community: View&Post token-permitted channel input is not locked when requirements do not match #14117

Closed
anastasiyaig opened this issue Mar 22, 2024 · 15 comments · Fixed by status-im/status-go#5023 or status-im/status-go#5046
Assignees
Labels
backend-team bug Something isn't working E:Desktop Comm Perms and Minting MVP Misc tasks about Community permissions that are not part of another Epic, due for the MVP
Milestone

Comments

@anastasiyaig
Copy link
Contributor

Description

  1. create community
  2. create a permission saying "Keep 0.1 ETH to view and post in general channel"
  3. for community member, create 2 addresses. One address should match the requirements, second address should not
  4. invite member to community
  5. as the member, share the address that do not match the requirements and confirm

As result, member is somewhat joined the community, is able to see a channel and the channel input is not locked. However, posting to the channel do not send the messages (they are not delivered)

Screen.Recording.2024-03-22.at.15.34.42.mov
@anastasiyaig anastasiyaig added bug Something isn't working backend-team E:Desktop Community Shared Addresses Selection Implementation of the Shared Addresses feature for joining communities and also for edits labels Mar 22, 2024
@anastasiyaig
Copy link
Contributor Author

@jrainville sounds like 2.29?

@jrainville
Copy link
Member

@jrainville sounds like 2.29?

Nah, that should be RC. It used to work in 2.27 though

@jrainville jrainville added this to the 2.28.0 Beta RC milestone Mar 22, 2024
@jrainville
Copy link
Member

@cammellos you self-assigned because you know what is going on? From the video, it feels like the backend code is ok. Or at least the canPost code works, because it doesn't send.

@cammellos
Copy link
Member

@jrainville I assigned myself since I was looking on similar issue on mobile status-im/status-mobile#19363 , but if anyone wants in the meantime to look into it, i will unassign myself, it might be 2 different issues

@cammellos cammellos removed their assignment Mar 22, 2024
@jrainville jrainville added E:Desktop Comm Perms and Minting MVP Misc tasks about Community permissions that are not part of another Epic, due for the MVP and removed E:Desktop Community Shared Addresses Selection Implementation of the Shared Addresses feature for joining communities and also for edits labels Mar 22, 2024
@jrainville jrainville assigned kounkou and unassigned MishkaRogachev Mar 25, 2024
@iurimatias iurimatias modified the milestones: 2.28.0 Beta RC, 2.28.0 Beta Mar 25, 2024
@kounkou
Copy link
Contributor

kounkou commented Mar 25, 2024

Hi there, I managed to reproduce the issue easily by following the steps.
We can see that we are allowed to type the message and hit the send message but the logs show

...
_sendChatMessage
DBG 2024-03-25 12:48:37.185-07:00 [threadpool task thread] initiating task   topics="task-threadpool" tid=422996 file=threadpool.nim:54 messageType=AsyncGetTextURLsToUnfurlTaskArg:ObjectType threadid=422996 task="{\"$type\":\"AsyncGetTextURLsToUnfurlTaskArg:ObjectType\",\"text\":\"plop\",\"requestUuid\":\"e3401975-e89a-4128-9a95-dd12b00d5101\",\"vptr\":93994562982848,\"slot\":\"onAsyncGetTextURLsToUnfurl\",\"tptr\":93994542558448}"
DBG 2024-03-25 12:48:37.185-07:00 NewBE_callPrivateRPC                       topics="rpc" tid=422996 file=core.nim:27 rpc_method=wakuext_getTextURLsToUnfurl
ERR 2024-03-25 12:48:37.189-07:00 rpc response error                         topics="rpc" tid=422962 file=core.nim:36 err="\nstatus-go error [methodName:wakuext_sendChatMessage, code:-32000, message:can't post message type '1' on chat '0x03d596865454ef963dfaaa687d4d7972b298874ecd08efa86432f4501cc9cd2a3d845c8962-35ed-490e-99c5-9a8786e0e7bf' ]\n"
ERR 2024-03-25 12:48:37.189-07:00 error doing rpc request                    topics="rpc" tid=422962 file=core.nim:40 methodName=wakuext_sendChatMessage exception="\nstatus-go error [methodName:wakuext_sendChatMessage, code:-32000, message:can't post message type '1' on chat '0x03d596865454ef963dfaaa687d4d7972b298874ecd08efa86432f4501cc9cd2a3d845c8962-35ed-490e-99c5-9a8786e0e7bf' ]\n"
ERR 2024-03-25 12:48:37.189-07:00 Error sending message                      topics="chat-service" tid=422962 file=service.nim:567 msg="\nstatus-go error [methodName:wakuext_sendChatMessage, code:-32000, message:can't post message type '1' on chat '0x03d596865454ef963dfaaa687d4d7972b298874ecd08efa86432f4501cc9cd2a3d845c8962-35ed-490e-99c5-9a8786e0e7bf' ]\n"
...
Screencast.from.2024-03-25.12.48.29.PM.webm

NOTE :

Please notice the missing members, this needs to be addressed too... in another ticket...

@jrainville
Copy link
Member

We can see that we are allowed to type the message and hit the send message but the logs show

That means that the backend works correctly and that the addresses were shared correctly.

We are just failing to update correctly the permission model so that the UI is blocked.

Does a restart fix it?

If so, we only need to recalculate the permissions.

If not, then we need to improve the calculation to use the selected addresses only.

@kounkou
Copy link
Contributor

kounkou commented Mar 25, 2024

We can see that we are allowed to type the message and hit the send message but the logs show

That means that the backend works correctly and that the addresses were shared correctly.

We are just failing to update correctly the permission model so that the UI is blocked.

Does a restart fix it?

If so, we only need to recalculate the permissions.

If not, then we need to improve the calculation to use the selected addresses only.

So a restart fixes it indeed!

EDIT :

So the address that was shared was the one that doesn't match the requirements to view and post. So something wrong with the calculations of the permissions in the first place 😕

So going to the analysis now to meet your guidance saying :

If not, then we need to improve the calculation to use the selected addresses only.

. Thanks so much

Screenshot from 2024-03-25 12-59-42

@jrainville
Copy link
Member

Looking at your screenshot, it doesn't look fixed. When you don't have the tokens for a view/post channel, you shouldn't be able to see inside of it.

@kounkou
Copy link
Contributor

kounkou commented Mar 27, 2024

I made some tests injecting fake values and could see that the UI is working perfect.

The issue is coming from the final calculation of the permission indeed. and this code is the entry point to dive deep https://github.com/status-im/status-go/blob/98c3be55b937582374e60adb651543c5f1348c29/protocol/communities/manager.go#L2884
I will probaby need 1-2 days to spot the error/improvment and produce a PR


Based on my latest finding, it seems like when we pass viewAndPostPermissions

[id:"92614925-8afd-4038-a640-0181cec118f8" type:CAN_VIEW_AND_POST_CHANNEL token_criteria:
{contract_addresses:{key:5 value:"0x0000000000000000000000000000000000000000"} 
contract_addresses:{key:420 value:"0x0000000000000000000000000000000000000000"} 
contract_addresses:{key:421613 value:"0x0000000000000000000000000000000000000000"} 
type:ERC20 symbol:"ETH" amount:"0.05" decimals:18 amountInWei:"50000000000000000"} 
chat_ids:"0x03d596865454ef963dfaaa687d4d7972b298874ecd08efa86432f4501cc9cd2a3d845c8962-35ed-
490e-99c5-9a8786e0e7bf" 
chat_ids:"0x03d596865454ef963dfaaa687d4d7972b298874ecd08efa86432f4501cc9cd2a3d229a46ef-d9e0-
48a4-8162-63f09a008af2"]

to the CheckPermissions function, we get an error message saying NetworksNotSupported.

// if there are no chain IDs that match token criteria chain IDs
// we aren't able to check balances on selected networks

This means that because of that error, we are unable to retrieve the balance for the addresses provided, therefore we do not validate any permissions.
The default result being false, we then always lock access to the channel. This is consistent with latest tests performed.

@cammellos
Copy link
Member

We had a discussion with @kounkou

One issue we identified is that lock icons are calculated using a client check for channel permissions.

That's not the correct way, mobile was doing the same and we had the same issue. Access is checked by the control node, as you need encryption keys for example, and those are only sent over once the admin can validate the user has the available tokens, so all the access control should be directed by the admin, rather than the client. There are flags tokenGated canPost canView for each channel which should be used for showing lock icons/etc.

happy to go into more details if needed.
Unfortunately, even if that's fixed, there's still underlaying issues, most likely control node side, as we have a couple of open issues in mobile (I was hoping that this was the same issue, but at this point it looks different).

@kounkou
Copy link
Contributor

kounkou commented Apr 1, 2024

So the main source of the problem I have identified in the flow of CheckPermissions here https://github.com/status-im/status-go/blob/develop/protocol/communities/permission_checker.go#L200 is that we are getting in chainID's for the permissions on the communities that are :

Ethereum Classic (ETC) network   : 420
Fantom Opera network (FTM)       : 421613
Goerli Testnet                   : 5

while we expect the following chainIDs

Ethereum Mainnet                 : 1
Optimistic Ethereum (OΞ) network : 10
Arbitrum network                 : 42161

Solving this issue will help fix the root cause of why we aren't able to fetch balance for an address and therefore not validate the constraints.
I need to reach out to Wallet to understand why this is the case.

@kounkou
Copy link
Contributor

kounkou commented Apr 2, 2024

Because I was using a community where the owner token was minted with Goerli, this had misleading effects on my tests. I need to move away from using that community and reperform some tests to make sure I can fix the issue more consistently.

kounkou added a commit that referenced this issue Apr 4, 2024
Currently the PR is WIP. This PR fixes #14117

-[] Detect changes when user edits/create/delete permissions
-[] Set the channel permissions at boot
-[] Restore write when channel is unlocked
kounkou added a commit that referenced this issue Apr 4, 2024
Currently the PR is WIP. This PR fixes #14117

-[] Detect changes when user edits/create/delete permissions
-[] Set the channel permissions at boot
-[] Restore write when channel is unlocked
kounkou added a commit that referenced this issue Apr 4, 2024
Currently the PR is WIP. This PR fixes #14117 and will fix the following
use cases

- Restore write when channel is unlocked
- Set the channel permissions at boot
- Detect changes when user
   - edit permissions
   - create permissions
   - delete permissions
kounkou added a commit that referenced this issue Apr 4, 2024
Currently the PR is WIP. This PR fixes #14117 and will fix the following
use cases

- Restore write when channel is unlocked
- Set the channel permissions at boot
- Detect changes when user
   - edit permissions
   - create permissions
   - delete permissions
kounkou added a commit to status-im/status-go that referenced this issue Apr 8, 2024
We are now able to lock consistently the channel if we do not have
permission to view&post.
This PR fixes status-im/status-desktop#14117
kounkou added a commit to status-im/status-go that referenced this issue Apr 8, 2024
We are now able to lock consistently the channel if we do not have
permission to view&post.
This PR fixes status-im/status-desktop#14117
kounkou added a commit to status-im/status-go that referenced this issue Apr 8, 2024
We are now able to lock consistently the channel if we do not have
permission to view&post.
This PR fixes status-im/status-desktop#14117
kounkou added a commit to status-im/status-go that referenced this issue Apr 8, 2024
We are now able to lock consistently the channel if we do not have
permission to view&post.
This PR fixes status-im/status-desktop#14117
kounkou added a commit to status-im/status-go that referenced this issue Apr 8, 2024
We are now able to lock consistently the channel if we do not have
permission to view&post.
This PR fixes status-im/status-desktop#14117
@kounkou
Copy link
Contributor

kounkou commented Apr 8, 2024

linking PR : status-im/status-go#5023

kounkou added a commit to status-im/status-go that referenced this issue Apr 8, 2024
We are now able to lock consistently the channel if we do not have
permission to view&post.
This PR fixes status-im/status-desktop#14117
kounkou added a commit to status-im/status-go that referenced this issue Apr 9, 2024
We are now able to lock consistently the channel if we do not have
permission to view&post.
This PR fixes status-im/status-desktop#14117
kounkou added a commit to status-im/status-go that referenced this issue Apr 9, 2024
We are now able to lock consistently the channel if we do not have
permission to view&post.
This PR fixes status-im/status-desktop#14117
kounkou added a commit to status-im/status-go that referenced this issue Apr 9, 2024
We are now able to lock consistently the channel if we do not have
permission to view&post.
This PR fixes status-im/status-desktop#14117
kounkou added a commit to status-im/status-go that referenced this issue Apr 9, 2024
We are now able to lock consistently the channel if we do not have
permission to view&post.
This PR fixes status-im/status-desktop#14117
@jrainville jrainville modified the milestones: 2.28.0 Beta, 2.28.1 Beta Apr 10, 2024
kounkou added a commit to status-im/status-go that referenced this issue Apr 10, 2024
…cked

We are now able to lock consistently the channel if we do not have
permission to view&post.
This PR fixes status-im/status-desktop#14117
kounkou added a commit to status-im/status-go that referenced this issue Apr 10, 2024
We are now able to lock consistently the channel if we do not have
permission to view&post.
This PR fixes status-im/status-desktop#14117
kounkou added a commit to status-im/status-go that referenced this issue Apr 10, 2024
We are now able to lock consistently the channel if we do not have
permission to view&post.
This PR fixes status-im/status-desktop#14117
kounkou added a commit to status-im/status-go that referenced this issue Apr 10, 2024
We are now able to lock consistently the channel if we do not have
permission to view&post.
This PR fixes status-im/status-desktop#14117
kounkou added a commit to status-im/status-go that referenced this issue Apr 10, 2024
We are now able to lock consistently the channel if we do not have
permission to view&post.
This PR fixes status-im/status-desktop#14117
@jrainville
Copy link
Member

@kounkou don't forget to create a cherry-pick for develop. Bonus points for bumping master to that merged develop afterwards

@jrainville
Copy link
Member

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment