Implementing CDP command to allow the trigger of an extension action [chromium/src : main]
Devlin Cronin (Gerrit)
Devlin Cronin added 14 comments![]( "Open in Gerrit")
Thanks for the CL!
It's also been a few years since the doc was written -- can we make sure security has signed off on this new capability? (If that review has already happened, feel free to point me to it!)
Bug: 455536313is there a reason this needs to be restricted?
}can we also add a test that exercises dispatching action.onClicked (i.e., has neither a popup nor a side panel)?
const protocol::String& in_extensionId) override;style should be "extension_id"
#include "base/observer_list.h"Is this needed?
#include "chrome/common/extensions/api/tabs.h"This is a lot of new includes; are they all needed?
const std::string& extensionId) {extension_id
auto* web_content = host->GetWebContents();nit: web_contents
auto* ctx = host->GetBrowserContext();nit: `context` or `browser_context` ([avoid uncommon abbreviations](https://google.github.io/styleguide/cppguide.html#General_Naming_Rules))
constexpr bool kGrantTabPermissions = true;Do we want this to grant tab permissions? I didn't see a reference to that decision or discussion in the design doc.
extension_action_trigger_service->TriggerAction(extension.value()->id(),This looks like it effectively results in triggering the action twice(ish), once through the ExtensionActionRunner and then again here, indirectly, via the observer.
class ExtensionActionTriggerService : public KeyedService {This seems unnecessarily complicated.
@emil...@chromium.org, what's the best way to trigger an action via C++?
chrome.runtime.onMessage.addListener((request, sender, sendResponse) => {this structure would be easier if we just called notifyPass() (or sent a message via chrome.test.sendMessage('popup opened')) from the popup
chrome.test.notifyPass();sending this pass signal here means we don't test the side panel's appearing. Should we instead have this test use [setPanelBehavior](https://developer.chrome.com/docs/extensions/reference/api/sidePanel#method-setPanelBehavior) so that clicking on the button triggers the panel, and then send a message from the panel itself?
Related details
- Nicholas Roscino
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nicholas Roscino (Gerrit)
Nicholas Roscino added 15 comments![]( "Open in Gerrit")
Thanks for the CL!
It's also been a few years since the doc was written -- can we make sure security has signed off on this new capability? (If that review has already happened, feel free to point me to it!)
I think you might have opened the old version that was the one I used as a baseline to create the new one! You can find the new one at this go link: go/chrome-devtools:enabling-extension-action-via-cdp-design. LMK what you think!
Bug: 455536313is there a reason this needs to be restricted?
Not sure what are we talking about here. You mean that the issue could be public on crbug?
GURL url = embedded_test_server()->GetURL("/blank.html");Nicholas Roscinocan we use `about:blank`?
Nicholas RoscinoI tried but this was not working. The test was hanging and failing. I tried with some other pages like other tests were doing and it passed. I think there should be a way to make it work also with about:blank but didn't spend much time on this. Do you have any idea?
Done
can we also add a test that exercises dispatching action.onClicked (i.e., has neither a popup nor a side panel)?
Done
const protocol::String& in_extensionId) override;Nicholas Roscinostyle should be "extension_id"
Done
#include "base/observer_list.h"Nicholas RoscinoIs this needed?
Done
This is a lot of new includes; are they all needed?
Done
const std::string& extensionId) {Nicholas Roscinoextension_id
Done
auto* web_content = host->GetWebContents();Nicholas Roscinonit: web_contents
Done
nit: `context` or `browser_context` ([avoid uncommon abbreviations](https://google.github.io/styleguide/cppguide.html#General_Naming_Rules))
Done
constexpr bool kGrantTabPermissions = true;Do we want this to grant tab permissions? I didn't see a reference to that decision or discussion in the design doc.
I tried to address this in the doc. The idea is that this simulates the user clicking on the UI.
The triggering code was taken from the ui part that handles the user click interaction. Since we want to make sure to replicate the same behavior I also added the GrantTabPermission to the automation (UI code: https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/ui/extensions/extension_action_view_model.cc;drc=eb33936323cf453f064c374c3176ffc202c61239;l=407).
Since all this block runs behind a specific flag that enables extension debugging features (--enable-unsafe-extension-debugging) I don't think this exposes a user to some security risks, am I missing something here?
extension_action_trigger_service->TriggerAction(extension.value()->id(),This looks like it effectively results in triggering the action twice(ish), once through the ExtensionActionRunner and then again here, indirectly, via the observer.
With the `ExtensionActionRunner` you are referring to the call to `RunAction`?
If that is the case, for my understanding of the RunAction method, that just returns the type of action that has to be executed unless there are no side panel and no popup to show. In that case it triggers the ExtensionAction itself but returns an action of kNone.
The TriggerAction method is triggering the action on the observer (ExtensionActionViewModel::OnExtensionActionTriggered) that is checking for the action type, if it is kNone, it does nothing. Otherwise it runs either the code for the Popup or the side panel.
Let me know if I am missing something tho!
<html><head><title>blank page</title></head>Nicholas Roscinois it needed?
Nicholas RoscinoThis was the result of what I said in https://chromium-review.googlesource.com/c/chromium/src/+/7132423/comment/073077bf_9cdf3395/ but would be glad if I can avoid this and remove it.
Done
chrome.runtime.onMessage.addListener((request, sender, sendResponse) => {this structure would be easier if we just called notifyPass() (or sent a message via chrome.test.sendMessage('popup opened')) from the popup
Done
sending this pass signal here means we don't test the side panel's appearing. Should we instead have this test use [setPanelBehavior](https://developer.chrome.com/docs/extensions/reference/api/sidePanel#method-setPanelBehavior) so that clicking on the button triggers the panel, and then send a message from the panel itself?
- Alex Rudenko
- Devlin Cronin
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Emilia Paz (Gerrit)
Emilia Paz added 3 comments![]( "Open in Gerrit")
Thanks Nicholas! Gave an example of how to use ExtensionActionViewModel::ExecuteUserAction without introducing a factory, although unsure if possible with CDP
constexpr bool kGrantTabPermissions = true;Nicholas RoscinoDo we want this to grant tab permissions? I didn't see a reference to that decision or discussion in the design doc.
I tried to address this in the doc. The idea is that this simulates the user clicking on the UI.
The triggering code was taken from the ui part that handles the user click interaction. Since we want to make sure to replicate the same behavior I also added the GrantTabPermission to the automation (UI code: https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/ui/extensions/extension_action_view_model.cc;drc=eb33936323cf453f064c374c3176ffc202c61239;l=407).
Since all this block runs behind a specific flag that enables extension debugging features (--enable-unsafe-extension-debugging) I don't think this exposes a user to some security risks, am I missing something here?
Not familiar with CDP. Is `ExtensionsHandler::TriggerAction` intended to be triggered from an user action? Tab permissions can only be granted if the action is executed by the user
class ExtensionActionTriggerService : public KeyedService {This seems unnecessarily complicated.
@emil...@chromium.org, what's the best way to trigger an action via C++?
An action is triggered via `ExtensionActionRunner::RunAction`. Since running an action should be user initiated, it gets called from `ExtensionActionViewModel::ExecuteUserAction(InvocationSource source)` which does:
- if action cannot be run on web contents: shows context menu as fallback
- closes extensions menu if opened
- runs actions
- records invocation source
- triggers popup or side panel, if neccesary
My understanding is if CSP command is a user-initiated command and mimics "clicking on the action", then all these items in `ExtensionActionViewModel::ExecuteUserAction` should be covered too. Thus, ideally we use that method.
`ExtensionActionViewModel` can be accessed through the extensions container, owned by the browser window. For example, developers private api [triggers](https://source.chromium.org/chromium/chromium/src/+/448017a4bb0441509cafdf0123f5664f27ea16bc:chrome/browser/extensions/api/extension_action/extension_action_api_non_android.cc;l=83-108;bpv=0;bpt=0) the popup via `browser.window()->GetExtensionsContainer()->ShowToolbarActionPopupForAPICall()`. We could have something similar if there is access to the browser/browserwindowinterface (not sure if that's possible, from a quick look didn't see browser usage on extensions handler)
Related details
- Alex Rudenko
- Devlin Cronin
- Nicholas Roscino
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Devlin Cronin (Gerrit)
Devlin Cronin added 5 comments![]( "Open in Gerrit")
Thanks, Nicholas! I took a fairly light pass at this one, since I'm still very much hoping we can simplify the flow to trigger the action, inline with Emilia's comments.
Bug: 455536313Nicholas Roscinois there a reason this needs to be restricted?
Not sure what are we talking about here. You mean that the issue could be public on crbug?
Correct. Chromium is open source, and bugs should generally be open, as well, unless there's compelling reasons for them to be restricted (e.g., security vulnerabilities that we're fixing or it being tied to internal-only projects).
return protocol::Response::ServerError("ExtensionActionRunner not found.");This seems fairly meaningless to a (web / extension) developer. Maybe just "Can't run on this target" or similar?
constexpr bool kGrantTabPermissions = true;Nicholas RoscinoDo we want this to grant tab permissions? I didn't see a reference to that decision or discussion in the design doc.
Emilia PazI tried to address this in the doc. The idea is that this simulates the user clicking on the UI.
The triggering code was taken from the ui part that handles the user click interaction. Since we want to make sure to replicate the same behavior I also added the GrantTabPermission to the automation (UI code: https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/ui/extensions/extension_action_view_model.cc;drc=eb33936323cf453f064c374c3176ffc202c61239;l=407).
Since all this block runs behind a specific flag that enables extension debugging features (--enable-unsafe-extension-debugging) I don't think this exposes a user to some security risks, am I missing something here?
Not familiar with CDP. Is `ExtensionsHandler::TriggerAction` intended to be triggered from an user action? Tab permissions can only be granted if the action is executed by the user
Ack; thanks, Nicholas. I'm okay with this, as long as this remains behind those flags. It makes sense that we want it to closely emulate user actions in order to be able write automated tests reflecting those.
extension_action_trigger_service->TriggerAction(extension.value()->id(),This looks like it effectively results in triggering the action twice(ish), once through the ExtensionActionRunner and then again here, indirectly, via the observer.
With the `ExtensionActionRunner` you are referring to the call to `RunAction`?
If that is the case, for my understanding of the RunAction method, that just returns the type of action that has to be executed unless there are no side panel and no popup to show. In that case it triggers the ExtensionAction itself but returns an action of kNone.
The TriggerAction method is triggering the action on the observer (ExtensionActionViewModel::OnExtensionActionTriggered) that is checking for the action type, if it is kNone, it does nothing. Otherwise it runs either the code for the Popup or the side panel.
Let me know if I am missing something tho!
The ExtensionActionRunner doesn't show any UI (side panel or popup), but it *does* do everything else, including:
- Granting tab permissions
- Firing action.onClicked in the extension
The former is, I suppose, safe to fire twice (though kind of awkward), but the latter would be a functional bug that could result in broken functionality. For instance, let's say you just had an extension that counted the number of times the action was clicked by listening to the onClicked event. After each call to this, it would result in _two_ triggers.
Related details
- Alex Rudenko
- Nicholas Roscino
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |