-
Notifications
You must be signed in to change notification settings - Fork 246
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
refactor: EventSenders forward RequestToJoin decision to control node #3843
Conversation
Jenkins BuildsClick to see older builds (48)
|
@@ -37,6 +37,8 @@ const ( | |||
ActivityCenterMembershipStatusPending | |||
ActivityCenterMembershipStatusAccepted | |||
ActivityCenterMembershipStatusDeclined | |||
ActivityCenterMembershipStatusAcceptedPending | |||
ActivityCenterMembershipStatusDeclinedPending |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@status-im/desktop-devs these are new states that desktop UI should be aware of
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am wondering why we need this notification. Isn't pending state shared only among admins and control node? If that's the case, I believe we don't need a notification for that 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How we should show those states in the UI?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Everything related to UI is discussed here: status-im/status-desktop#11649
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should it be ...AcceptPending
and ..DeclinePending
(without ed
)?
Or am I misunderstanding smth?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well the state is in "accepted pending" meaning, an admin has accepted the request but the decision is still pending until a control node confirms. Not sure if -ed
is an issue here
protocol/communities/community.go
Outdated
if isControlNode { | ||
o.removeMemberFromOrg(pk) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is moved into the condition because admins should not be able to remove users like that, We'll need a pending state for this.
For now, we at least prevent the member to be removed by the admin
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Undoing this as I'm not handling kicking/banning members in this PR.
|
||
if !isControlNode && !allowedToSendEvents { | ||
if !isControlNode { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Admins should not make changes to members directly, so only control node is allowed to do stuff here.
Also, we want to move away from attaching revealed addresses to members in the community description (as this is public metadata).
Next step is to remove usage of this function entirely.
@@ -160,7 +160,6 @@ func (o *Community) ToCommunityRequestToJoinAcceptCommunityEvent(changes *Commun | |||
return &CommunityEvent{ | |||
CommunityEventClock: o.NewCommunityEventClock(), | |||
Type: protobuf.CommunityEvent_COMMUNITY_REQUEST_TO_JOIN_ACCEPT, | |||
MembersAdded: changes.MembersAdded, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need attach MembersAdded
if admins can't actually do that anymore.
Now we're just signalling which request to join is marked as accepted
return err | ||
} | ||
o.removeMemberFromOrg(pk) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removal/addition of members via admin events is now done later in handleAdditionalAdminChanges()
where the control node makes the ultimate decision
} | ||
} | ||
if community.IsControlNode() { | ||
m.publish(&Subscription{AcceptedRequestsToJoin: acceptedRequestsToJoin}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This event causes Messenger
to AcceptCommunityRequestToJoin()
for all accepted requests
} | ||
|
||
// TODO: Don't add member to community, instead store revealed accounts in database | ||
_, err = community.AddMemberWithRevealedAccounts(dbRequest, memberRoles, revealedAccounts) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs to be removed as part of status-im/status-desktop#11573
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Happening in #3853
@@ -16,6 +16,8 @@ const ( | |||
RequestToJoinStateDeclined | |||
RequestToJoinStateAccepted | |||
RequestToJoinStateCanceled | |||
RequestToJoinStateAcceptedPending | |||
RequestToJoinStateDeclinedPending |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@status-im/desktop-devs also new states we probably want to use in the UI
m.logger.Warn("failed to accept request to join ", zap.Error(err)) | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will be triggered as a side effect in HandleCommunityEventsMessage
when there were accepted requests to join
5bc0083
to
8bee717
Compare
8bee717
to
bf60cd6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work.
Left few comments
return nil | ||
} | ||
|
||
func (m *Manager) handleCommunityEventRequestRejected(community *Community, communityEvent *CommunityEvent) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does the control node need to notify privileged users about the declined request, as you do for an accepted one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They should notify the privileged users yes, so that they can delete the request and stop being in the pending state.
We still shouldn't send it to the user requesting however.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good point. I also just realized we're not updating other admins to update their pending state on accepted either.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After a few discussions with @osmaczko , this signal should be part of status-im/status-desktop#11715, so that has to land first.
protocol/communities_events_owner_without_community_key_test.go
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work, although I have few questions/uncertainties.
@@ -37,6 +37,8 @@ const ( | |||
ActivityCenterMembershipStatusPending | |||
ActivityCenterMembershipStatusAccepted | |||
ActivityCenterMembershipStatusDeclined | |||
ActivityCenterMembershipStatusAcceptedPending | |||
ActivityCenterMembershipStatusDeclinedPending |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am wondering why we need this notification. Isn't pending state shared only among admins and control node? If that's the case, I believe we don't need a notification for that 🤔
if existingRequestToJoin != nil { | ||
// node already knows about this request to join, so let's compare clocks | ||
// and update it if necessary | ||
if existingRequestToJoin.Clock <= requestToJoin.Clock { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if existingRequestToJoin.Clock <= requestToJoin.Clock { | |
if existingRequestToJoin.Clock < requestToJoin.Clock { |
I know it was like that before, but I believe same clock should be ignored.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As discussed in https://github.com/status-im/status-go/pull/3843/files#r1282298019 we expect it to be the same clock, so we don't want to ignore this
protocol/communities/manager.go
Outdated
memberRoles = []protobuf.CommunityMember_Roles{protobuf.CommunityMember_ROLE_ADMIN} | ||
} | ||
|
||
// TODO: Don't add member to community, instead store revealed accounts in database |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure I got it right. When/where do we add members to community then?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They need to be store in a separated database table instead of the community descriptoin so they aren't shared with public metadata.
Then, revealed addresses only locally known need to be backed up on waku such that control nodes can recover them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They need to be store in a separated database table instead of the community descriptoin so they aren't shared with public metadata.
How will clients then construct the member list?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah sorry, the comment has a mistake. This is not about the members but about the revealed addresses
// Don't check permissions here, | ||
// it will be done further in the processing pipeline. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment could still be useful.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay will undo this
ecee890
to
8b64785
Compare
27fdfb4
to
7c2f6d3
Compare
0c2b655
to
a58b6bd
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very good work. I had 2 questions.
} | ||
if !permissionsSatisfied { | ||
requestToJoin.State = RequestToJoinStateDeclined | ||
if community.IsControlNode() && len(request.RevealedAccounts) > 0 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if len(request.RevealedAccounts) == 0
and the community has permissions to join, we should auto set it to RequestToJoinStateDeclined
. I'm not sure if the code does this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah actually, the check on len(request.RevealedAccounts)
is a left over but it probably wouldn't hurt. It ends up being marked as Declined
a few line below latest after the satisfaction check was done
if err != nil { | ||
m.logger.Warn("failed to accept request to join ", zap.Error(err)) | ||
} | ||
// TODO INFORM ADMINS |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are those TODOs done or to be done in another PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs to be done as part of status-im/status-desktop#11715
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work! And no questions from my side :)
This is a bigger change in how community membership requests are handled among admins, token masters, owners, and control nodes. Prior to this commit, all privileged users, also known as `EventSenders`, were able to accept and reject community membership requests and those changes would be applied by all users. This commit changes this behaviour such that: 1. EventSenders can make a decision (accept, reject), but merely forward their decision to the control node, which ultimately has to confirm it 2. EventSenders are no longer removing or adding members to and from communities 3. When an eventsender signaled a decision, the membership request will enter a pending state (acceptedPending or rejectedPending) 4. Once a decision was made by one eventsender, no other eventsender can override that decision This implementation is covered with a bunch of tests: - Ensure that decision made by event sender is shared with other event senders - `testAcceptMemberRequestToJoinResponseSharedWithOtherEventSenders()` - `testRejectMemberRequestToJoinResponseSharedWithOtherEventSenders()` - Ensure memebrship request stays pending, until control node has confirmed decision by event senders - `testAcceptMemberRequestToJoinNotConfirmedByControlNode()` - `testRejectMemberRequestToJoinNotConfirmedByControlNode()` - Ensure that decision made by event sender cannot be overriden by other event senders - `testEventSenderCannotOverrideRequestToJoinState()` These test cases live in three test suites for different event sender types respectively - `OwnerWithoutCommunityKeyCommunityEventsSuite` - `TokenMasterCommunityEventsSuite` - `AdminCommunityEventsSuite` In addition to the changes mentioned above, there's also a smaller changes that ensures membership requests to *not* attached revealed wallet addresses when the requests are sent to event senders (in addition to control nodes). Requests send to a control node will still include revealed addresses as the control node needs them to verify token permissions. This commit does not yet handle the case of event senders attempting to kick and ban members. Similar to accepting and rejecting membership requests, kicking and banning need a new pending state. However, we don't track such state in local databases yet so those two cases will be handled in future commit to not have this commit grow larger.
a58b6bd
to
a4dc80d
Compare
There is one UI requirement that might need backed support and I can't spot it in this PR: Only the admin that made the initial decision can change the decision. (Other admins will not see the alternate button on hover).. Since only the initiator can see the alternate button and change the membership request state after accepting/rejecting it, we need to identify the source of membership state change. Was it considered for this PR? |
@alexjba this is indeed not covered in this PR. The PR assumes that, once a state has been introduced by an admin, it can no longer be changed. @jrainville this would need change in the database and a migration script. Should this happen in this PR as well or should we create a separate issue for it so this can be tackled in follow-up work? |
It can be done in another ticket |
@jrainville added an issue describing what needs to be done here: status-im/status-desktop#11842 Might be a good task for Teodor to get his feet wet with status-go |
This is a bigger change in how community membership requests are handled among admins, token masters, owners, and control nodes.
Prior to this commit, all privileged users, also known as
EventSenders
, were able to accept and reject community membership requests and those changes would be applied by all users.This commit changes this behaviour such that:
This implementation is covered with a bunch of tests:
testAcceptMemberRequestToJoinResponseSharedWithOtherEventSenders()
testRejectMemberRequestToJoinResponseSharedWithOtherEventSenders()
testAcceptMemberRequestToJoinNotConfirmedByControlNode()
testRejectMemberRequestToJoinNotConfirmedByControlNode()
testEventSenderCannotOverrideRequestToJoinState()
These test cases live in three test suites for different event sender types respectively
OwnerWithoutCommunityKeyCommunityEventsSuite
TokenMasterCommunityEventsSuite
AdminCommunityEventsSuite
In addition to the changes mentioned above, there's also a smaller changes that ensures membership requests to not attached revealed wallet addresses when the requests are sent to event senders (in addition to control nodes).
Requests send to a control node will still include revealed addresses as the control node needs them to verify token permissions.
This commit does not yet handle the case of event senders attempting to kick and ban members.
Similar to accepting and rejecting membership requests, kicking and banning need a new pending state. However, we don't track such state in local databases yet so those two cases will be handled in future commit to not have this commit grow larger.