[MM-68986][MM-69203] Add module to collect Session Attributes from Mobile App - #9830
Conversation
devinbinnie
added
2: Dev Review
Documentation Impact Analysis — updates neededDocumentation Impact AnalysisOverall Assessment: Documentation Updates Recommended Changes SummaryThis PR introduces a Documentation Impact Details
Recommended Actions
ConfidenceMedium — The server-side session attributes and ABAC infrastructure is already documented in the v11 changelog and the admin guide. The gaps identified here are mobile-specific: the new OS permission requests and their conditional trigger are not covered anywhere in the current docs, and admins managing enrolled devices via MDM/EMM will need this information. The feature flag / license-driven activation is also not described in existing mobile documentation. |
Coverage Comparison Report |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds end-to-end session attributes support. The change defines manifest contracts, manages server and device data, synchronizes websocket updates, refreshes state during lifecycle events, and forwards outbound headers through Android and iOS networking paths. ChangesSession Attributes Integration
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant App
participant SessionAttributesManager
participant WebSocket
participant ClientUsers
participant NativeNetwork
App->>SessionAttributesManager: syncStaticValues()
WebSocket->>SessionAttributesManager: refreshManifest(serverUrl)
SessionAttributesManager->>ClientUsers: fetch session attributes manifest
ClientUsers-->>SessionAttributesManager: SAField[] manifest
NativeNetwork->>SessionAttributesManager: get outbound header
SessionAttributesManager-->>NativeNetwork: X-MM-Session-Attributes
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (7)
app/managers/session_attributes_manager/collector/index.ts (7)
13-15: ⚡ Quick winPrefer nullish coalescing (
??) for the fallback.As per coding guidelines, use
??instead of||for fallbacks to avoid converting falsy values incorrectly.♻️ Proposed fix
getOSVersion() { - return osVersion || ''; + return osVersion ?? ''; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/managers/session_attributes_manager/collector/index.ts` around lines 13 - 15, The getOSVersion method uses the logical OR fallback (return osVersion || '') which can incorrectly treat valid falsy values as absent; update getOSVersion to use the nullish coalescing operator (return osVersion ?? '') so only null or undefined trigger the fallback, keeping the method behavior the same otherwise and referencing the getOSVersion function and the osVersion variable.Source: Coding guidelines
66-82: 💤 Low valueAdd explicit return type annotation.
For clarity, add
: stringreturn type to the helper method.♻️ Proposed fix
-protected mapNetInfoType(state: NetInfoState) { +protected mapNetInfoType(state: NetInfoState): string { switch (state.type) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/managers/session_attributes_manager/collector/index.ts` around lines 66 - 82, The helper method mapNetInfoType lacks an explicit return type; update its signature (mapNetInfoType) to declare a return type of string (e.g., add ": string") so the function signature clearly indicates it returns a string and helps with type checking and readability while leaving the existing switch behavior intact.
42-45: 💤 Low valueAdd explicit return type annotation.
For consistency and type safety, add
: Promise<string>return type annotation.♻️ Proposed fix
-async getClientIPAddress() { +async getClientIPAddress(): Promise<string> { const state = await NetInfo.fetch(); return this.extractIpAddress(state); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/managers/session_attributes_manager/collector/index.ts` around lines 42 - 45, The method getClientIPAddress lacks an explicit return type; update its signature to include an explicit Promise<string> return type (i.e., change getClientIPAddress() to getClientIPAddress(): Promise<string>) and ensure the returned value from this.extractIpAddress(state) is compatible with string (adjust extractIpAddress’s return type if needed) so the compiler enforces type safety.
84-90: 💤 Low valueAdd explicit return type annotation.
For clarity, add
: stringreturn type to the helper method.♻️ Proposed fix
-protected extractIpAddress(state: NetInfoState) { +protected extractIpAddress(state: NetInfoState): string { const details = state.details;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/managers/session_attributes_manager/collector/index.ts` around lines 84 - 90, The helper method extractIpAddress currently lacks an explicit return type; update its signature to declare a string return type (i.e., add : string) so the function signature reads with a clear return type, keeping the implementation unchanged; locate the extractIpAddress method in the class (method name extractIpAddress and parameter NetInfoState) and add the explicit return type annotation.
37-40: 💤 Low valueAdd explicit return type annotation.
For consistency and type safety, add
: Promise<string>return type annotation.♻️ Proposed fix
-async getNetworkInterfaceType() { +async getNetworkInterfaceType(): Promise<string> { const state = await NetInfo.fetch(); return this.mapNetInfoType(state); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/managers/session_attributes_manager/collector/index.ts` around lines 37 - 40, The getNetworkInterfaceType method lacks an explicit return type; update its signature to declare a Promise<string> return type and ensure the implementation still returns a string via this.mapNetInfoType(state) so the signature matches; locate the async method getNetworkInterfaceType in the collector index and add ": Promise<string>" to the method declaration.
47-54: 💤 Low valueAdd explicit return type annotations.
Both methods should have explicit return types for consistency:
getIsVpnActive(): Promise<string>andgetOSPlatform(): string.♻️ Proposed fix
-async getIsVpnActive() { +async getIsVpnActive(): Promise<string> { const state = await NetInfo.fetch(); return state.type === NetInfoStateType.vpn ? 'true' : 'false'; } -getOSPlatform() { +getOSPlatform(): string { return Platform.OS; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/managers/session_attributes_manager/collector/index.ts` around lines 47 - 54, The two methods lack explicit return type annotations; update the method signatures for getIsVpnActive to declare a Promise<string> return type and getOSPlatform to declare a string return type (i.e., change getIsVpnActive() to getIsVpnActive(): Promise<string> and getOSPlatform() to getOSPlatform(): string) so their declarations are explicit and consistent with their returned values.
33-35: ⚡ Quick winAdd explicit return type annotation.
The manager's
collectAttributemethod expects all attribute getters to returnPromise<string>, but this method's return type is inferred. Add an explicit: Promise<string>annotation to ensure type safety and clarity.♻️ Proposed fix
-async getClientDeviceId() { +async getClientDeviceId(): Promise<string> { return getDeviceToken(); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/managers/session_attributes_manager/collector/index.ts` around lines 33 - 35, The getClientDeviceId method currently has an inferred return type; add an explicit Promise<string> return annotation to match the expectations of collectAttribute and ensure type safety by changing the signature of getClientDeviceId to async getClientDeviceId(): Promise<string> (keeping the existing body that returns getDeviceToken()); verify no other callers rely on a different signature.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/actions/websocket/users.ts`:
- Around line 211-222: The handler handleSessionAttributesPropertyFieldEvent is
synchronous but calls the async
SessionAttributesManager.refreshManifest(serverUrl) without awaiting it, so
exceptions escape the try/catch and it behaves fire-and-forget; make
handleSessionAttributesPropertyFieldEvent an async function (returning
Promise<void>) and await SessionAttributesManager.refreshManifest(serverUrl)
inside the existing try block so any errors are caught by the catch and behavior
matches other handlers like handleUserUpdatedEvent.
In `@app/managers/session_attributes_manager/index.ts`:
- Line 57: The log message incorrectly names the method as fetchManifest; update
the debug call inside SessionAttributesManager.refreshManifest to reference the
correct method name so the log reads
'[SessionAttributesManager.refreshManifest]'. Locate the logDebug invocation
that passes getFullErrorMessage(error) and change the static string token to
reflect refreshManifest, keeping the rest of the call intact.
- Around line 84-90: The loop updates state.lastSentAt for every field even when
collectAttribute(field.name, serverUrl) returns an empty value and the attribute
is not added to payload; change the logic in the Promise.all mapped async
function inside the SessionAttributesManager (the block iterating over
fieldsToSend) so that state.lastSentAt.set(field.name, now) is executed only
when value is non-empty and payload[field.name] is assigned (i.e., move the
lastSentAt update into the same conditional that checks if (value) so
failed/empty collections do not advance the TTL).
- Around line 66-99: The getOutboundHeader method can leave state.isSending true
if collectAttribute throws; wrap the Promise.all block in a try/finally so
state.isSending is always reset (set state.isSending = true before collection,
perform the await Promise.all(...) inside try, and set state.isSending = false
in finally). Ensure you still update state.lastSentAt inside the try only after
successful attribute reads (as currently done using collectAttribute) and keep
the rest of the payload/return logic unchanged.
In `@types/api/session_attributes.d.ts`:
- Around line 4-9: The SAField type defined in types/api/session_attributes.d.ts
is not exported but is referenced elsewhere (e.g., app/client/rest/users.ts),
causing TypeScript errors; update the declaration for SAField to be exported by
adding the export keyword (export type SAField = { ... }) so other modules can
import and use the type; ensure the exported name matches all usages (SAField)
across the codebase.
---
Nitpick comments:
In `@app/managers/session_attributes_manager/collector/index.ts`:
- Around line 13-15: The getOSVersion method uses the logical OR fallback
(return osVersion || '') which can incorrectly treat valid falsy values as
absent; update getOSVersion to use the nullish coalescing operator (return
osVersion ?? '') so only null or undefined trigger the fallback, keeping the
method behavior the same otherwise and referencing the getOSVersion function and
the osVersion variable.
- Around line 66-82: The helper method mapNetInfoType lacks an explicit return
type; update its signature (mapNetInfoType) to declare a return type of string
(e.g., add ": string") so the function signature clearly indicates it returns a
string and helps with type checking and readability while leaving the existing
switch behavior intact.
- Around line 42-45: The method getClientIPAddress lacks an explicit return
type; update its signature to include an explicit Promise<string> return type
(i.e., change getClientIPAddress() to getClientIPAddress(): Promise<string>) and
ensure the returned value from this.extractIpAddress(state) is compatible with
string (adjust extractIpAddress’s return type if needed) so the compiler
enforces type safety.
- Around line 84-90: The helper method extractIpAddress currently lacks an
explicit return type; update its signature to declare a string return type
(i.e., add : string) so the function signature reads with a clear return type,
keeping the implementation unchanged; locate the extractIpAddress method in the
class (method name extractIpAddress and parameter NetInfoState) and add the
explicit return type annotation.
- Around line 37-40: The getNetworkInterfaceType method lacks an explicit return
type; update its signature to declare a Promise<string> return type and ensure
the implementation still returns a string via this.mapNetInfoType(state) so the
signature matches; locate the async method getNetworkInterfaceType in the
collector index and add ": Promise<string>" to the method declaration.
- Around line 47-54: The two methods lack explicit return type annotations;
update the method signatures for getIsVpnActive to declare a Promise<string>
return type and getOSPlatform to declare a string return type (i.e., change
getIsVpnActive() to getIsVpnActive(): Promise<string> and getOSPlatform() to
getOSPlatform(): string) so their declarations are explicit and consistent with
their returned values.
- Around line 33-35: The getClientDeviceId method currently has an inferred
return type; add an explicit Promise<string> return annotation to match the
expectations of collectAttribute and ensure type safety by changing the
signature of getClientDeviceId to async getClientDeviceId(): Promise<string>
(keeping the existing body that returns getDeviceToken()); verify no other
callers rely on a different signature.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3aca214b-0d20-4011-9586-a94459674eeb
📒 Files selected for processing (31)
app/actions/remote/entry/login.tsapp/actions/remote/file.test.tsapp/actions/remote/file.tsapp/actions/remote/user.test.tsapp/actions/remote/user.tsapp/actions/websocket/event.tsapp/actions/websocket/index.test.tsapp/actions/websocket/index.tsapp/actions/websocket/system.test.tsapp/actions/websocket/system.tsapp/actions/websocket/users.test.tsapp/actions/websocket/users.tsapp/client/rest/constants.tsapp/client/rest/files.test.tsapp/client/rest/files.tsapp/client/rest/tracking.test.tsapp/client/rest/tracking.tsapp/client/rest/users.tsapp/constants/session_attributes.tsapp/constants/websocket.tsapp/managers/draft_upload_manager/index.test.tsapp/managers/draft_upload_manager/index.tsapp/managers/session_attributes_manager/collector/index.test.tsapp/managers/session_attributes_manager/collector/index.tsapp/managers/session_attributes_manager/index.test.tsapp/managers/session_attributes_manager/index.tsapp/managers/session_manager.test.tsapp/managers/session_manager.tsapp/screens/channel_bookmark/components/bookmark_file/bookmark_file.tsxtypes/api/config.d.tstypes/api/session_attributes.d.ts
enahum
left a comment
enahum
left a comment
There was a problem hiding this comment.
A few pointers to improve on this, happy to discuss if you disagree on anything
| return {error: clData.error}; | ||
| } | ||
|
|
||
| await SessionAttributesManager.refreshManifest(serverUrl); |
There was a problem hiding this comment.
looks like this is being called too early as you are calling it regardless if the check for the credentials just after this, and is also being executed twice, once here and once when the WS calls doReconnect.
Also, can this be fire and forget? if not why not ?
There was a problem hiding this comment.
Fetching it extra times doesn't hurt, we do need to ensure that it is there otherwise the app won't know which session attributes to send and when. And we don't want to default to sending them all all the time or server performance will degrade.
We could fire and forget but then the error case would mean session attributes are never sent and the user would not have access to certain resources/permissions.
Given all that, where is the best place to do this call correctly? I wasn't fully sure.
| prepareRequestHeaders = async (requestMethod: string) => { | ||
| const headers = this.getRequestHeaders(requestMethod); | ||
|
|
||
| if (headers[ClientConstants.HEADER_AUTH]) { |
There was a problem hiding this comment.
have you confirm that this true for all the requests? I ask because the network library injects the Authorization token on the native side for all requests that match the url but I'm unsure the Authorization header is added for all requests
There was a problem hiding this comment.
Which native requests might we need to inject this for? I assume this matches most of the applications functionality and should make sure the attributes are kept up to date. Is there any native function that gets data that might require some kind of permission?
There was a problem hiding this comment.
Looking at Daniel's comment, replying to a push notification might be the only one that needs this since we feasibly might have permissions around being able to post in a channel based on session attributes.
This is where my mobile knowledge falls short - can we somehow use the same hooks in the native code that we're using in react-native so we don't need to duplicate all of the gathering and header code?
There was a problem hiding this comment.
Ok let me explain here.
In cases where the request does not include an Authorization header, the native network layer would check that the request is being made to a server where the Authorization header is stored in the keychain and will include it, that way we really remove that burden from the JS side, it does apply more commonly for images and videos being loaded.
So the reality is that here you are gating the session headers when the request includes an Authorization token, and I don't know if that is the correct gate, specially as I said for things like images and videos.
To answer your question about native side, we do perform some requests from native, for push notifications and the Share Extension on both Android and iOS. (the extension for Android uses the code in libraries/@mattermost/rnshare, these requests are not including the headers you are adding here and NO, they do NOT share the same code, you need to write the code in Swift or Objective-C for iOS and write the code in Kotlin or Java from Android. OR you could dive into the react-native-network-client library, see what is done with the Authorization header and potentially do something similar so that every requests includes whatever session attributes you set from JS which is probably WORTH looking into.
If you want to explore this network-client approach, I'm more than happy to have a convo and see what can we do, considering the fact that we also open sourced the library and is being used by many.
edgarbellot
left a comment
edgarbellot
left a comment
There was a problem hiding this comment.
@devinbinnie looking good! I only have one suggestion
larkox
left a comment
larkox
left a comment
There was a problem hiding this comment.
Apart of Elias' comments, we haven't done anything on the native side (Push Notifications, Share Extension, and Reply from notification features). Those will not add the correct headers.
That being said, I think after Elias' proposed changes, the PR will be quite different, so feel free to re-request my review.
mattermost-build
added
E2E/Run
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/managers/session_attributes_manager/index.ts (1)
34-68: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winAwait cache priming before saving the manifest.
getOutboundHeader()can run beforeNetInfo.fetch(),isRootedExperimentalAsync(), orgetDeviceToken()resolve, so the first request can mark empty/default security attributes as sent and suppress the real values until the TTL window ends.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/managers/session_attributes_manager/index.ts` around lines 34 - 68, `SessionAttributesManager.refreshManifest` is saving the manifest before the security-attribute cache has finished priming, which lets `getOutboundHeader()` send empty/default values first and cache them as sent. Update the manifest refresh flow to wait for the cache priming work triggered by `NetInfo.fetch()`, `isRootedExperimentalAsync()`, and `getDeviceToken()` before calling `this.servers.set(...)`. Keep the existing early-return behavior, but ensure the priming promise(s) are awaited before the manifest is stored so the first header generation uses the real values.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@app/managers/session_attributes_manager/index.ts`:
- Around line 34-68: `SessionAttributesManager.refreshManifest` is saving the
manifest before the security-attribute cache has finished priming, which lets
`getOutboundHeader()` send empty/default values first and cache them as sent.
Update the manifest refresh flow to wait for the cache priming work triggered by
`NetInfo.fetch()`, `isRootedExperimentalAsync()`, and `getDeviceToken()` before
calling `this.servers.set(...)`. Keep the existing early-return behavior, but
ensure the priming promise(s) are awaited before the manifest is stored so the
first header generation uses the real values.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9a4198d3-f4dd-4def-91d5-3ff44f228a6b
📒 Files selected for processing (12)
app/actions/remote/file.test.tsapp/actions/remote/file.tsapp/actions/remote/session_attributes.test.tsapp/actions/remote/session_attributes.tsapp/actions/remote/user.tsapp/client/rest/files.test.tsapp/client/rest/files.tsapp/client/rest/tracking.test.tsapp/client/rest/tracking.tsapp/client/rest/users.tsapp/managers/session_attributes_manager/index.test.tsapp/managers/session_attributes_manager/index.ts
✅ Files skipped from review due to trivial changes (2)
- app/client/rest/files.ts
- app/actions/remote/user.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- app/client/rest/users.ts
- app/client/rest/tracking.test.ts
mattermost-build
added
E2E/Run
mattermost-build
added
E2E/Run
devinbinnie
changed the title
mattermost-build
removed
the
E2E/Run
mattermost-build
added
the
E2E/Run
|
❌ E2E Test Setup Failed Failed to create E2E test instances: installation wait cancelled: context canceled |
|
/update-branch |
mattermost-build
added
E2E/Run
amyblais
added
CherryPick/Approved
amyblais
added
the
CherryPick/Approved
devinbinnie
added
E2E/Run
|
/update-branch |
mattermost-build
added
E2E/Run
enahum
left a comment
enahum
left a comment
There was a problem hiding this comment.
One confirmation needed the reat seems fine
There was a problem hiding this comment.
I don't think it's likely to hit this, and if it does, it's always the same code path so I don't think there's a problem here.
| <key>NSFaceIDUsageDescription</key> | ||
| <string>Enabling access to your Face ID means we can restrict unauthorized users from accessing $(PRODUCT_NAME) on your device.</string> | ||
| <key>NSLocationWhenInUseUsageDescription</key> | ||
| <string>Your location can be used to report the Wi-Fi network name to your administrator when required by your organization's security policy.</string> |
There was a problem hiding this comment.
Is this accurate? Location is needed for the Wifi ssid?
There was a problem hiding this comment.
Yeah I think you need it to get any wi-fi information.
| import UIKit | ||
| import os.log | ||
| import Sentry | ||
| import react_native_network_client |
There was a problem hiding this comment.
Does this add the entire RN framework as a dep of the share extension? Based on the Podfile it shouldn't but please confirm
There was a problem hiding this comment.
Confirmed it doesn't.
| import Intents | ||
| import os.log | ||
| import TurboLogIOSNative | ||
| import react_native_network_client |
There was a problem hiding this comment.
Same question as share extension
enahum
left a comment
enahum
left a comment
There was a problem hiding this comment.
Approving with the promise that deltas will be added in the next release
devinbinnie
added
4: Reviews Complete
devinbinnie
removed
the
E2E/Run
|
Cherry pick is scheduled. |
8 participants
Summary
This PR introduces a
SessionAttributesManagerthat reports client Session Attributes to enabled servers for use in ABAC policies.The collection and header injection themselves live in
@mattermost/react-native-network-client1.11.0, which is bumped here: a request adapter attaches the base64-encodedX-MM-Session-Attributesheader to authenticated traffic, including uploads and downloads, and honours each attribute's TTL so values are only re-sent once stale. It's opt-in per client viaenableSessionAttributes, and the native paths that bypass the API client (share extension, notification service, notification reply) resolve the same header through a small hook in Gekidou.Reporting the Wi-Fi SSID needs new platform capabilities:
ACCESS_WIFI_STATEandACCESS_FINE_LOCATIONon Android, and thewifi-infoentitlement plus a location usage string on iOS.Ticket Link
MM-68986
MM-69203
Release Note