Open Bug 1936020 Opened 2 months ago Updated 2 days ago

Make CA requests that are part of the same user action have the same user_action_id

Categories

(Firefox :: Data Loss Prevention, task)

task

Tracking

()

ASSIGNED

People

(Reporter: gstoll, Assigned: gstoll)

References

(Blocks 3 open bugs)

Details

Attachments

(20 files, 5 obsolete files)

48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review

We think this will help avoid cases where multiple parts of the same paste, etc. are blocked or warned and the DLP agent pops up their own dialog for each part. (sort of like bug 1927992)

This will require making changes in the clipboard paste but also for file upload. This will also require changes to cancel code since the CancelRequests() SDK message requires the user_action_id.

There's another optional field user_action_requests_count but I'm not sure what the consequences of not filling that out are. (this would be mildly annoying in the clipboard case since we fire off requests one at a time; not sure about file upload)

Assignee: nobody → gstoll
Status: NEW → ASSIGNED
Blocks: 1927992
See Also: → 1936530
No longer blocks: 1927992

This is helpful for later patches in this stack because when
multiple folders are dragged and dropped onto a page Content Analysis
needs to get all of their contents.

This is helpful so callers don't have to worry about specifying
user_action_id and user_action_requests_count or expanding a request
to analyze a folder into multiple requests for the files contained
inside.

This means the clipboard code is now just responsible for
gathering all the requests together and then it can just
call the new entry point.

Also move the logic for file dialog Content Analysis into
ContentAnalysis.cpp, and make it call the agent in parallel
instead of serialized.

As this will cancel all of the requests with the same userActionId,
this also calls the callbacks for those requests to indicate that.

Blocks: 1895248

Rename "request" variable that is a response to "response".

Depends on D236626

Remove variant version in favor of two classes.
Use MakeRefPtr as factory and make constructors private so that refcounting
is guaranteed.

Depends on D236627

Removed flavors from cache since they were not being considered in verdicts.
The cache is all or nothing so it added nothing to the seqno comparison.
This does not include the changes to the non-test callers, which are in another
patch.

Depends on D236628

The changes include:

  1. Making various IDL classes builtin.
  2. Making nsIContentAnalysisResponse subclass nsIContentAnalysisResult.
  3. Making requestToken and userAction fields internal to CA service.
  4. Added sourceWindowGlobal (the source of the data to scan) to request and
    use it for content-source-is-same-tab determinations.
  5. Made nsIContentAnalysisCallback expect a Result, not a Response.
  6. Added cancelRequestsByUserAction and renamed cancelContentAnalysisRequest to cancelRequestsByRequestToken (deprecated).
  7. Added ability for CancelAllRequests to forbid future requests (not needed but for peace of mind).
  8. Added MakeResponseForTest since response is not builtin.
  9. Made ContentAnalysis SupportsWeakPtr so that it can be weakly held by the MultipartRequestCallback,
    which can't just re-get the service since it doesn't want the mock one.
  10. Move SafeContentAnalysisResultCallback functionality to ContentAnalysisCallback.
  11. Make various routines use userActionId instead of requestToken.
  12. Add MultipartRequestCallback to count that we get the right number of responses.
  13. Remove unneeded thread sync behavior. The only part that was kind-of
    neeed was to abort behavior on a background thread if the request were
    canceled first -- this is a race that we don't need to win so we now just
    ignore it and ignore the result (TODO: I'm not positive I send cancel to
    the agent in this case).
  14. Send cancel to the agent (in all other cancel cases).
  15. Simplify the request map, which is now the user action map mUserActionMap.
  16. Makes the MultipartRequestCallback the nexus for filtering requests --
    this way filters are applied uniformly instead of sometimes-at-some-point-
    for-some-use-cases.

Depends on D236629

AnalyzeContentRequest* calls now require fewer fields. For example, the
URI is, by default, now obtained from the request's WindowGlobalParent's
BrowsingContext.

The commonDialog promise change is cosmetic but avoids async/await, which is nice.

browser-custom-element.js needed to re-get service for tests, since it was
sometimes getting an old mock version from a previous test.

ContentAnalysisCallback does what SafeContentAnalysisResultCallback used to do.

Depends on D236630

Add and use makeContentAnalysisResponse to make now native object.
Delegate analyzeContentRequest* to non-mock CA service. Folder
expansion, etc is done and tested with the real service now.
Also improves logging.

Depends on D236631

Allows us to delay at a granularity finer than one second.

Depends on D236632

Make browser_clipboard_paste_prompt_content_analysis.js wait for
selectionchange instead of paste, since paste happens immediately but
selectionchange happens after CA allows the contents to change.

Depends on D236634

Some tests change the expected order of the generated requests, which is not
important. Some also now expect null URI because getting the URI from the
windowGlobalParent is now the fallback/default. Note that DND tests are in a
separate patch because they get broader changes and additional tests.

Depends on D236635

Adds userActionId/userActionCount DND tests. Has AnalyzeContentRequest*
delegation (like head.js, which it does not use). Switches to using
MakeResponseForTest. Adds bypassForSameTab test. Makes dropPromise actually
wait for the drop.

Depends on D236636

The URL filter now takes uri as parameter because request may not have it,
since the windowGlobalParent is sometimes used to get it (which we could do
but this is a more thorough test).

Depends on D236637

nsIContentAnalysisCallback now receives an nsIContentAnalysisResult, not an
nsIContentAnalysisResponse. Also fixes a test typo.

Depends on D236638

Adds CheckUserActionRequestsCanCancelAndHaveSameUserActionId.

Depends on D236639

Attachment #9446345 - Attachment is obsolete: true
Attachment #9446346 - Attachment is obsolete: true
Attachment #9446347 - Attachment is obsolete: true
Attachment #9446348 - Attachment is obsolete: true
Attachment #9446349 - Attachment is obsolete: true

Allows clients to set the user action ID (consistently) but the more useful
effect is that, on return from AnalyzeContentRequests*, this will guarantee
a user action ID is set that can be canceled, or an nserror exception will
be thrown. It does this by adding an entry for the user action into the
user action map before all requests are finalized (notably before request
tokens have been generated), and if it has been removed from the map by
the time requests are final then it was canceled in the interim.

Depends on D237136

You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: