[remoting]: Remove enterprise params from JSON message interface. [chromium/src : main]
Joe Downing (Gerrit)
Joe Downing added 1 comment![]( "Open in Gerrit")
Related details
- Yuwei Huang
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Yuwei Huang (Gerrit)
Yuwei Huang voted and added 3 comments![]( "Open in Gerrit")
Votes added by Yuwei Huang
| Code-Review | +1 |
3 comments
LGTM w/ AI comments that look valid to me.
if (reconnect_params_.has_value()) {Since `reconnect_params_` is now a member variable, you can eliminate the local `std::optional<ReconnectParams> reconnect_params;` entirely and avoid an unnecessary copy.
Also, doing the `CHECK` here means the host will crash if `reconnect_params_` is set but `enterprise_params_` is not. In the previous logic, reconnect parameters were gracefully ignored if it wasn't an enterprise session.
Consider removing the local `std::optional<ReconnectParams>` and simplifying this section to just:
```cpp
#if BUILDFLAG(IS_CHROMEOS) || !defined(NDEBUG)
bool is_enterprise_admin_user = enterprise_params_.has_value();
#endif
```
if (reconnect_params.has_value()) {If you remove the local `reconnect_params` above, you can update this block to use the member variable directly and safely perform the `CHECK` here since we are inside the `is_enterprise_admin_user` block:
```cpp
if (reconnect_params_.has_value()) {
CHECK(enterprise_params_->allow_reconnections);
it2me_host_->set_reconnect_params(*reconnect_params_);
}
```
Related details
- Joe Downing
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. |
Joe Downing (Gerrit)
Joe Downing voted and added 3 comments![]( "Open in Gerrit")
Votes added by Joe Downing
| Commit-Queue | +2 |
3 comments
Since `reconnect_params_` is now a member variable, you can eliminate the local `std::optional<ReconnectParams> reconnect_params;` entirely and avoid an unnecessary copy.
Also, doing the `CHECK` here means the host will crash if `reconnect_params_` is set but `enterprise_params_` is not. In the previous logic, reconnect parameters were gracefully ignored if it wasn't an enterprise session.
Consider removing the local `std::optional<ReconnectParams>` and simplifying this section to just:
```cpp
#if BUILDFLAG(IS_CHROMEOS) || !defined(NDEBUG)
bool is_enterprise_admin_user = enterprise_params_.has_value();
#endif
```
I was on the fence about the AI suggestion about the CHECK condition but I think the local var cleanup is a good improvement so I'll apply both.
If you remove the local `reconnect_params` above, you can update this block to use the member variable directly and safely perform the `CHECK` here since we are inside the `is_enterprise_admin_user` block:
```cpp
if (reconnect_params_.has_value()) {
CHECK(enterprise_params_->allow_reconnections);
it2me_host_->set_reconnect_params(*reconnect_params_);
}
```
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change with unreviewed changes![]( "Open in Gerrit")
Unreviewed changes
7 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: remoting/host/it2me/it2me_native_messaging_host.cc
Insertions: 5, Deletions: 9.
@@ -299,13 +299,8 @@
}
}
- std::optional<ReconnectParams> reconnect_params;
#if BUILDFLAG(IS_CHROMEOS) || !defined(NDEBUG)
bool is_enterprise_admin_user = enterprise_params_.has_value();
- if (reconnect_params_.has_value()) {
- CHECK(enterprise_params_ && enterprise_params_->allow_reconnections);
- reconnect_params.emplace(*reconnect_params_);
- }
#endif
It2MeHost::CreateDeferredConnectContext create_connection_context;
@@ -332,8 +327,8 @@
api_token_getter_.set_access_token(access_token);
}
std::string ftl_device_id;
- if (reconnect_params.has_value()) {
- ftl_device_id = reconnect_params->ftl_device_id;
+ if (reconnect_params_.has_value()) {
+ ftl_device_id = reconnect_params_->ftl_device_id;
}
bool is_corp_user = message.FindBool(kIsCorpUser).value_or(false);
create_connection_context = base::BindOnce(
@@ -378,8 +373,9 @@
dialog_style = It2MeConfirmationDialog::DialogStyle::kEnterprise;
- if (reconnect_params.has_value()) {
- it2me_host_->set_reconnect_params(std::move(*reconnect_params));
+ if (reconnect_params_.has_value()) {
+ CHECK(enterprise_params_->allow_reconnections);
+ it2me_host_->set_reconnect_params(*reconnect_params_);
}
}
#endif
```
Change information
[remoting]: Remove enterprise params from JSON message interface.
This change refactors some code to prevent enterprise options from
being injected via the NMH JSON interface since enterprise sessions
have been using the Mojo interface for several years and no longer
rely on that codepath.
- M remoting/host/it2me/it2me_native_messaging_host.cc
- M remoting/host/it2me/it2me_native_messaging_host.h
- M remoting/host/it2me/it2me_native_messaging_host_ash.cc
- M remoting/host/it2me/it2me_native_messaging_host_ash.h
- M remoting/host/it2me/it2me_native_messaging_host_unittest.cc
- M remoting/host/it2me/reconnect_params.cc
- M remoting/host/it2me/reconnect_params.h
Code-Review: +1 by Yuwei Huang
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |