Merge pull request #1456 from bugsnag/bengourley/browser-device-id-de…

…fault-user-id

[PLAT-5052] feat(plugin-browser-device): Use device id as default user id

CHANGELOG.md

@@ -9,6 +9,7 @@
 ### Added
 
 - (expo): User ID now defaults to `device.id` if no user is set [#1454](https://github.com/bugsnag/bugsnag-js/pull/1454)
+- (browser): User ID now defaults to `device.id` if no user is set (when `collectUserIp=false`) [#1456](https://github.com/bugsnag/bugsnag-js/pull/1456)
 
 ### Changed
 

packages/plugin-browser-device/device.js

@@ -49,6 +49,8 @@ module.exports = (nav = navigator, screen = window.screen) => ({
 
     client.addOnSession(session => {
       session.device = assign({}, session.device, device)
+      // only set device id if collectUserIp is false
+      if (!client._config.collectUserIp) setDefaultUserId(session)
     })
 
     // add time just as the event is sent
@@ -58,6 +60,7 @@ module.exports = (nav = navigator, screen = window.screen) => ({
         device,
         { time: new Date() }
       )
+      if (!client._config.collectUserIp) setDefaultUserId(event)
     }, true)
   },
   configSchema: {
@@ -68,3 +71,11 @@ module.exports = (nav = navigator, screen = window.screen) => ({
     }
   }
 })
+
+const setDefaultUserId = (eventOrSession) => {
+  // device id is also used to populate the user id field, if it's not already set
+  const user = eventOrSession.getUser()
+  if (!user || !user.id) {
+    eventOrSession.setUser(eventOrSession.device.id)
+  }
+}

packages/plugin-browser-device/test/device.test.ts

@@ -6,6 +6,7 @@ import Client, {
 } from '@bugsnag/core/client'
 import { Device, Session } from '@bugsnag/core'
 import EventWithInternals from '@bugsnag/core/event'
+import { schema } from '@bugsnag/core/config'
 
 declare class SessionWithDevice extends Session { public device: Device }
 
@@ -457,5 +458,98 @@ describe('plugin: device', () => {
       expect(sessions).toHaveLength(1)
       expect(sessions[0].device.id).toBe(events[0].device.id)
     })
+
+    it('should not set device.id as user.id when collectUserIp=true', () => {
+      const client = new Client(
+        { apiKey: 'API_KEY_YEAH' },
+        {
+          ...schema,
+          collectUserIp: {
+            defaultValue: () => true,
+            validate: () => true,
+            message: ''
+          }
+        },
+        [plugin(navigator)]
+      )
+      const events: EventWithInternals[] = []
+      const sessions: SessionWithDevice[] = []
+
+      expect(client._cbs.e).toHaveLength(1)
+
+      mockDelivery(client, events, sessions)
+      client.notify(new Error('noooo'))
+
+      expect(events).toHaveLength(1)
+      expect(events[0]._user.id).toBe(undefined)
+
+      client.startSession()
+
+      expect(sessions).toHaveLength(1)
+      expect(sessions[0].getUser().id).toBe(undefined)
+    })
+
+    it('should set device.id as user.id when collectUserIp=false', () => {
+      const client = new Client(
+        { apiKey: 'API_KEY_YEAH', collectUserIp: false },
+        {
+          ...schema,
+          collectUserIp: {
+            defaultValue: () => true,
+            validate: () => true,
+            message: ''
+          }
+        },
+        [plugin(navigator)]
+      )
+      const events: EventWithInternals[] = []
+      const sessions: SessionWithDevice[] = []
+
+      expect(client._cbs.e).toHaveLength(1)
+
+      mockDelivery(client, events, sessions)
+      client.notify(new Error('noooo'))
+
+      expect(events).toHaveLength(1)
+      expect(events[0]._user.id).toBeTruthy()
+      expect(events[0]._user.id).toBe(events[0].device.id)
+
+      client.startSession()
+
+      expect(sessions).toHaveLength(1)
+      expect(sessions[0].getUser().id).toBeTruthy()
+      expect(sessions[0].getUser().id).toBe(events[0].device.id)
+    })
+
+    it('should not replace an existing user.id with device.id', () => {
+      const client = new Client(
+        { apiKey: 'API_KEY_YEAH', collectUserIp: false },
+        {
+          ...schema,
+          collectUserIp: {
+            defaultValue: () => true,
+            validate: () => true,
+            message: ''
+          }
+        },
+        [plugin(navigator)]
+      )
+      const events: EventWithInternals[] = []
+      const sessions: SessionWithDevice[] = []
+
+      expect(client._cbs.e).toHaveLength(1)
+
+      mockDelivery(client, events, sessions)
+      client.setUser('123', '[email protected]', 'User Email')
+      client.notify(new Error('noooo'))
+
+      expect(events).toHaveLength(1)
+      expect(events[0]._user.id).toBe('123')
+
+      client.startSession()
+
+      expect(sessions).toHaveLength(1)
+      expect(sessions[0].getUser().id).toBe('123')
+    })
   })
 })

test/browser/features/fixtures/user_info/script/f.html

@@ -0,0 +1,22 @@
+<!DOCTYPE html>
+<html>
+  <head>
+    <meta charset="utf-8" />
+    <script src="/node_modules/@bugsnag/browser/dist/bugsnag.min.js"></script>
+    <script type="text/javascript">
+      var NOTIFY = decodeURIComponent(window.location.search.match(/NOTIFY=([^&]+)/)[1])
+      var SESSIONS = decodeURIComponent(window.location.search.match(/SESSIONS=([^&]+)/)[1])
+      var API_KEY = decodeURIComponent(window.location.search.match(/API_KEY=([^&]+)/)[1])
+      Bugsnag.start({
+        apiKey: API_KEY,
+        endpoints: { notify: NOTIFY, sessions: SESSIONS },
+        collectUserIp: false
+      })
+    </script>
+  </head>
+  <body>
+    <script>
+      Bugsnag.notify(new Error('user info does work'))
+    </script>
+  </body>
+</html>

test/browser/features/fixtures/user_info/script/g.html

@@ -0,0 +1,23 @@
+<!DOCTYPE html>
+<html>
+  <head>
+    <meta charset="utf-8" />
+    <script src="/node_modules/@bugsnag/browser/dist/bugsnag.min.js"></script>
+    <script type="text/javascript">
+      var NOTIFY = decodeURIComponent(window.location.search.match(/NOTIFY=([^&]+)/)[1])
+      var SESSIONS = decodeURIComponent(window.location.search.match(/SESSIONS=([^&]+)/)[1])
+      var API_KEY = decodeURIComponent(window.location.search.match(/API_KEY=([^&]+)/)[1])
+      Bugsnag.start({
+        apiKey: API_KEY,
+        endpoints: { notify: NOTIFY, sessions: SESSIONS },
+        collectUserIp: false
+      })
+    </script>
+  </head>
+  <body>
+    <script>
+      Bugsnag.setUser('123', '[email protected]', 'User Email')
+      Bugsnag.notify(new Error('user info does work'))
+    </script>
+  </body>
+</html>

test/browser/features/ip_redaction.feature

@@ -1,12 +1,16 @@
 @ip_redaction
 Feature: Redacting IP addresses
 
+@skip_if_local_storage_is_unavailable
 Scenario: setting collectUserIp option to false
   When I navigate to the test URL "/ip_redaction/script/a.html"
   Then I wait to receive an error
   And the error is a valid browser payload for the error reporting API
+  And the event "device.id" is not null
+  And the error payload field "events.0.device.id" is stored as the value "device_id"
   And the event "request.clientIp" equals "[REDACTED]"
-  And the event "user.id" equals "[REDACTED]"
+  And the event "user.id" is not null
+  And the error payload field "events.0.user.id" equals the stored value "device_id"
 
 Scenario: setting collectUserIp option to false and providing user id
   When I navigate to the test URL "/ip_redaction/script/b.html"

test/browser/features/user_info.feature

@@ -24,3 +24,15 @@ Scenario: not setting user
   Then I wait to receive an error
   And the error is a valid browser payload for the error reporting API
   And the event "user.id" is null
+
+Scenario: defaulting to device.id
+  When I navigate to the test URL "/user_info/script/f.html"
+  Then I wait to receive an error
+  And the error is a valid browser payload for the error reporting API
+  And the event "user.id" is not null
+
+Scenario: default device.id does not override user.id
+  When I navigate to the test URL "/user_info/script/g.html"
+  Then I wait to receive an error
+  And the error is a valid browser payload for the error reporting API
+  And the event "user.id" equals "123"