1950424 - If one file upload is BLOCKed by DLP agent, allow the other files through

Status: RESOLVED FIXED

(Firefox :: Data Loss Prevention, defect)

Description

[Greg Stoll :gstoll]

(this is by request from a vendor)

If the user selects multiple individual files to upload, FIrefox should allow any files that get the ALLOW response to go through.

If the user selects a folder to upload, Firefox should continue the current behavior where if any file is BLOCKed, they all are.

This should cover clipboard paste, file picker, and drag and drop.

Comment 1

[Greg Stoll :gstoll]

Proposal and discussion here: https://docs.google.com/document/d/1aARizOuj5dgGuHgT9jsnHpej2YbKymQZ9wnaL9rUEqU/edit?usp=sharing

Comment 2

[Greg Stoll :gstoll]

Attachment: WIP: Bug 1950424 - implement Content Analysis batch mode

Comment 3

[Greg Stoll :gstoll]

Attachment: Bug 1950424 part 1 - add Content Analysis "batch" mode to analyze requests independently r=#dlp-reviewers!

This adds CheckFilesInBatchMode() which will analyze file requests
independently, so if one of them gets BLOCKed the other requests will
still be analyzed.

Comment 4

[Greg Stoll :gstoll]

Attachment: Bug 1950424 part 2 - make nsFilePicker call the new Content Analysis "batch" entry point r=#dlp-reviewers!

Comment 5

[Greg Stoll :gstoll]

Attachment: Bug 1950424 part 3 - add gtest for batch mode r=#dlp-reviewers!

Comment 6

[Greg Stoll :gstoll]

Attachment: Bug 1950424 part 4 - handle race-y condition with multiple requests and user action IDs r=#dlp-reviewers!

The new gtest added in part 3 was failing intermittently by hitting an
assert that indicated the userActionId and requestToken didn't match
what we had set up in MultipartRequestCallback.

The reason is that in DoAnalyzeRequest(), calling Send() on the client
will return the a response that does not necessary match the request!
Since the agent does not return a userActionId in its response (sigh),
we used to remember the passed-in userActionId and set that on the response,
but that is wrong.

Now we look up the userActionId from the mUserActionMap and set it on
ContentAnalysisRequest. This means that if the request is cancelled we
will not be able to find the real userActionId, so adjust
mUserActionIdToCanceledResponseMap to keep track of the canceled responses
by request token instead.

Comment 7

[Greg Stoll :gstoll]

Attachment: Bug 1950424 part 5 - avoid excessive client recreation r=#dlp-reviewers!

Yet another intermittent gtest failure. This time the problem seems to
be if there are a lot of background threads trying to connect to a client
and failing, we can end up recreating the client a lot of times and
invalidating old mCaClientPromise's, which can mean we give up trying to
send a message.

Instead, only recreate a new client if we haven't tried to recreate it
since we failed; otherwise, just try again, and if that fails we're probably
doomed to failure anyway.

Comment 8

[Greg Stoll :gstoll]

Attachment: Bug 1950424 part 6 - fix issue with cancelling a request by userActionId very early r=#dlp-reviewers!

Some tests were hitting race conditions when trying to cancel a
request very early after calling AnalyzeContentRequests(). This change
moves setting the request token to earlier in the code.

Comment 9

[Greg Stoll :gstoll]

Attachment: Bug 1950424 part 7 - make Content Analysis gtests less race-y r=#dlp-reviewers!

Adding the batch mode test exposed some other problems in the gtests.

There are a few categories of changes here:

1. Wait for acknowledgments after sending a request and getting a response.
   This is helpful because sending an acknowledgment requires calling
   into the client, and this was often happening after the test was
   finished, which could cause the client to be recreated at weird times.
   Note that this isn't possible for all tests, but it is possible for most.
   This is also helpful in some cases because we weren't testing that
   setting autoAcknowledge (like all of our real use cases do right now)
   was actually causing acknowledgments to be sent.
2. When waiting for acknowledgments, be tolerant to the fact
   that other operations might be going on, and just look for the
   request token that the code is interested in instead of asserting
   that the number of acknowledgements is what we expect.
3. When killing the agent (for testing purposes) and starting the agent
   again, call SendSimpleRequestAndWaitForResponse() to force the client
   to be created before the end of the test and avoid issues like #1
   above.

There are a few other very small things I'll flag as review comments.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6a19f18f157c
part 1 - add Content Analysis "batch" mode to analyze requests independently r=dlp-reviewers,handyman
https://hg.mozilla.org/integration/autoland/rev/1ce2439b3b91
part 2 - make nsFilePicker call the new Content Analysis "batch" entry point r=dlp-reviewers,win-reviewers,rkraesig,handyman
https://hg.mozilla.org/integration/autoland/rev/7f09f3ddba51
part 3 - add gtest for batch mode r=dlp-reviewers,handyman
https://hg.mozilla.org/integration/autoland/rev/529ea124f966
part 4 - handle race-y condition with multiple requests and user action IDs r=dlp-reviewers,handyman
https://hg.mozilla.org/integration/autoland/rev/e5caecba9386
part 6 - fix issue with cancelling a request by userActionId very early r=dlp-reviewers,handyman
https://hg.mozilla.org/integration/autoland/rev/cc184b7716c2
part 7 - make Content Analysis gtests less race-y r=dlp-reviewers,handyman

Comment 11

[amarc]

https://hg.mozilla.org/mozilla-central/rev/6a19f18f157c
https://hg.mozilla.org/mozilla-central/rev/1ce2439b3b91
https://hg.mozilla.org/mozilla-central/rev/7f09f3ddba51
https://hg.mozilla.org/mozilla-central/rev/529ea124f966
https://hg.mozilla.org/mozilla-central/rev/e5caecba9386
https://hg.mozilla.org/mozilla-central/rev/cc184b7716c2