-
Notifications
You must be signed in to change notification settings - Fork 1.7k
[MM-68986][MM-69203] Add module to collect Session Attributes from Mobile App #9830
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
Changes from all commits
83a0e35
6c2f05c
11ae38d
0dce0d6
b2778dd
c39b637
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,51 @@ | ||
| // Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. | ||
| // See LICENSE.txt for license information. | ||
|
|
||
| import NetworkManager from '@managers/network_manager'; | ||
|
|
||
| import {fetchSessionAttributesManifest} from './session_attributes'; | ||
|
|
||
| jest.mock('@managers/network_manager'); | ||
| jest.mock('@utils/log'); | ||
|
|
||
| const serverUrl = 'https://chat.example.com'; | ||
| const manifest: SAField[] = [ | ||
| {name: 'os_platform', type: 'string', ttl_seconds: 0, grace_period_seconds: 0}, | ||
| ]; | ||
|
|
||
| describe('fetchSessionAttributesManifest', () => { | ||
| const mockClient = {getSessionAttributesManifest: jest.fn()}; | ||
|
|
||
| beforeEach(() => { | ||
| jest.clearAllMocks(); | ||
| (NetworkManager.getClient as jest.Mock).mockReturnValue(mockClient); | ||
| }); | ||
|
|
||
| it('should return the manifest from the client', async () => { | ||
| mockClient.getSessionAttributesManifest.mockResolvedValue(manifest); | ||
|
|
||
| const result = await fetchSessionAttributesManifest(serverUrl); | ||
|
|
||
| expect(result).toEqual({manifest}); | ||
| }); | ||
|
|
||
| it('should return the error when the client request fails', async () => { | ||
| const error = new Error('request failed'); | ||
| mockClient.getSessionAttributesManifest.mockRejectedValue(error); | ||
|
|
||
| const result = await fetchSessionAttributesManifest(serverUrl); | ||
|
|
||
| expect(result).toEqual({error}); | ||
| }); | ||
|
|
||
| it('should return the error when the client cannot be resolved', async () => { | ||
| const error = new Error('no client'); | ||
| (NetworkManager.getClient as jest.Mock).mockImplementation(() => { | ||
| throw error; | ||
| }); | ||
|
|
||
| const result = await fetchSessionAttributesManifest(serverUrl); | ||
|
|
||
| expect(result).toEqual({error}); | ||
| }); | ||
| }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,17 @@ | ||
| // Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved. | ||
| // See LICENSE.txt for license information. | ||
|
|
||
| import NetworkManager from '@managers/network_manager'; | ||
| import {getFullErrorMessage} from '@utils/errors'; | ||
| import {logDebug} from '@utils/log'; | ||
|
|
||
| export const fetchSessionAttributesManifest = async (serverUrl: string): Promise<{manifest?: SAField[]; error?: unknown}> => { | ||
| try { | ||
| const client = NetworkManager.getClient(serverUrl); | ||
| const manifest = await client.getSessionAttributesManifest(); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. so it seems we are fetching the manifest every time. a few questions:
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 3 is still a valid concern. The manifest is a list of SAField. How big is that list supposed to be? If it is small, we can ignore it, but if it is potentially big, we don't want to send 100 registries on every call to this endpoint. We can call
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think it is anymore, I made a change such that we aren't re-fetching the manifest every time when the property fields update, we are only checking what is coming from the websocket event and upserting to our store. The only time this runs is on reconnect, and while I suppose we could optimize this to be more efficient, we don't have a large list of attributes here (currently it's 19), and it would require us to completely rearchitect how the endpoint works (which means changes to server, desktop, all which are mostly finalized). To me, this seems like an improvement we could do later on, but likely isn't necessary right now.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As long as you strongly believe that "doing it later" is not going to bring any headaches, I am ok with it. But I leave you here some things to consider before making that decision:
If considering all this you still believe we can go ahead with things as they are, I am ok with it.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That change later is 100% needed, the changes in no re-fetching anytime are good, but I can see this being a headache in the future for not addressing now for whatever valid reasons we may think of |
||
| return {manifest}; | ||
| } catch (error) { | ||
| logDebug('error on fetchSessionAttributesManifest', getFullErrorMessage(error)); | ||
| return {error}; | ||
| } | ||
| }; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -21,6 +21,7 @@ import {isSupportedServerCalls} from '@calls/utils'; | |
| import {Screens} from '@constants'; | ||
| import DatabaseManager from '@database/manager'; | ||
| import AppsManager from '@managers/apps_manager'; | ||
| import SessionAttributesManager from '@managers/session_attributes_manager'; | ||
| import {handlePlaybookReconnect} from '@playbooks/actions/websocket/reconnect'; | ||
| import {getActiveServerUrl} from '@queries/app/servers'; | ||
| import {getLastPostInThread} from '@queries/servers/post'; | ||
|
|
@@ -64,6 +65,8 @@ async function doReconnect(serverUrl: string, groupLabel?: BaseRequestGroupLabel | |
| const {database} = operator; | ||
|
|
||
| try { | ||
| await SessionAttributesManager.refreshManifest(serverUrl); | ||
|
devinbinnie marked this conversation as resolved.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. At this point is too early.. the On top of that, the
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So to answer the question about the feature, yes every authenticated request should have session attributes, and this is by design. We could feasibility use session attributes to write policies that gate data access at any API endpoint, so if the values are missing and not cached on the server at the right time, the user could get denied access to something they should have. Right now, we are only using these for permission policies which is pretty limited, but we should be able to use these for anything we want going forward. Tech spec here if you'd like to see: https://mattermost.atlassian.net/wiki/spaces/ICT/pages/4559732738/WIP+Session+Attributes+v1.0+MVF?atl_p=eyJpIjoiM2Q1Mjc3ZDYtZWIxYy1hNWQyLWQ3YTItZTI5Y2Y5Y2RhMDEzIiwidCI6InRvcExlZnRWaWV3Q29udGFpbmVyIiwic291cmNlIjoiZW1haWwiLCJlIjoiY2Mtbm90aWZpY2F0aW9uc19iYXRjaF91cGRhdGUifQ
larkox marked this conversation as resolved.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Reconnect waits on a telemetry call before fetching any messages. When the websocket reconnects, the very first thing we do is wait for the session attributes manifest. Only after that finishes do we call So the order is: ask the server about session attributes → then go get the user's messages. If that manifest endpoint is slow, the user just stares at a stale channel while we're waiting on a call that has nothing to do with showing them their messages. Should we fire it without awaiting, or move it after the entry sync?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The reason it needs to be synchronous is so that theoretically, any endpoint we hit will make sure that the user has up-to-date session attributes before evaluating whether they have access to resources. This will become very important when we start using ABAC to control if a user can see a channel or not (with session attributes). |
||
|
|
||
| const lastFullSync = await getLastFullSync(database); | ||
| const now = Date.now(); | ||
|
|
||
|
|
||
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.
Both permissions go into every build, including servers that can never use the feature.
The app uses
react-native-permissionsin 18 places, but location isn't one of them. Also, Desktop shippedenableSessionAttributesin settings. Mobile has no way to opt out.Is this working as expected or should it be scoped?
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 think this is fine - this just means users will see the prompt and they can feel free to deny the permission.