Initial attempt to block request base on user ID

[GustavoCaso]

No description provided.

[lloeki]

Looks like it's going in a good direction, I feel like I'm missing something though.

[lloeki]

This looks okay

[lloeki]

Like for e.g Rack request and response gateway points, we should pass a user object here as a hash, or maybe a struct. It can contain just the id for now.

[GustavoCaso]

I see your point of passing an object or a struct, but at the same time, it feels like the early decision to pass an object when we only care about a single attribute of the thing might not be necessary. Delaying that internal decision an internal thing is a good idea.

Also, the Gateway does not validate what the data is when pushed, so in a way, it could be anything, an object, a string...

[lloeki]

Having it be an object makes it easier to track the expectations and usage from the sender to the receiver. It also encapsulates the extensibility of it

In addition it forces us to "find a place" to define the matching class, which was not the case previously since they were predefined Rack::Request and Rack::Response

... which I was wondering about whether they should be boxed into some gateway-specific object. Something like (very rough, pseudocode, names not exact, jus to give an idea of the hierarchy):

Gateway::Object
Gateway::User < Gateway::Object
Gateway::Request < Gateway::Object
Gateway::Response < Gateway::Object

and then we can type (again, rough pseudocode to get the idea):

• Gateway#push: (address, Gateway::Object) -> ...
• Gateway#watch: () { |Gateway::Object| }

Let's try something like that in a subsequent PR.

[lloeki]

There's something I don't quite like about passing this [nil, appsec_response] here, but I can't quite put my finger on it yet. Something to do about breaking encapsulation. I need to step back.

[GustavoCaso]

I think we should create a return object rather than an array, which is the rack expected interface

[lloeki]

Yeah, also by being an object, it would have a type name, thus make it easier to track the expectations and usage from the sender to the receiver.

I feel this is out of scope for this PR though.

[lloeki]

Hmm, interesting, I was not quite expecting that, which implements a sort of forwarding. I would have expected that catch to be in RequestMiddleware around @app.call:

dd-trace-rb/lib/datadog/appsec/contrib/rack/request_middleware.rb

Lines 38 to 40 in 63b55b5

	request_return, request_response = Instrumentation.gateway.push('rack.request', request) do
	@app.call(env)
	end

I need to take a step back and think about it a little bit more.

Any reason you put it here instead?

[GustavoCaso]

Because since the event happen with the request flow, when we throw if we call catch in the RequestMiddleware the code from the watch_request after calling stack.call(request) would continue and the result would be extra wrap the result in an extra array.

We can probably look for other places to have the catch located.

Basically I wanted this code to not be executed:

dd-trace-rb/lib/datadog/appsec/contrib/rack/gateway/watcher.rb

Lines 69 to 74 in b1f492f

	if event
	res ||= []
	res << [:monitor, event]
	end
	[ret, res]

[lloeki]

Thinking about it more, there are a bunch of things:

• throw Datadog::AppSec::Ext::REQUEST_INTERRUPT: set_user is theoretically and conceptually supposed to be called anytime and possibly not in a HTTP request. This is an argument to not have a name such as REQUEST_INTERRUPT in set_user (where I wanted it to be initially)

• the above also applies to the fact that the gateway controls the instrumentation and without appsec then there will be no throw by construction since there will be no watcher, just like that, and set_user is completely appsec-free

• yet I am wondering about:

  I wanted this code to not be executed

  Is that what we want? kind of breaks the isolation of these watcher blocks

In any case I suggested some changes but now I feel this is actually going in the right direction, even though the latter point is kind of left up in the air for when we have more than one watch block.

[GustavoCaso]

Closing in favour of #2642