SQSERVICES-1083 Separate Access Control For Guests And Services #2035

battermann · 2022-01-11T14:56:28Z

https://wearezeta.atlassian.net/browse/SQSERVICES-1083

Checklist

The PR Title explains the impact of the change.
The PR description provides context as to why the change should occur and what the code contributes to that effect. This could also be a link to a JIRA ticket or a Github issue, if there is one.
If a cassandra schema migration has been added, I ran make git-add-cassandra-schema to update the cassandra schema documentation.
changelog.d contains the following bits of information (details):
- A file with the changelog entry in one or more suitable sub-sections. The sub-sections are marked by directories inside changelog.d.

libs/wire-api-federation/src/Wire/API/Federation/API/Galley.hs

libs/wire-api/src/Wire/API/Conversation.hs

libs/wire-api/test/golden/Test/Wire/API/Golden/Generated/Event_user.hs

services/galley/src/Galley/API/Action.hs

services/galley/src/Galley/Data/Conversation/Types.hs

response contains both access roles wip conversion in ToSchema instance pushed old access roles to the boundaries wip schema migration update brig wip fix tests fix golden tests fix galley integration fix brig-integration fix wire-api-federation golden tests fix problems update cql schema ref use to/from access role legacy imported from wire-api roundtrip for access roles old and new (re-)added golden test for access role legacy (re-)added golden test for access role legacy access role conversion test (fails) wip wip clean up business logic

fisx · 2022-01-18T09:41:51Z

libs/wire-api/src/Wire/API/Conversation.hs

        schema
    <*> cnvmAccess .= field "access" (array schema)
-    <*> cnvmAccessRole .= field "access_role" schema
+    <*> cnvmAccessRoles .= accessRolesSchema


Doesn't this mean that both fields are required? I would use "optional" here and leave a description that explains that the server promises to always deliver both?

This is defined in the accessRolesSchema. When we deserialize, at least one of them has to be set, and when we serialize, we return both.

libs/wire-api/src/Wire/API/Conversation.hs

services/galley/src/Galley/Data/Conversation.hs

libs/wire-api/src/Wire/API/Conversation.hs

fisx · 2022-01-18T11:00:53Z

libs/wire-api/test/unit/Test/Wire/API/Conversation.hs

+    [ fromLegacyToV2ToLegacy,
+      fromV2ToLegacyToV2


Suggested change

[ fromLegacyToV2ToLegacy,

fromV2ToLegacyToV2

[ acessRoleFromLegacyToV2ToLegacy,

acessRoleFromV2ToLegacyToV2

(Also, it's probably much faster if we don't run QC here but enumerate all test cases exhaustively. But I think we should probably resist, and do that more exhaustively once we don't have anything better to do.)

I like the renaming.
Regarding QC, all wire-api unit tests together take 1.5s to run. I don't see a problem.
Checking properties has the benefit that if we add additional access roles later, they are automatically checked.
We can't ensure exhaustiveness on sets anyway, AFAIK.

libs/wire-api/src/Wire/API/Conversation.hs

libs/wire-api/test/unit/Test/Wire/API/Conversation.hs

services/brig/src/Brig/Provider/API.hs

fisx

...

services/galley/src/Galley/API/Action.hs

services/galley/src/Galley/API/Create.hs

services/galley/src/Galley/Data/Conversation/Types.hs

Co-authored-by: fisx <[email protected]>

services/galley/src/Galley/API/Action.hs

services/galley/test/integration/API/Util.hs

services/galley/test/integration/API.hs

fisx · 2022-01-18T15:04:02Z

services/galley/test/integration/API.hs

  (owner, teamId, []) <- Util.createBindingTeamWithNMembers 0
  let createConvWithGuestLink = do
-        convId <- decodeConvId <$> postTeamConv teamId owner [] (Just "testConversation") [CodeAccess] (Just ActivatedAccessRole) Nothing
+        convId <- decodeConvId <$> postTeamConv teamId owner [] (Just "testConversation") [CodeAccess] (Just (Set.fromList [TeamMemberAccessRole, NonTeamMemberAccessRole])) Nothing


this looks like the module where you can test the new cases (in particular services-not-guests, and guests-not-services). maybe this can be even randomized somehow?

libs/wire-api/test/unit/Test/Wire/API/Conversation.hs

libs/wire-api/src/Wire/API/Conversation.hs

services/galley/src/Galley/Cassandra/Instances.hs

fisx · 2022-01-18T20:23:47Z

services/galley/src/Galley/API/Action.hs

+          then pure bm
+          else do
+            activated <- map User.userId <$> E.lookupActivatedUsers (toList (bmLocals bm))
+            -- FUTUREWORK: should we also remove non-activated remote users?


Federation people: can users joining via a guest link without account be non-local?

services/galley/src/Galley/API/Util.hs

fisx

#2057 should (does?) provide some tests that are essential for #2045, but you may have tested this manually? otherwise LGTM!

fisx · 2022-01-23T08:47:21Z

libs/wire-api/src/Wire/API/Conversation.hs

+        \`[team_member]` => `team` - team-only conversation\
+        \`[team_member, non_team_member, service]` => `activated` - conversation for users who have activated email, phone or SSO and services\
+        \`[team_member, non_team_member, service, guest]` => `non_activated` - all allowed, no checks.\
+        \All other configurations of `access_role_v2` are mapped to the smallest superset containing all given access roles."


Would it make more sense to document the mapping in the deprecated type, then they will be deleted together?

fisx · 2022-01-23T08:48:52Z

changelog.d/0-release-notes/pr-2035

@@ -0,0 +1 @@
+Backend now separates conversation access control for guests and services. The old access roles are still supported but it is encouraged to upgrade clients since mapping between the old access roles and the new access roles are is not isomorphic. For more details refer to the API changes changelog, the Swagger docs, or the technical specification.


If we already know which versions of clients support this, we should add those here, but I suspect we're not certain yet. Bring up in the next standup?

…or-guests-and-services

fisx reviewed Jan 12, 2022

View reviewed changes

battermann force-pushed the SQSERVICES-1083-separate-access-control-for-guests-and-services branch from 95863d4 to 93f04a6 Compare January 13, 2022 15:45

battermann force-pushed the SQSERVICES-1083-separate-access-control-for-guests-and-services branch from 6007754 to 46eecd4 Compare January 17, 2022 14:47

battermann added 2 commits January 17, 2022 15:35

impl parseAccessRoles

3b586f4

correct column name

d568a10

battermann requested a review from fisx January 17, 2022 15:44

fisx reviewed Jan 18, 2022

View reviewed changes

battermann and others added 5 commits January 18, 2022 11:52

integration tests brig pass

0603d1e

added comment

d854c6f

Update services/galley/src/Galley/Data/Conversation.hs

a74f4d6

Co-authored-by: fisx <[email protected]>

clean up

92619cc

clean up, refactoring, renaming

ff5ca7d

battermann marked this pull request as ready for review January 18, 2022 12:53

test lower bounds

dfff900

fisx reviewed Jan 18, 2022

View reviewed changes

battermann added 2 commits January 18, 2022 15:46

added docs

e19c360

fix issues and pr comments

fd088b1

battermann requested a review from fisx January 18, 2022 16:17

fisx reviewed Jan 18, 2022

View reviewed changes

services/galley/src/Galley/API/Util.hs Outdated Show resolved Hide resolved

services/galley/src/Galley/API/Util.hs Outdated Show resolved Hide resolved

fisx mentioned this pull request Jan 18, 2022

Additional/Updated Tests for Conversation Access Control (#2035) #2057

Merged

battermann added 6 commits January 19, 2022 10:16

fix pr issues

740fcf6

correct access roles schema

759906c

docs for access role v2

c3ee505

changelog

7335810

identify guests and service users correctly

eab1613

renaming

80c5556

battermann requested a review from fisx January 19, 2022 12:33

fix golden tests

0a0fca2

fisx approved these changes Jan 23, 2022

View reviewed changes

moved mapping docs to old access role

7cbfd50

fisx mentioned this pull request Jan 25, 2022

Fix (some) HLint issues in wire-api #2065

Merged

4 tasks

Merge branch 'develop' into SQSERVICES-1083-separate-access-control-f…

dc0b834

…or-guests-and-services

fisx merged commit d1df63f into develop Jan 25, 2022

fisx deleted the SQSERVICES-1083-separate-access-control-for-guests-and-services branch January 25, 2022 11:38

fisx mentioned this pull request Jan 27, 2022

Release 2022-01-27 #2080

Merged

battermann pushed a commit that referenced this pull request Jan 28, 2022

Additional/Updated Tests for Conversation Access Control (#2035) (#2057)

3f7e555

		@@ -0,0 +1 @@
		Backend now separates conversation access control for guests and services. The old access roles are still supported but it is encouraged to upgrade clients since mapping between the old access roles and the new access roles are is not isomorphic. For more details refer to the API changes changelog, the Swagger docs, or the technical specification.

SQSERVICES-1083 Separate Access Control For Guests And Services #2035

SQSERVICES-1083 Separate Access Control For Guests And Services #2035

Uh oh!

Conversation

battermann commented Jan 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

fisx left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

fisx left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

battermann commented Jan 11, 2022 •

edited

Loading