Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Introduce AppSec::Instrumentation::Gateway::Argument #2648

Merged
merged 7 commits into from
Feb 28, 2023

Conversation

GustavoCaso
Copy link
Member

@GustavoCaso GustavoCaso commented Feb 24, 2023

This PR introduces AppSec::Instrumentation::Gateway::Argument class.

This class is the expected object to be provided when passing information to Gateway#push

Each contrib integration can define their own Argument objects. The idea is to encapsulate all the logic regarding argument handling inside those classes. This helps with encapsulation and ease of testing each argument individually.

For now, the parent class AppSec::Instrumentation::Gateway::Argument has no method defined, as there isn't any shared logic among the types of arguments. This could change in the future.

The different classes added in this PR are:

Datadog::AppSec::Instrumentation::Gateway::User
Datadog::AppSec::Contrib::Rack::Gateway::Request
Datadog::AppSec::Contrib::Rack::Gateway::Response
Datadog::AppSec::Contrib::Sinatra::Gateway::Request
Datadog::AppSec::Contrib::Sinatra::Gateway::RouteParams
Datadog::AppSec::Contrib::Rails::Gateway::Request

AppSec::Instrumentation::Gateway::User is a particular case since it is framework agnostic and currently is only used in kit/identity.rb

For that reason, the definition of the class is inside lib/datadog/appsec/instrumentation/gateway/argument.rb and not in any contrib folder.

@github-actions github-actions bot added appsec Application Security monitoring product integrations Involves tracing integrations labels Feb 24, 2023
@GustavoCaso GustavoCaso force-pushed the appsec-add-gateway-result-object branch 4 times, most recently from 9b9987d to da2e304 Compare February 27, 2023 10:17
- Replace contrib Rack::Request and Rack::Response with Gateway::Request and Gateway::Response
@GustavoCaso GustavoCaso force-pushed the appsec-add-gateway-result-object branch from da2e304 to c8a200f Compare February 27, 2023 11:56
@GustavoCaso GustavoCaso changed the title Initial attempt at creating Gateway::Object Introduce AppSec::Instrumentation::Gateway::Argument Feb 27, 2023
@GustavoCaso GustavoCaso force-pushed the appsec-add-gateway-result-object branch from 9a64baa to f55473c Compare February 27, 2023 14:27
@GustavoCaso GustavoCaso marked this pull request as ready for review February 27, 2023 14:27
@GustavoCaso GustavoCaso requested review from a team and lloeki February 27, 2023 14:27
Copy link
Contributor

@lloeki lloeki left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, except usage of AppSec::Processor.active_context (which uses a thread local) should be made minimal when it can be accessed reliably otherwise.

lib/datadog/appsec/contrib/sinatra/patcher.rb Outdated Show resolved Hide resolved
module Rails
module Gateway
# Gateway Request argument. Normalized extration of data from ActionDispatch::Request
class Request < Instrumentation::Gateway::Argument
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it make sense to inherit from Contrib::Rack::Gateway::Request?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At first, I thought about, but since I saw we use the Rack::RequestBody directly in the Sinatra contrib folder I thought it would be ok.

I'm happy to change it.

lib/datadog/appsec/contrib/rack/gateway/watcher.rb Outdated Show resolved Hide resolved
@GustavoCaso GustavoCaso force-pushed the appsec-add-gateway-result-object branch 2 times, most recently from 20abc18 to 9043bcd Compare February 28, 2023 12:20
@codecov-commenter
Copy link

Codecov Report

Merging #2648 (9043bcd) into master (d76de94) will decrease coverage by 0.01%.
The diff coverage is 98.25%.

@@            Coverage Diff             @@
##           master    #2648      +/-   ##
==========================================
- Coverage   98.08%   98.08%   -0.01%     
==========================================
  Files        1160     1166       +6     
  Lines       63676    63832     +156     
  Branches     2849     2849              
==========================================
+ Hits        62457    62608     +151     
- Misses       1219     1224       +5     
Impacted Files Coverage Δ
spec/datadog/kit/identity_spec.rb 100.00% <ø> (ø)
...tadog/appsec/contrib/rails/reactive/action_spec.rb 99.02% <80.00%> (+0.01%) ⬆️
lib/datadog/appsec/contrib/rack/gateway/request.rb 91.83% <91.83%> (ø)
...ib/datadog/appsec/contrib/rack/gateway/response.rb 100.00% <100.00%> (ø)
lib/datadog/appsec/contrib/rack/gateway/watcher.rb 95.60% <100.00%> (ø)
...ib/datadog/appsec/contrib/rack/reactive/request.rb 100.00% <100.00%> (ø)
...tadog/appsec/contrib/rack/reactive/request_body.rb 100.00% <100.00%> (ø)
...b/datadog/appsec/contrib/rack/reactive/response.rb 100.00% <100.00%> (ø)
...dog/appsec/contrib/rack/request_body_middleware.rb 100.00% <100.00%> (ø)
.../datadog/appsec/contrib/rack/request_middleware.rb 96.10% <100.00%> (+0.15%) ⬆️
... and 23 more

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

@GustavoCaso GustavoCaso merged commit fe58f11 into master Feb 28, 2023
@GustavoCaso GustavoCaso deleted the appsec-add-gateway-result-object branch February 28, 2023 12:58
@github-actions github-actions bot added this to the 1.10.0 milestone Feb 28, 2023
@TonyCTHsu TonyCTHsu mentioned this pull request Mar 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
appsec Application Security monitoring product integrations Involves tracing integrations
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants