devilry / devilry-django

Devilry project main repository
http://devilry.org
BSD 3-Clause "New" or "Revised" License
51 stars 24 forks source link

Minimize social account data #1284

Closed torgeirl closed 6 months ago

torgeirl commented 9 months ago

The allauth_adapter and socialaccount_user_updaters should only receive five variables from the socialaccount.extra_data object:

And given the correct provider configuration it should only receive those five data points, but to ensure that the application doesn't save anything else if the provider by mistake pass on other data points than the requested ones it would be good to

a) either limit what get stored from the data exchange with the provider, or b) minimize the JSON object stored in social account's extra data to only the variables we need

As a bonus it would be good if we could flag the removal of certain data points. Ie. if value X is removed it makes an error Note: the docs should remind sysadmins that such values must undergo proper data scrubbing if used in combination with a log service.

Levijatan commented 8 months ago

Added removal of anything that is not needed in extra_data before saving of sociallogin. Logs as error when unwanted data is found.

054e382

torgeirl commented 8 months ago

Logs as error when unwanted data is found.

I believe we only want to log the keys removed since the values could contain sensitive information.

Levijatan commented 8 months ago

Removed value from logging

torgeirl commented 8 months ago

Hold on - this will never reach a monitoring service like Sentry so unless one actively look in the logs a sysadmin will never notice that there is an issue with the data the login provider is returning.

I guess there are many ways of doing this, but for it to be picked up by Sentry either an unhandled exception has to be raised or the handled exception needs to be captured explicitly as seen in #1286. I have only tested the later, but I believe the first might result in the user experiencing a HTTP 500 while the later only happens in the background.

Levijatan commented 8 months ago

Sentry should treat logger.error as an error event if it is configured correctly. https://docs.sentry.io/platforms/python/integrations/logging/

torgeirl commented 8 months ago

You're right; guess I read too much into this when implementing this for Trix:

The Sentry SDK will automatically capture and report any unhandled error that happens in your application runtime without any additional configuration or explicit handling. (...) The Sentry SDK contains several methods that you can use to explicitly report errors, events, and custom messages in except clauses, critical areas of your code, and so on.

torgeirl commented 6 months ago

It works as intended, but I forgot to mention the fifth value to expect (updated opening post now) since it is an unusable dummy value: profilephoto. extra_data_to_keep needs to also include it so it doesn't trigger false positives.

torgeirl commented 6 months ago

Fixed in 82ebfc9.