hicommonwealth / commonwealth

A platform for decentralized communities
https://commonwealth.im
GNU General Public License v3.0
67 stars 44 forks source link

Group Checking Ticket (When to check a group) #4992

Closed zakhap closed 11 months ago

zakhap commented 1 year ago

When do we check a user's group membership?

Short answer is:

Longer answer is:

"Edge" Cases

Sequence Diagram

Please see the updated sequence diagram for a visual diagram of the user flow. (Above stated requirements take precedence over diagram, which may contain incidental errors WRT requirements.)

TTL Considerations

Open Questions

mzparacha commented 12 months ago

The OP has lots of questions, going through them and will leave thoughts here....

mzparacha commented 11 months ago

When the user signs in - We collect their group memberships across all communities they have joined. [endpoint: tbd]

Is this included in the scope of this ticket? If so can you please confirm if a new dedicated endpoint is planned for this?

When they land on a Community page that has groups. (TBC- to be confirmed)

I will use the GET /groups endpoint for this. Plz lmk if anything else is planned for this?

When they attempt any group-gated (write) action.

This seems very broad. Is content creation for threads/proposals of a gated group the only thing affected by this?

On content creation - We optimistically update UI based on group memberships collected in previous checks. [endpoint: content creation route]

Does this mean all content creation routes?

On content read - API (read) Request made → meaning the user has already signed in, at which point the membership check ran (“immediately”) and so when they land on a gated group for which they have the appropriate membership, that content is visible for them to read.

Just to understand this better. If a topic is gated, and a thread is created in that topic, only the member of that gated group will be able to view the thread?

Furthermore, when a user loses membership in a group, content they formerly had permission to view will no longer be accessible (?) but in theory would be cached locally by their client. Expectation is that shouldn't be a concern (they had permission to view it when their client cached it.) That being said, Product has stated membership revocation should trigger a purge of the particular cache. Open question whether that is feasible for MVP.

This will depend on the type of data/cache we are handling in frontend. To reduce complexity, I think it's better to handle this as a followup.

Are we refreshing membership status for user landing on a community page ("after" signing in)?

ATM, no

Are we only checking when they try to create content?

ATM, no.

Is "revoking membership purges client cache" feasible for MVP? Many related questions incl. how we handle that across devices.

I think it's better to handle this as a followup.

cc: @zakhap @ForestMars @jnaviask @rbennettcw

mzparacha commented 11 months ago

Added the blocked label, as i am unsure about some things (asked above)

mzparacha commented 11 months ago

Per discussion with @ForestMars, these are engineering questions for @rbennettcw.

cc: @rbennettcw

CowMuon commented 11 months ago

When. user signs in - We collect their group memberships across all communities they have joined.

Correct, this is so the client has the user's groups cached.

NB. that is mainly for performance and has no correctness guarantees, given their original session will not expire unless they are inactive for 7 days (extant), now 30 days (we are about to change, don't recall exact ticket # offhand.)

[endpoint: tbd]

Confirm with Ryan on endpoint pls

rbennettcw commented 11 months ago

Regarding membership refresh:

On-the-fly membership checks (topic level) are already implemented on the backend within the action routes:

Each membership has a TTL of 2 minutes (via hardcoded constant), and will only hit the TBC if a membership is stale/expired.

In terms of read access, the /refresh-memberships route also provides access info

The backend is effectively complete in terms of what's scoped in this ticket.

CowMuon commented 11 months ago

Given Ryan's reply Is this still blocked? @mzparacha

mzparacha commented 11 months ago

Yes, thank you both @CowMuon @rbennettcw.

On content creation - We optimistically update UI based on group memberships collected in previous checks. [endpoint: content creation route]

On-the-fly membership checks (topic level) are already implemented on the backend within the action routes: Create thread Create comment Create reaction

Per this, I will implement the frontend gating checks for creating threads, comments, and reactions.

mzparacha commented 11 months ago

Status

I summarized some actions items from the ticket description (those action items are updated in OP)

image

@zakhap Please lmk if you feel these are good? (asking since it looks like a product question)

Also for this question in OP

Furthermore, when a user loses membership in a group, content they formerly had permission to view will no longer be accessible (?) but in theory would be cached locally by their client. Expectation is that shouldn't be a concern (they had permission to view it when their client cached it.) That being said, Product has stated membership revocation should trigger a purge of the particular cache. Open question whether that is feasible for MVP.

Its totally doable, infact easier to do since we have react query setup, that can invalidate a query/data (let's say threads/topics) based on another query data (i.e /refresh-membership response) -- I made the action item 3 in ticket description for this.

cc: @CowMuon @zakhap @jnaviask

zakhap commented 11 months ago

@rbennettcw @mzparacha: I want to make clear what is and is not going into V1. When @CowMuon added the following line to Ticket above:

On content read - API (read) Request made → meaning the user has already signed in, at which point the membership check ran (“immediately”) and so when they land on a gated group for which they have the appropriate membership, that content is visible for them to read.

I want to make sure that we are NOT implementing READ access control in V1. All content on Commonwealth will continue to be visible to All users at this time. However, it is important to know whether or not the user is the requisite Group to participate (create threads, comments, reactions) on the page they are accessing. We have disabled states in the UI on "View Thread" Pages for commenting + reactions as well as helper callout on the "Create Thread" Page as well (#5397). It is for these UX affordances that we need to know what Group a user is in on Content Read.

@mzparacha in response to your last comment, these action items are not necessary in the present moment given that we are not making private (or hiding, or adding "Read access control") Topics at this time.

CowMuon commented 11 months ago

Thanks for the clarification here Zak, updated to indicate what's v1 and what's v2.