phptek / silverstripe-sentry

Flexible Sentry compatible bug aggregation client for Silverstripe applications.
BSD 3-Clause "New" or "Revised" License
12 stars 22 forks source link

User data not sent #64

Closed purplespider closed 3 years ago

purplespider commented 3 years ago

Data for the current logged in user doesn't seem to be being sent to Sentry: image

Is this a bug or do I need to do something to enable this functionality?

phptek commented 3 years ago

Have you been able to send data to Sentry successfully before now? If so, has anything changed since? What version of Silverstripe and the module are you using?

purplespider commented 3 years ago

First time using the module. Using Silverstripe 4.7.0 and "phptek/sentry": "^3.0"

purplespider commented 3 years ago

I installed the module using composer require phptek/sentry, added SENTRY_DSN="https://..." to my .env and tested an error by just removing a use statement. The error appears in Sentry, but doesn't include user data despite being logged in.

I can see some references to getCurrentUser() in the module code, so presume it is set up to do this.

dcentrica commented 3 years ago

OK, so error reporting is working, but user-data isn't being displayed. Sorry, I missed that.

As you may be able to see from the logic itself here: https://github.com/phptek/silverstripe-sentry/blob/master/src/Log/SentryLogger.php#L276 - the data is simply being drawn from Security::getCurrentUser() and there's not much to it.

Can I suggest a less "severe" test that data is being sent, by re-instating your use statement and simply invoking an Exception?I'd like to hope that removing a use is not an issue you're likely to encounter in production, given tat you should have tested things in dev first. However, exceptions are and will be :-)

dcentrica commented 3 years ago

Sorry, I logged in as a different user but "dcentrica" and "phptek" are both me :-)

purplespider commented 3 years ago

Thanks! I was ready to apologise, as testing using an Exception seemed an obvious solution, but unfortunately, it wasn't that.

I added an exception with the current logged in user's email in the title: throw new \LogicException("Sentry Test: ".Security::getCurrentUser()->Email);

The error appeared in Sentry as "Sentry Test: user@example.com" but still without populating the user data.

I have also tested the Raygun module on exactly the same site with exactly the same errors, and Raygun does correctly obtain the user data.

Do I need to somehow enable the capture of the user data for this module? I can see references to user in config but can't see an obvious way to control this globally.

phptek commented 3 years ago

Are you logged in as Silverstripe's "admin" user or an actual account with a valid email address? If the latter and you're not seeing it in user data, then I'd say this was a bona-fide bug.

purplespider commented 3 years ago

I tried both, as I too wondered if it could be due to being 'admin', but can confirm that it still doesn't work when logged in as a normal 'user' with an email address.

phptek commented 3 years ago

I can confirm that this also doesn't work on my setup. However, it did at one time used to. Given that the implementation in the Sentry module is very simple, I went looking in the framework issues list and found this. However, the patch has been implemented and I can see it in framework's source.

I'm still thinking there's a problem with the way in which SentryHandleris pushed onto the stack of MonologHandlers, perhaps such that the $member data isn't yet populated in some upstream middleware, but I am unsure. I suspect, the module's config needs to be modified to be After some other, but I am unsure what (pinging @tractorcow for guidance when he has time - I've been out of the Silverstripe loop for almost a year, things may have changed) but I imagine - something like this

---
Name: sentry-config
After:
  - 'requestprocessors'
  - 'coresecurity'
  - 'logging'
---

...our config here.
purplespider commented 3 years ago

In case it helps, this Silverstripe Raygun module does successfully send the user data with exceptions. Maybe it could provide some clues.

(After enabling it in config:)

Raygun4php\RaygunClient:
  disable_user_tracking: false
phptek commented 3 years ago

@purplespider I have patched "master". Can you verify that this fix works for you. if so, I'll tag a release.

purplespider commented 3 years ago

Ace! Yep, I've run a few tests with the master and can confirm it now appears to be working as expected, both when logged in and not logged in.

phptek commented 3 years ago

@purplespider Fix is in 3.0.7.

tractorcow commented 3 years ago

Getting current user will either work or fail, depending on whether the error occurs after, or before authentication (or even during).

Doing the getCurrentUser() as late as possible, as implemented, seems to be the correct fix. :)

phptek commented 3 years ago

Thanks for the input :-)