Skip to content

Commit

Permalink
Revert "webauthn: don't prelink in work profiles."
Browse files Browse the repository at this point in the history
This reverts commit d0cc56e.

Pinpoint thinks this slowed things down: crbug.com/1459794. It's possible, even though this does work on a background thread, as I don't know how expensive this query is under the covers. Trying a revert to see what happens to the benchmark.

BUG=1459794

Original change's description:
> webauthn: don't prelink in work profiles.
>
> When Chrome is running in an Android work profile, it would still
> publish prelinking information. This can cause Chrome on the desktop to
> select that instance of Chrome to connect to for hybrid transactions,
> but from inside of a work profile, only work profile passkeys can be
> used. Thus this change suppresses publishing any pre-linking information
> from inside a work profile.
>
> Change-Id: I4ae114e5337af9dcab77dcc46e317fc739a986c1
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4656022
> Commit-Queue: Adam Langley <agl@chromium.org>
> Code-Coverage: Findit <findit-for-me@appspot.gserviceaccount.com>
> Reviewed-by: Ken Buchanan <kenrb@chromium.org>
> Cr-Commit-Position: refs/heads/main@{#1165001}

Change-Id: If589c06370868fc5cd14707ed26b41ae3445d14c
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4662505
Commit-Queue: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Auto-Submit: Adam Langley <agl@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1165070}
  • Loading branch information
Adam Langley authored and Chromium LUCI CQ committed Jul 1, 2023
1 parent 50793f0 commit 8c4a5b4
Show file tree
Hide file tree
Showing 10 changed files with 2 additions and 103 deletions.
1 change: 0 additions & 1 deletion chrome/browser/webauthn/android/BUILD.gn
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@ android_library("java") {
"//build/android:build_java",
"//chrome/android:chrome_app_java_resources",
"//chrome/android/modules/cablev2_authenticator/public:java",
"//chrome/browser/enterprise/util:java",
"//chrome/browser/notifications:java",
"//components/browser_ui/notifications/android:java",
"//components/externalauth/android:java",
Expand Down
37 changes: 0 additions & 37 deletions chrome/browser/webauthn/android/cable_module_android.cc
Original file line number Diff line number Diff line change
Expand Up @@ -93,16 +93,6 @@ class SystemInterface : public RegistrationState::SystemInterface {
std::move(callback));
}

void AmInWorkProfile(base::OnceCallback<void(bool)> callback) override {
DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
// This Java function must run on the UI thread, but that's ok because it
// defers work to a worker thread itself.
work_profile_callback_ = std::move(callback);
Java_CableAuthenticatorModuleProvider_amInWorkProfile(
base::android::AttachCurrentThread(),
reinterpret_cast<uintptr_t>(this));
}

void CalculateIdentityKey(
const std::array<uint8_t, 32>& secret,
base::OnceCallback<void(bssl::UniquePtr<EC_KEY>)> callback) override {
Expand Down Expand Up @@ -153,13 +143,6 @@ class SystemInterface : public RegistrationState::SystemInterface {
std::move(prelink_callback_).Run(std::move(cbor));
}

// Called when the Java code has finished checking if we're running in a work
// profile.
void OnHaveWorkProfileResult(bool in_work_profile) {
DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
std::move(work_profile_callback_).Run(in_work_profile);
}

private:
static instance_id::InstanceIDDriver* GetDriver() {
return instance_id::InstanceIDProfileServiceFactory::GetForProfile(
Expand Down Expand Up @@ -191,7 +174,6 @@ class SystemInterface : public RegistrationState::SystemInterface {

base::OnceCallback<void(absl::optional<std::vector<uint8_t>>)>
prelink_callback_;
base::OnceCallback<void(bool)> work_profile_callback_;
};

RegistrationState* GetRegistrationState() {
Expand Down Expand Up @@ -301,12 +283,6 @@ GetSyncDataIfRegistered() {
return absl::nullopt;
}

if (state->am_in_work_profile()) {
// Never publish pre-linking information when in a work profile, instead
// route hybrid requests into the main profile.
return absl::nullopt;
}

if (state->prelink_play_services() && state->link_data_from_play_services()) {
absl::optional<syncer::DeviceInfo::PhoneAsASecurityKeyInfo> paask_info =
internal::PaaskInfoFromCBOR(*state->link_data_from_play_services());
Expand Down Expand Up @@ -405,19 +381,6 @@ static void JNI_CableAuthenticatorModuleProvider_OnHaveLinkingInformation(
std::move(optional_cbor)));
}

static void JNI_CableAuthenticatorModuleProvider_OnHaveWorkProfileResult(
JNIEnv* env,
jlong system_interface_pointer,
jboolean in_work_profile) {
content::BrowserThread::GetTaskRunnerForThread(content::BrowserThread::UI)
->PostTask(
FROM_HERE,
base::BindOnce(&SystemInterface::OnHaveWorkProfileResult,
base::Unretained(reinterpret_cast<SystemInterface*>(
static_cast<uintptr_t>(system_interface_pointer))),
in_work_profile));
}

static void JNI_PrivacySettingsFragment_RevokeAllLinkedDevices(JNIEnv* env) {
// Invalidates the current cloud messaging (GCM) token and creates a new one.
// This causes the tunnel server to reject connection attempts with a 410
Expand Down
11 changes: 2 additions & 9 deletions chrome/browser/webauthn/android/cable_registration_state.cc
Original file line number Diff line number Diff line change
Expand Up @@ -42,8 +42,6 @@ void RegistrationState::Register() {
base::BindRepeating(&RegistrationState::OnEvent, base::Unretained(this)));
interface_->CanDeviceSupportCable(base::BindOnce(
&RegistrationState::OnDeviceSupportResult, base::Unretained(this)));
interface_->AmInWorkProfile(base::BindOnce(
&RegistrationState::OnWorkProfileResult, base::Unretained(this)));

std::string secret_base64 = interface_->GetRootSecret();

Expand Down Expand Up @@ -71,8 +69,8 @@ void RegistrationState::Register() {
// put information into sync's DeviceInfo.
bool RegistrationState::have_data_for_sync() const {
return device_supports_cable_.has_value() && identity_key_ &&
am_in_work_profile_.has_value() && sync_registration_ != nullptr &&
sync_registration_->contact_id() && have_play_services_data();
sync_registration_ != nullptr && sync_registration_->contact_id() &&
have_play_services_data();
}

void RegistrationState::SignalSyncWhenReady() {
Expand Down Expand Up @@ -202,11 +200,6 @@ void RegistrationState::OnDeviceSupportResult(bool result) {
MaybeSignalSync();
}

void RegistrationState::OnWorkProfileResult(bool result) {
am_in_work_profile_ = result;
MaybeSignalSync();
}

void RegistrationState::OnIdentityKeyReady(
bssl::UniquePtr<EC_KEY> identity_key) {
identity_key_ = std::move(identity_key);
Expand Down
10 changes: 0 additions & 10 deletions chrome/browser/webauthn/android/cable_registration_state.h
Original file line number Diff line number Diff line change
Expand Up @@ -53,9 +53,6 @@ class RegistrationState {
virtual void CanDeviceSupportCable(
base::OnceCallback<void(bool)> callback) = 0;

// Test whether the current process is an in Android work profile.
virtual void AmInWorkProfile(base::OnceCallback<void(bool)> callback) = 0;

// Generate a P-256 key pair from a seed.
virtual void CalculateIdentityKey(
const std::array<uint8_t, 32>& secret,
Expand Down Expand Up @@ -94,7 +91,6 @@ class RegistrationState {
const EC_KEY* identity_key() const { return identity_key_.get(); }
bool device_supports_cable() const { return *device_supports_cable_; }
bool prelink_play_services() const { return prelink_play_services_; }
bool am_in_work_profile() const { return *am_in_work_profile_; }
const absl::optional<std::vector<uint8_t>>& link_data_from_play_services()
const {
DCHECK(prelink_play_services_);
Expand Down Expand Up @@ -134,9 +130,6 @@ class RegistrationState {
// OnCanDeviceSupportCable is run with the result of `TestDeviceSupport`.
void OnDeviceSupportResult(bool result);

// OnWorkProfileResult is run with the result of `AmInWorkProfile`.
void OnWorkProfileResult(bool result);

// OnIdentityKeyReady is run with the result of `CalculateIdentityKey`.
void OnIdentityKeyReady(bssl::UniquePtr<EC_KEY> identity_key);

Expand All @@ -160,9 +153,6 @@ class RegistrationState {
// always use a QR code if pre-linking hasn't worked by the time they need
// it.
absl::optional<bool> device_supports_cable_;
// am_in_work_profile_ stores whether the current process is in an Android
// work profile.
absl::optional<bool> am_in_work_profile_;
// link_data_from_play_services_ contains the response from Play Services, as
// CBOR-encoded linking information, or `nullopt` if the call was
// unsuccessful. This field is only meaningful if
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -71,11 +71,6 @@ class TestSystemInterface : public RegistrationState::SystemInterface {
support_callback_ = std::move(callback);
}

void AmInWorkProfile(base::OnceCallback<void(bool)> callback) override {
CHECK(!work_profile_callback_);
work_profile_callback_ = std::move(callback);
}

void CalculateIdentityKey(
const std::array<uint8_t, 32>& secret,
base::OnceCallback<void(bssl::UniquePtr<EC_KEY>)> callback) override {
Expand Down Expand Up @@ -106,7 +101,6 @@ class TestSystemInterface : public RegistrationState::SystemInterface {
bool refresh_local_device_info_called_ = false;

base::OnceCallback<void(bool)> support_callback_;
base::OnceCallback<void(bool)> work_profile_callback_;
base::OnceCallback<void(bssl::UniquePtr<EC_KEY>)> identity_key_callback_;
base::OnceCallback<void(absl::optional<std::vector<uint8_t>>)>
prelink_callback_;
Expand Down Expand Up @@ -165,13 +159,9 @@ TEST_F(CableRegistrationStateTest, HaveDataForSync) {
state_->SignalSyncWhenReady();
EXPECT_FALSE(state_->have_data_for_sync());
std::move(interface_->prelink_callback_).Run(absl::nullopt);
EXPECT_FALSE(interface_->refresh_local_device_info_called_);
EXPECT_FALSE(state_->have_data_for_sync());
std::move(interface_->work_profile_callback_).Run(true);
EXPECT_TRUE(interface_->refresh_local_device_info_called_);
EXPECT_TRUE(state_->have_data_for_sync());
EXPECT_TRUE(state_->device_supports_cable());
EXPECT_TRUE(state_->am_in_work_profile());
EXPECT_FALSE(state_->link_data_from_play_services().has_value());
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -34,12 +34,10 @@
import org.chromium.base.ContextUtils;
import org.chromium.base.Log;
import org.chromium.base.PackageUtils;
import org.chromium.base.ThreadUtils;
import org.chromium.base.annotations.CalledByNative;
import org.chromium.base.annotations.NativeMethods;
import org.chromium.base.task.PostTask;
import org.chromium.base.task.TaskTraits;
import org.chromium.chrome.browser.enterprise.util.EnterpriseInfo;
import org.chromium.chrome.browser.notifications.NotificationConstants;
import org.chromium.chrome.browser.notifications.NotificationWrapperBuilderFactory;
import org.chromium.chrome.browser.notifications.channels.ChromeChannelDefinitions;
Expand Down Expand Up @@ -255,24 +253,6 @@ public static boolean canDeviceSupportCable() {
return NotificationManagerCompat.from(context).areNotificationsEnabled();
}

/**
* Calls back into native code with whether we are running in a work profile.
*/
@CalledByNative
public static void amInWorkProfile(long pointer) {
if (!DeviceFeatureMap.isEnabled(DeviceFeatureList.WEBAUTHN_DONT_PRELINK_IN_PROFILES)) {
CableAuthenticatorModuleProviderJni.get().onHaveWorkProfileResult(pointer, false);
return;
}

ThreadUtils.assertOnUiThread();
EnterpriseInfo enterpriseInfo = EnterpriseInfo.getInstance();
enterpriseInfo.getDeviceEnterpriseInfo(
(state)
-> CableAuthenticatorModuleProviderJni.get().onHaveWorkProfileResult(
pointer, state.mProfileOwned));
}

@CalledByNative
public static void getLinkingInformation(long pointer) {
boolean ok = true;
Expand Down Expand Up @@ -325,8 +305,5 @@ interface Natives {
// Services. The argument is a CBOR-encoded linking structure, as defined in CTAP 2.2, or is
// null on error.
void onHaveLinkingInformation(long pointer, byte[] cbor);
// onHaveWorkProfileResult is called when it has been determined if
// Chrome is running in a work profile or not.
void onHaveWorkProfileResult(long pointer, boolean inWorkProfile);
}
}
5 changes: 0 additions & 5 deletions device/fido/features.cc
Original file line number Diff line number Diff line change
Expand Up @@ -128,9 +128,4 @@ BASE_FEATURE(kWebAuthnCableViaCredMan,
"WebAuthenticationCableViaCredMan",
base::FEATURE_ENABLED_BY_DEFAULT);

// Enabled in M117. Remove in or after M120.
BASE_FEATURE(kWebAuthnDontPrelinkInProfiles,
"WebAuthenticationDontPrelinkInProfiles",
base::FEATURE_ENABLED_BY_DEFAULT);

} // namespace device
5 changes: 0 additions & 5 deletions device/fido/features.h
Original file line number Diff line number Diff line change
Expand Up @@ -98,11 +98,6 @@ BASE_DECLARE_FEATURE(kWebAuthConditionalUIExperimentation);
COMPONENT_EXPORT(DEVICE_FIDO)
BASE_DECLARE_FEATURE(kWebAuthnCableViaCredMan);

// Don't publish prelinking information if Chrome is running in a work profile
// on Android.
COMPONENT_EXPORT(DEVICE_FIDO)
BASE_DECLARE_FEATURE(kWebAuthnDontPrelinkInProfiles);

} // namespace device

#endif // DEVICE_FIDO_FEATURES_H_
1 change: 0 additions & 1 deletion services/device/public/cpp/device_feature_map.cc
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@ const base::Feature* const kFeaturesExposedToJava[] = {
&device::kWebAuthnAndroidCredMan,
&device::kWebAuthnAndroidHybridClientUi,
&device::kWebAuthnCableViaCredMan,
&device::kWebAuthnDontPrelinkInProfiles,
&device::kWebAuthnHybridLinkWithoutNotifications,
&kGenericSensorExtraClasses,
};
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,6 @@ public abstract class DeviceFeatureList {
public static final String WEBAUTHN_ANDROID_HYBRID_CLIENT_UI =
"WebAuthenticationAndroidHybridClientUi";
public static final String WEBAUTHN_CABLE_VIA_CREDMAN = "WebAuthenticationCableViaCredMan";
public static final String WEBAUTHN_DONT_PRELINK_IN_PROFILES =
"WebAuthenticationDontPrelinkInProfiles";
public static final String WEBAUTHN_HYBRID_LINK_WITHOUT_NOTIFICATIONS =
"WebAuthenticationHybridLinkWithoutNotifications";
}

0 comments on commit 8c4a5b4

Please sign in to comment.