launchWebAuthFlow to do authentication through tab [chromium/src : main]

Ryan Sultanem (Gerrit)

unread,

Nov 16, 2022, 8:24:09 AM11/16/22

to Alex Ilin, chromium-a...@chromium.org, extension...@chromium.org, mparch-revi...@chromium.org

Attention is currently required from: Alex Ilin.

Ryan Sultanem would like Alex Ilin to review this change.

launchWebAuthFlow to do authentication through tab

Created the feature flag `kAuthWithWebContentTab` to separate the new behavior.
For the `launchWebAuthFlow` flow, `WebAuthFlow` now tries to authenticate using a web content displayed in a new tab instead of an app window.

Change-Id: I46d1b6cb8e4a43eed7ea63afef56aee85cc5e0e4
---
M chrome/browser/extensions/api/identity/web_auth_flow.cc
M chrome/browser/extensions/api/identity/web_auth_flow.h
M chrome/browser/extensions/api/identity/web_auth_flow_browsertest.cc
3 files changed, 120 insertions(+), 7 deletions(-)

To view, visit change 4030540. To unsubscribe, or for help writing mail filters, visit settings.

Ryan Sultanem (Gerrit)

unread,

Nov 16, 2022, 8:26:21 AM11/16/22

to chromium-a...@chromium.org, extension...@chromium.org, mparch-revi...@chromium.org, Alex Ilin, Chromium LUCI CQ, Tricium, chromium...@chromium.org

Attention is currently required from: Alex Ilin.

View Change

1 comment:

Patchset:
- Patch Set #4:
  CL might not be fully ready at the test level.
  Adding you early to make sure that it is on the right track.

To view, visit change 4030540. To unsubscribe, or for help writing mail filters, visit settings.

Alex Ilin (Gerrit)

unread,

Nov 16, 2022, 2:14:34 PM11/16/22

to Ryan Sultanem, chromium-a...@chromium.org, extension...@chromium.org, mparch-revi...@chromium.org, Alex Ilin, Chromium LUCI CQ, Tricium, chromium...@chromium.org

Attention is currently required from: Ryan Sultanem.

View Change

16 comments:

Patchset:
- Patch Set #4:
  I think you're on a good track! I left some comments, mostly naming nits.
  We will also have to adapt chrome/browser/extensions/api/identity/identity_apitest.cc to your feature, but that will is a task for a separate CL.
File chrome/browser/extensions/api/identity/web_auth_flow.h:
- Patch Set #4, Line 33: kAuthWithWebContentTab
  naming nit: does `kWebAuthFlowInBrowserTab` sound good to you?
  WebContentTab sounds weird to me. All tabs contain WebContents so there are no NotWebContentTabs.
- Patch Set #4, Line 86: WebAuthParams
  I don't think this inner struct is necessary. It adds an additional level of nesting and I don't really see its benefits.
  Instead, you could group different class fields together based on the type of WebAuthFlow:
```
```
    raw_ptr<Delegate> delegate_;
    raw_ptr<Profile> profile_;
    GURL provider_url_;
    Mode mode_;
    Partition partition_;
    // Used to make sure that the user authentication is triggered once.
    bool using_auth_with_tab_;
```
```
    // Variables used only if displaying the auth flow in an app window. 
    raw_ptr<AppWindow> app_window_;
    std::string app_window_key_;
    bool embedded_window_created_;
```
```
    // Variables used only if displaying the auth flow in a browser tab.
    //
    // WebContents used to initialize the auth flow. It is not displayed
    // and not owned by a browser window. This `WebContents` is observed by
    // `this`.
    std::unique_ptr<content::WebContents> web_contents_;
    
```
```
- Patch Set #4, Line 97: content::WebContents* tab_web_contents_ptr_;
  You don't need to keep this pointer inside of the class. Since `WebAuthFlow` implements `WebContentsObserver`, it can access the web contents it's observing by calling `web_contents()`.
- Patch Set #4, Line 99: bool interactive_auth_started_
  Could you rely on `web_contents_` being nullptr instead? `web_contents_` becomes null after you move it into `NavigateParams`.
- Patch Set #4, Line 123: GetGuestPartition
  I'm wondering what we need to do with `GetGuestPartition()` and `GetWebViewPartitionConfig()` methods when the feature is enabled.
  `GetGuestPartition()` could return nullptr if the auth in tab is used. We'll need to update `GaiaRemoteConsentFlow` to handle nullptr and use a default storage partition (profile->GetDefaultStoragePartition()) when needed.
  I think we should leave `GetWebViewPartitionConfig()` as is, since its usage is really specific to the guest webview.
File chrome/browser/extensions/api/identity/web_auth_flow.cc:
- Patch Set #4, Line 113: using_auth_with_tab_(false)
  nit: could you please modernize this code and default initialize variables in a .h file instead?
  .h file:
```
const raw_ptr<Delegate> delegate_;
const raw_ptr<Profile> profile_;
const GURL provider_url_;
const Mode mode_;
const Partition partition_;
raw_ptr<AppWindow> app_window_ = nullptr;
std::string app_window_key_;
bool embedded_window_created_ = false;
bool using_auth_with_tab_ = false;
WebAuthParams auth_params_;
```
- Patch Set #4, Line 195:
```
 if (using_auth_with_tab_ && auth_params_.tab_web_contents_ptr_) {
    auth_params_.tab_web_contents_ptr_->ClosePage();
  }
```
  nit: We call `app_window_->web_contents()->Close();` in the `~WebAuthFlow()`. I'd also put your code into the destructor to have closing logic in one place.
  Also, shouldn't we use `Close()` instead of `ClosePage()`? We want to destroy the whole WebContents, not only the current page. However, I don't know if there are cases when WebContents stays alive after closing the page.
- Patch Set #4, Line 249: embedded_window_created_ || using_auth_with_tab_
  nit: I feel like this condition will be more readable if we create a helper method
  `IsObservingAuthWebContents()`
```
```
  bool IsObservingProviderWebContents() {
    return web_contents() && (embedded_window_created_ || using_auth_with_tab_);
  }
```
```
- Patch Set #4, Line 289: void WebAuthFlow::WebContentsDestroyed() {
  It feels like we should stop observing WebContents here. Otherwise, we might call `ClosePage()` on a destroyed object in `DetachDelegateAndDelete()`.
```
WebContentsObserver::Observe(nullptr);
```
File chrome/browser/extensions/api/identity/web_auth_flow_browsertest.cc:
- Patch Set #4:
  I think it'd be worth to also add some test that are specific to your implementation.
  For example, start the auth flow and check that no new tab is created first. Wait for the page load and verify that the tab is now added.
  Also check what happens when the tab is manually closed.
  Once https://crrev.com/c/3996076 lands, there will be more tests around the non-interactive mode that should be parameterized.
- Patch Set #4, Line 67: WebAuthFlowParamBrowserTest
  nit: `WebAuthFlowInBrowserTabParamBrowserTest`
- Patch Set #4, Line 75: usetab_feature_enabled
  nit: `use_tab_feature_enabled()`
- Patch Set #4, Line 118: AuthUsingTabFeature
  nit: please use the exact feature name to avoid confusion
- Patch Set #4, Line 121: WebAuthFlowGuestPartitionParamTest
  I agree that this test suite doesn't make sense with the authInTab feature enabled.
  Could you please explicitly disable your feature via `scoped_feature_list_` so that these tests don't conflict with your feature?
- Patch Set #4, Line 224: WebAuthFlowFencedFrameTest
  I think these test can also run with your feature enabled. You could derive them from `WebAuthFlowParamBrowserTest`

To view, visit change 4030540. To unsubscribe, or for help writing mail filters, visit settings.

Ryan Sultanem (Gerrit)

unread,

Nov 17, 2022, 2:07:45 PM11/17/22

to chromium-a...@chromium.org, extension...@chromium.org, mparch-revi...@chromium.org, Alex Ilin, Chromium LUCI CQ, Tricium, chromium...@chromium.org

Attention is currently required from: Alex Ilin.

View Change

15 comments:

File chrome/browser/extensions/api/identity/web_auth_flow.h:

- Patch Set #4, Line 33: kAuthWithWebContentTab

- naming nit: does `kWebAuthFlowInBrowserTab` sound good to you? […]
  `kWebAuthFlowInBrowserTab` sounds better indeed!
  I am not super comfortable with finding the proper names yet, hopefully it will get better with time!

- Patch Set #4, Line 86: WebAuthParams

- I don't think this inner struct is necessary. […]
  Done

- Patch Set #4, Line 97: content::WebContents* tab_web_contents_ptr_;

- You don't need to keep this pointer inside of the class. […]
  Done

- Patch Set #4, Line 99: bool interactive_auth_started_

- Could you rely on `web_contents_` being nullptr instead? `web_contents_` becomes null after you move […]
  I thought about that, but I felt like it was more explicit to have a variable for that. But I agree there is no actual need for additional information.

- Patch Set #4, Line 123: GetGuestPartition

I'm wondering what we need to do with `GetGuestPartition()` and `GetWebViewPartitionConfig()` method […]

Instead of returning nullptr in `GetGuestPartition()`, could we make it return the default storage from the profile? 
```
content::StoragePartition* WebAuthFlow::GetGuestPartition() {
  if (base::FeatureList::IsEnabled(kWebAuthFlowInBrowserTab))
    return profile_->GetDefaultStoragePartition();

  return profile_->GetStoragePartition(
      GetWebViewPartitionConfig(partition_, profile_));
}
```

And leave `GetWebViewPartitionConfig()` as is.

File chrome/browser/extensions/api/identity/web_auth_flow.cc:

- Patch Set #4, Line 113: using_auth_with_tab_(false)

- nit: could you please modernize this code and default initialize variables in a .h file instead? […]
  Done

Patch Set #4, Line 195:

 if (using_auth_with_tab_ && auth_params_.tab_web_contents_ptr_) {
    auth_params_.tab_web_contents_ptr_->ClosePage();
  }

- nit: We call `app_window_->web_contents()->Close();` in the `~WebAuthFlow()`. […]
  Initially I had an issue with using `Close()` but it seems to be fine now.

- Patch Set #4, Line 249: embedded_window_created_ || using_auth_with_tab_

- nit: I feel like this condition will be more readable if we create a helper method […]
  Done

- Patch Set #4, Line 289: void WebAuthFlow::WebContentsDestroyed() {

- It feels like we should stop observing WebContents here. […]
  Done
File chrome/browser/extensions/api/identity/web_auth_flow_browsertest.cc:
- Patch Set #4:
  I think it'd be worth to also add some test that are specific to your implementation. […]
  Thanks, I have added tests to check the tab creation, manually deleting it, and a test in silent mode to make sure no tab is created.
  I will look into adding more use cases if possible, if there is a way to mock a consent capture or something similar.

- > Once https://crrev.com/c/3996076 lands, there will be more tests around the non-interactive mode that should be parameterized.

- Thanks, noted.

- Patch Set #4, Line 67: WebAuthFlowParamBrowserTest
  nit: `WebAuthFlowInBrowserTabParamBrowserTest`

- Done

- Patch Set #4, Line 75: usetab_feature_enabled
  nit: `use_tab_feature_enabled()`

- Done

- Patch Set #4, Line 118: AuthUsingTabFeature
  nit: please use the exact feature name to avoid confusion

- Done

- Patch Set #4, Line 121: WebAuthFlowGuestPartitionParamTest

- I agree that this test suite doesn't make sense with the authInTab feature enabled. […]
  Done

- Patch Set #4, Line 224: WebAuthFlowFencedFrameTest

- I think these test can also run with your feature enabled. […]
  Done

To view, visit change 4030540. To unsubscribe, or for help writing mail filters, visit settings.

Alex Ilin (Gerrit)

unread,

Nov 18, 2022, 9:00:49 AM11/18/22

to Ryan Sultanem, chromium-a...@chromium.org, extension...@chromium.org, mparch-revi...@chromium.org, Alex Ilin, Chromium LUCI CQ, Tricium, chromium...@chromium.org

Attention is currently required from: Ryan Sultanem.

View Change

9 comments:

File chrome/browser/extensions/api/identity/web_auth_flow.h:
- Patch Set #4, Line 123: GetGuestPartition
  Instead of returning nullptr in `GetGuestPartition()`, could we make it return the default storage f […]
  This is also a valid option. We should rename `GetGuestPartition()` into something like `GetAuthTokenPartition()` in this case.
  I feel like the callers of `GetGuestPartition()` will need to handle the default partition differently in some cases (like WebAuthFlowGuestPartitionParamTest only cares about the guest partition).
File chrome/browser/extensions/api/identity/web_auth_flow.cc:
- Patch Set #8, Line 248: (
  nit: remove extra parenthesis around `IsObservingProviderWebContents()`
- Patch Set #8, Line 253: (
  ditto nit: remove extra parenthesis
- Patch Set #8, Line 258: i
  nit: start sentence with a capital letter: "If"
File chrome/browser/extensions/api/identity/web_auth_flow_browsertest.cc:
- Patch Set #4:
  Thanks, I have added tests to check the tab creation, manually deleting it, and a test in silent mod […]
  Thanks a lot for adding these tests!
  Another test idea:
  Close browser() and then trigger WebAuthFlow. Check that it creates a new browser window with `auth_url` tab.
  You need to be careful since the browser process will shut down after the last browser window is closed. To protect against that, use `ScopedKeepAlive`. [Example](https://crsrc.org/c/chrome/browser/profiles/avatar_menu_browsertest.cc;drc=8db7fa2980b8bab04f5b8ad09628ad2151fcf991;l=89).
File chrome/browser/extensions/api/identity/web_auth_flow_browsertest.cc:
- Patch Set #8, Line 355: AddObserver
  nit: clean observer at the end of the test. You can use a scoped `ScopedObservation<>` object for this.
- Patch Set #8, Line 367: EXPECT_TRUE(tab_observer.tab_added());
  optional: you can replace the tab observer with the following `tabs` checks
```
EXPECT_EQ(tabs->count(), previous_tab_count + 1);
EXPECT_EQ(tabs->GetActiveWebContents()->GetLastCommittedURL(), auth_url);
```
- Patch Set #8, Line 388:
```
// This call in not interesting because we still get to the auto_url despite
  // the failure since it is not a real auth_url, adding it to suppress the
  // warning.
```
  nit: It is expected that `OnAuthFlowURLChange(auth_url)` will be called first because the navigation happens before the page loads. I should admit I don't fully understand explanation in this comment, so I'd simply remove it.
- Patch Set #8, Line 398: EXPECT_EQ(tab_observer.inserted_url(), GURL());
  nit: this check is unnecessary. It tests some behavior in `TabInsertedTestObserver` rather than `WebAuthFlow`.

To view, visit change 4030540. To unsubscribe, or for help writing mail filters, visit settings.

Ryan Sultanem (Gerrit)

unread,

Nov 21, 2022, 10:17:19 AM11/21/22

to chromium-a...@chromium.org, extension...@chromium.org, mparch-revi...@chromium.org, Alex Ilin, Chromium LUCI CQ, Tricium, chromium...@chromium.org

View Change

7 comments:

File chrome/browser/extensions/api/identity/web_auth_flow.cc:

- Patch Set #8, Line 248: (
  nit: remove extra parenthesis around `IsObservingProviderWebContents()`

- Done

- Patch Set #8, Line 253: (
  ditto nit: remove extra parenthesis

- Done

- Patch Set #8, Line 258: i
  nit: start sentence with a capital letter: "If"

- Done
File chrome/browser/extensions/api/identity/web_auth_flow_browsertest.cc:

- Patch Set #8, Line 355: AddObserver

- nit: clean observer at the end of the test. […]
  Not using the Observer anymore, but thanks for the information, noted.

- Patch Set #8, Line 367: EXPECT_TRUE(tab_observer.tab_added());

- optional: you can replace the tab observer with the following `tabs` checks […]
  I had done that exactly at first, but it felt odd to simply check a count increment (even though effectively the same is happening through the observer).
  It does make the test simpler though.
  I will revert to the count then.

Patch Set #8, Line 388:

// This call in not interesting because we still get to the auto_url despite
  // the failure since it is not a real auth_url, adding it to suppress the
  // warning.

- nit: It is expected that `OnAuthFlowURLChange(auth_url)` will be called first because the navigation […]
  Done

- Patch Set #8, Line 398: EXPECT_EQ(tab_observer.inserted_url(), GURL());

- nit: this check is unnecessary. […]
  Done

To view, visit change 4030540. To unsubscribe, or for help writing mail filters, visit settings.

Alex Ilin (Gerrit)

unread,

Nov 21, 2022, 12:07:01 PM11/21/22

to Ryan Sultanem, chromium-a...@chromium.org, extension...@chromium.org, mparch-revi...@chromium.org, Alex Ilin, Chromium LUCI CQ, Tricium, chromium...@chromium.org

Attention is currently required from: Ryan Sultanem.

View Change

1 comment:

Patchset:
- Patch Set #8:
  Did you forget to upload the latest patchset when releasing the comments?

To view, visit change 4030540. To unsubscribe, or for help writing mail filters, visit settings.

Ryan Sultanem (Gerrit)

unread,

Nov 21, 2022, 12:26:22 PM11/21/22

to chromium-a...@chromium.org, extension...@chromium.org, mparch-revi...@chromium.org, Alex Ilin, Chromium LUCI CQ, Tricium, chromium...@chromium.org

View Change

1 comment:

Patchset:

- Patch Set #8:
  Did you forget to upload the latest patchset when releasing the comments?

- Sorry this happened again, you re right.
  Anyway the CL is not ready to be reviewed, I am still implementing the test with the closed browser.

To view, visit change 4030540. To unsubscribe, or for help writing mail filters, visit settings.

Alex Ilin (Gerrit)

unread,

Nov 21, 2022, 4:54:53 PM11/21/22

to Ryan Sultanem, chromium-a...@chromium.org, extension...@chromium.org, mparch-revi...@chromium.org, Alex Ilin, Chromium LUCI CQ, Tricium, chromium...@chromium.org

Attention is currently required from: Ryan Sultanem.

View Change

1 comment:

Patchset:
- Patch Set #8:
  Sorry this happened again, you re right. […]
  No worries at all. I just wanted to make sure that I'm not blocking you.

To view, visit change 4030540. To unsubscribe, or for help writing mail filters, visit settings.

Ryan Sultanem (Gerrit)

unread,

Nov 22, 2022, 6:48:07 AM11/22/22

to chromium-a...@chromium.org, extension...@chromium.org, mparch-revi...@chromium.org, Alex Ilin, Chromium LUCI CQ, Tricium, chromium...@chromium.org

Attention is currently required from: Alex Ilin.

View Change

2 comments:

File chrome/browser/extensions/api/identity/web_auth_flow.h:

- Patch Set #4, Line 123: GetGuestPartition

- This is also a valid option. […]
  I reverted back to your initial approach, I felt like it was more appropriate.
File chrome/browser/extensions/api/identity/web_auth_flow_browsertest.cc:
- Patch Set #4:
  Thanks a lot for adding these tests! […]
  Done

To view, visit change 4030540. To unsubscribe, or for help writing mail filters, visit settings.

Alex Ilin (Gerrit)

unread,

Nov 22, 2022, 7:05:09 AM11/22/22

to Ryan Sultanem, chromium-a...@chromium.org, extension...@chromium.org, mparch-revi...@chromium.org, Alex Ilin, Chromium LUCI CQ, Tricium, chromium...@chromium.org

Attention is currently required from: Ryan Sultanem.

Patch set 11:Code-Review +1

View Change

4 comments:

Patchset:
- Patch Set #11:
  Your CL LGTM

File chrome/browser/extensions/api/identity/gaia_remote_consent_flow.cc:

Patch Set #11, Line 165: storage_partition = profile_->GetDefaultStoragePartition();

nit: I'd also add a DCHECK here:

```
if (!storage_partition) {
  // `web_flow_` doesn't have a guest partition only when the Auth Through 
  // Browser Tab flow is used.
  DCHECK(base::FeatureList::IsEnabled(kWebAuthFlowInBrowserTab));
  storage_partition = profile_->GetDefaultStoragePartition();
}
```

File chrome/browser/extensions/api/identity/web_auth_flow_browsertest.cc:
- Patch Set #8, Line 367: EXPECT_TRUE(tab_observer.tab_added());
  I had done that exactly at first, but it felt odd to simply check a count increment (even though eff […]
  The option with an observer would be more appropriate if you need to asynchronously wait for some event. For example, `navigation_observer` in this test is needed for waiting until the page loads.
  A tab is added synchronously after the page load, so having an extra observer is unnecessary.
File chrome/browser/extensions/api/identity/web_auth_flow_browsertest.cc:
- Patch Set #11, Line 362: InteractiveNoBrowser_WebAuthCreatesBrowserWithTab
  Perfect! Thanks a lot for adding this!

To view, visit change 4030540. To unsubscribe, or for help writing mail filters, visit settings.

Ryan Sultanem (Gerrit)

unread,

Nov 22, 2022, 11:02:54 AM11/22/22

to chromium-a...@chromium.org, extension...@chromium.org, mparch-revi...@chromium.org, Alex Ilin, Chromium LUCI CQ, Tricium, chromium...@chromium.org

View Change

1 comment:

File chrome/browser/extensions/api/identity/gaia_remote_consent_flow.cc:

- Patch Set #11, Line 165: storage_partition = profile_->GetDefaultStoragePartition();

- nit: I'd also add a DCHECK here: […]
  Done

To view, visit change 4030540. To unsubscribe, or for help writing mail filters, visit settings.

Ryan Sultanem (Gerrit)

unread,

Nov 22, 2022, 11:37:32 AM11/22/22

to chromium-a...@chromium.org, extension...@chromium.org, mparch-revi...@chromium.org, Alex Ilin, Chromium LUCI CQ, Tricium, chromium...@chromium.org

Attention is currently required from: Ryan Sultanem.

Patch set 12:Commit-Queue +2

View Change

To view, visit change 4030540. To unsubscribe, or for help writing mail filters, visit settings.

Chromium LUCI CQ (Gerrit)

unread,

Nov 22, 2022, 11:38:30 AM11/22/22

to Ryan Sultanem, chromium-a...@chromium.org, extension...@chromium.org, mparch-revi...@chromium.org, Alex Ilin, Tricium, chromium...@chromium.org

Chromium LUCI CQ submitted this change.

View Change



11 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:

```
The name of the file: chrome/browser/extensions/api/identity/gaia_remote_consent_flow.cc
Insertions: 5, Deletions: 1.

@@ -161,8 +161,12 @@
 
 content::StoragePartition* GaiaRemoteConsentFlow::GetStoragePartition() {
   content::StoragePartition* storage_partition = web_flow_->GetGuestPartition();
-  if (!storage_partition)
+  if (!storage_partition) {
+    // `web_flow_` doesn't have a guest partition only when the Auth Through
+    // Browser Tab flow is used.
+    DCHECK(base::FeatureList::IsEnabled(kWebAuthFlowInBrowserTab));
     storage_partition = profile_->GetDefaultStoragePartition();
+  }
 
   return storage_partition;
 }
```

Approvals:
  Ryan Sultanem: Commit
  Alex Ilin: Looks good to me

launchWebAuthFlow to do authentication through tab

Created the feature flag `kAuthWithWebContentTab` to separate the new behavior.
For the `launchWebAuthFlow` flow, `WebAuthFlow` now tries to authenticate using a web content displayed in a new tab instead of an app window.

Change-Id: I46d1b6cb8e4a43eed7ea63afef56aee85cc5e0e4

Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4030540
Reviewed-by: Alex Ilin <alex...@chromium.org>
Commit-Queue: Ryan Sultanem <rs...@google.com>
Cr-Commit-Position: refs/heads/main@{#1074688}
---
M chrome/browser/extensions/api/identity/gaia_remote_consent_flow.cc
M chrome/browser/extensions/api/identity/gaia_remote_consent_flow.h


M chrome/browser/extensions/api/identity/web_auth_flow.cc
M chrome/browser/extensions/api/identity/web_auth_flow.h
M chrome/browser/extensions/api/identity/web_auth_flow_browsertest.cc

5 files changed, 288 insertions(+), 36 deletions(-)

To view, visit change 4030540. To unsubscribe, or for help writing mail filters, visit settings.

Reply all

Reply to author

Forward