[APPSEC-9545] Devise integration and automatic user events #2877

GustavoCaso · 2023-05-25T14:46:19Z

What does this PR do?

The PR adds a new AppSec integration devise. The integration is responsible for instrumenting User events: login, and singup.

The PR also adds a couple of things:

A new AppSec configuration automated_track_user_events
Add a AppSec user event track_signup

Motivation

Additional Notes

How to test the change?

CI

codecov-commenter · 2023-05-30T09:28:22Z

Codecov Report

Merging #2877 (131b27e) into master (d3bdba5) will decrease coverage by 0.11%.
The diff coverage is 81.81%.

@@            Coverage Diff             @@
##           master    #2877      +/-   ##
==========================================
- Coverage   98.09%   97.98%   -0.11%     
==========================================
  Files        1268     1274       +6     
  Lines       70031    70313     +282     
  Branches     3195     3237      +42     
==========================================
+ Hits        68697    68897     +200     
- Misses       1334     1416      +82

Impacted Files	Coverage Δ
...ec/contrib/devise/patcher/authenticatable_patch.rb	`24.32% <24.32%> (ø)`
...ib/devise/patcher/registration_controller_patch.rb	`31.03% <31.03%> (ø)`
lib/datadog/appsec/contrib/devise/tracking.rb	`50.00% <50.00%> (ø)`
lib/datadog/appsec/contrib/devise/resource.rb	`68.75% <68.75%> (ø)`
lib/datadog/appsec/contrib/devise/patcher.rb	`72.00% <72.00%> (ø)`
lib/datadog/appsec/component.rb	`97.72% <75.00%> (-2.28%)`	⬇️
lib/datadog/appsec/contrib/devise/integration.rb	`85.00% <85.00%> (ø)`
lib/datadog/kit/appsec/events.rb	`96.42% <85.71%> (-3.58%)`	⬇️
spec/datadog/appsec/configuration/settings_spec.rb	`99.51% <99.50%> (-0.49%)`	⬇️
lib/datadog/appsec.rb	`90.62% <100.00%> (+0.30%)`	⬆️
... and 5 more

... and 7 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

lloeki

I'm OK to have this merged as is because it works and would need costly rebasing if it drags along, but several comments need addressing in short order, especially public API things.

Also watch out for conflicts with #2858

lloeki · 2023-06-01T10:26:19Z

lib/datadog/appsec/configuration/settings.rb

@@ -113,6 +114,7 @@ def duration(base = :ns, type = :integer)
          'DD_APPSEC_TRACE_RATE_LIMIT' => [:trace_rate_limit, Settings.integer],
          'DD_APPSEC_OBFUSCATION_PARAMETER_KEY_REGEXP' => [:obfuscator_key_regex, Settings.string],
          'DD_APPSEC_OBFUSCATION_PARAMETER_VALUE_REGEXP' => [:obfuscator_value_regex, Settings.string],
+          'DD_APPSEC_AUTOMATED_USER_EVENTS_TRACKING' => [:automated_track_user_events, Settings.string],


Not too fond of that naming. Is DD_APPSEC_AUTOMATED_USER_EVENTS_TRACKING a specified env var?

Yes is part of the RFC

OK for the env var, although a bit sad.

Indeed I feel like it should probably align with auto instrumentation, e.g:

DD_APPSEC_USER_TRACKING_AUTO_INSTRUMENT

And then overall, I'm thinking we might want a better structure for the code configuration API.

Notably setting something to disabled rubs me the wrong way because of the double negative:

automated_track_user_events_mode == Patcher::DISABLED_MODE

I feel it should look something like:

c.appsec.user_tracking.enabled = false c.appsec.user_tracking.enabled = true c.appsec.user_tracking.enabled = :safe c.appsec.user_tracking.enabled = :extended

or even this since auto instrumenting being enabled and operation mode are in fact orthogonal:

c.appsec.user_tracking.enabled = false # default? c.appsec.user_tracking.enabled = true # or c.appsec.user_tracking.auto_instrument = false # default? c.appsec.user_tracking.auto_instrument = true # and the reporting mode being separate c.appsec.user_tracking.mode = :safe c.appsec.user_tracking.mode = :extended

Also, also I don't understand how one is "safe" and the other "extended", somewhat implying "extended" is not "safe" (when IIUC it's actually safer WRT user naming in some ways since "extended" is supposed to hash user ids? It is very opaque as to what this does. In fact it seems to cover two very different things: the amount of data gathered automatically (which kind of ties in with autoinstrumentation) and the fact that some information may be hashed (which is orthogonal with autoinstrumentation).

And regarding he amount of data gathered automatically I have the feeling that the data gathered should be controllable by the user, e.g here the user may want to configure obtaining the username but not the email.

resource_email = devise_resource.email resource_username = devise_resource.username event_information[:email] = resource_email if resource_email event_information[:username] = resource_username if resource_username

That could give us some configuration that would look like:

c.appsec.user_tracking.collect = :safe c.appsec.user_tracking.collect = :extended c.appsec.user_tracking.collect = [ :id ] # equivalent to :save c.appsec.user_tracking.collect = [ :id, :email, username ] # equivalent to :extended c.appsec.user_tracking.collect = { email: :email, username: :username } # or something like that

These settings would apply to all instrumented frameworks. They should be overridable per framework:

c.appsec.instrument :devise, **options

... or something like that. Probably out of scope for this PR.

lib/datadog/appsec/configuration/settings.rb

lloeki · 2023-06-01T10:28:30Z

lib/datadog/appsec/contrib/devise/ext.rb

+          DISABLED_MODE = 'disabled'
+          EXTENDED_MODE = 'extended'


safe does not appear here, so this doesn't feel exhaustive. It feels like it should be an exhaustively listing enum.

lib/datadog/appsec/contrib/devise/patcher.rb

lib/datadog/appsec/extensions.rb

lib/datadog/kit/appsec/events.rb

The new setting can be configured on the normal Datadog.configure block and with the Env variable DD_APPSEC_AUTOMATED_USER_EVENTS_TRACKING. The valid values are: [:safe, :extended, :disabled]

Allow passing a custom block to all Datadog::Kit::AppSec::Events methods to modify the tags that get emitted

…ppSec:Events

…ule. Move it to Datadog::AppSec::Contrib::Devise::Patcher internal module

lloeki · 2023-06-14T08:54:40Z

lib/datadog/appsec/contrib/devise/ext.rb

+      module Devise
+        # Devise integration constants
+        module Ext
+          APP = 'devise'


Is this constant used anywhere?

None of the Ext::APP constants are used at all. We can decide to remove them all, but since is the standard I prefer to remove it on a separate PR

Yeah we should remove them (and any other unused constant), they're confusing as hell.

marcotc · 2023-06-15T22:26:08Z

General question: should devise be also a traced gem, from your experience working with it?

GustavoCaso · 2023-06-16T11:13:09Z

@marcotc

General question: should devise be also a traced gem, from your experience working with it?

Yes. I want to add the integration myself, but for the sake of reducing the amount of code on this PR I wanted to do as a separate PR

github-actions bot added appsec Application Security monitoring product integrations Involves tracing integrations labels May 25, 2023

GustavoCaso force-pushed the apsec-poc-automated-user-login-events branch 9 times, most recently from 8e43101 to 764382b Compare May 30, 2023 09:09

GustavoCaso force-pushed the apsec-poc-automated-user-login-events branch from 7b2c972 to 9f28d75 Compare May 30, 2023 11:40

GustavoCaso marked this pull request as ready for review May 30, 2023 12:42

GustavoCaso requested a review from a team May 30, 2023 12:42

GustavoCaso changed the title ~~Apsec poc automated user login events~~ [AppSec] Devise integration and automatic user events May 30, 2023

GustavoCaso requested a review from lloeki May 30, 2023 12:43

lloeki approved these changes Jun 1, 2023

View reviewed changes

GustavoCaso added 3 commits June 8, 2023 14:06

Add new AppSec configuration setting :automated_track_user_events

ad25612

The new setting can be configured on the normal Datadog.configure block and with the Env variable DD_APPSEC_AUTOMATED_USER_EVENTS_TRACKING. The valid values are: [:safe, :extended, :disabled]

Add track_signup method to Datadog::Kit::AppSec::Events

b1bfaa6

Allow passing a custom block to all Datadog::Kit::AppSec::Events methods to modify the tags that get emitted

Add Devise integration

1953be2

GustavoCaso force-pushed the apsec-poc-automated-user-login-events branch from 9f28d75 to 76d3abe Compare June 8, 2023 12:57

GustavoCaso added 4 commits June 9, 2023 12:56

Create internal tracking module. Avoid altering public API for Kit::A…

78ed0d9

…ppSec:Events

Fix typo

04c332c

Extract modules to separate files to be able to unit test them

11e72fb

update rbs files

8f95f78

GustavoCaso force-pushed the apsec-poc-automated-user-login-events branch from 76d3abe to 8f95f78 Compare June 9, 2023 16:18

GustavoCaso changed the title ~~[AppSec] Devise integration and automatic user events~~ [APPSEC-9545] Devise integration and automatic user events Jun 9, 2023

GustavoCaso added 2 commits June 13, 2023 08:51

add specs for Datadog::AppSec::Contrib::Devise::Resource

2b9ac01

remove constant from public Datadog::AppSec::Contrib::Devise::Ext mod…

50b5c02

…ule. Move it to Datadog::AppSec::Contrib::Devise::Patcher internal module

GustavoCaso force-pushed the apsec-poc-automated-user-login-events branch 3 times, most recently from c8c619b to fd9e0f4 Compare June 13, 2023 07:12

instrument devise automatically when appsec is enabled

131b27e

GustavoCaso force-pushed the apsec-poc-automated-user-login-events branch from fd9e0f4 to 131b27e Compare June 13, 2023 07:18

lloeki reviewed Jun 14, 2023

View reviewed changes

GustavoCaso merged commit 7e5e727 into master Jun 16, 2023
201 of 202 checks passed

GustavoCaso deleted the apsec-poc-automated-user-login-events branch June 16, 2023 11:22

github-actions bot added this to the 1.13.0 milestone Jun 16, 2023

GustavoCaso mentioned this pull request Jun 19, 2023

Removed unused constants from Appsec::Contrib::*::Ext modules #2911

Merged

TonyCTHsu mentioned this pull request Jul 30, 2023

Bump to version 1.13.0 #3009

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[APPSEC-9545] Devise integration and automatic user events #2877

[APPSEC-9545] Devise integration and automatic user events #2877

GustavoCaso commented May 25, 2023 •

edited

codecov-commenter commented May 30, 2023 •

edited

lloeki left a comment •

edited

lloeki Jun 1, 2023

GustavoCaso Jun 1, 2023

lloeki Jun 14, 2023 •

edited

lloeki Jun 1, 2023

lloeki Jun 14, 2023

GustavoCaso Jun 14, 2023

lloeki Jun 15, 2023

marcotc commented Jun 15, 2023

GustavoCaso commented Jun 16, 2023

[APPSEC-9545] Devise integration and automatic user events #2877

[APPSEC-9545] Devise integration and automatic user events #2877

Conversation

GustavoCaso commented May 25, 2023 • edited

codecov-commenter commented May 30, 2023 • edited

Codecov Report

lloeki left a comment • edited

Choose a reason for hiding this comment

lloeki Jun 1, 2023

Choose a reason for hiding this comment

GustavoCaso Jun 1, 2023

Choose a reason for hiding this comment

lloeki Jun 14, 2023 • edited

Choose a reason for hiding this comment

lloeki Jun 1, 2023

Choose a reason for hiding this comment

lloeki Jun 14, 2023

Choose a reason for hiding this comment

GustavoCaso Jun 14, 2023

Choose a reason for hiding this comment

lloeki Jun 15, 2023

Choose a reason for hiding this comment

marcotc commented Jun 15, 2023

GustavoCaso commented Jun 16, 2023

GustavoCaso commented May 25, 2023 •

edited

codecov-commenter commented May 30, 2023 •

edited

lloeki left a comment •

edited

lloeki Jun 14, 2023 •

edited