Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
242983004:
Disable some API calls in networkingPrivate for non-primary user (Closed)
|
Created:
6 years, 8 months ago by stevenjb Modified:
6 years, 6 months ago Reviewers:
pneubeck (no reviews) CC:
chromium-reviews, extensions-reviews_chromium.org, nkostylev+watch_chromium.org, oshima+watch_chromium.org, chromium-apps-reviews_chromium.org, stevenjb+watch_chromium.org, davemoore+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionDisable some API calls in networkingPrivate for non-primary user
We will disable createNetwork, and getManagedProperties if this is called from the non-primary user because these methods currently require a user id hash.
This does not address other methods (e.g. setProperties) which should probably also take a user id hash and enforce policy based on it.
BUG=364922
R=pneubeck@chromium.org
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=277103
Patch Set 1 #
Total comments: 2
Patch Set 2 : Fix comment #Patch Set 3 : Fail to configure networks from non-primary user #
Total comments: 13
Patch Set 4 : Adddress feedback #
Total comments: 2
Patch Set 5 : Remove handling of LoginState -> none #Patch Set 6 : Fix test #
Messages
Total messages: 21 (0 generated)
This should both fix the behavior of this and reduce the chrome dependencies (which will eventually allow us to move NetworkingPrivateApi out of src/chrome so that AppShell, etc can use it).
https://codereview.chromium.org/242983004/diff/1/chrome/browser/extensions/ap... File chrome/browser/extensions/api/networking_private/networking_private_api_chromeos.cc (right): https://codereview.chromium.org/242983004/diff/1/chrome/browser/extensions/ap... chrome/browser/extensions/api/networking_private/networking_private_api_chromeos.cc:61: // Currently we always configure networks for the *primary* logged in user. If 'we' is 'chrome://settings' then yes. But this was meant to become a public API. This change is I think not acceptable for a public API. I asked the same question on the bug tracker, we can discuss there at first.
https://codereview.chromium.org/242983004/diff/1/chrome/browser/extensions/ap... File chrome/browser/extensions/api/networking_private/networking_private_api_chromeos.cc (right): https://codereview.chromium.org/242983004/diff/1/chrome/browser/extensions/ap... chrome/browser/extensions/api/networking_private/networking_private_api_chromeos.cc:61: // Currently we always configure networks for the *primary* logged in user. On 2014/04/19 09:17:43, pneubeck wrote: > If 'we' is 'chrome://settings' then yes. > But this was meant to become a public API. This change is I think not acceptable > for a public API. > I asked the same question on the bug tracker, we can discuss there at first. Actually, "we" was referring to Chrome OS behavior in general. I will confer with zel@ and kuscher@ tomorrow, but I don't believe that configuring a network for a non primary user is something we have any plans to support on the Chrome OS platform. I will clarify the comment. Also, if I recall correctly, the networkingPrivate API only supports the concept of "shared", so we are not breaking that. The behavior of configuring a network from an extension installed by User/Profile A vs. User/Profile B on a platform that supports multiple profiles is undocumented. On Win/Mac any such configuration will be system-wide. On Chrome OS the current design always affects the primary user.
OK, getting back to this. I think that the right thing to do really is to fail to configure a new non-shared network from a non primary user profile.
On 2014/06/11 23:36:06, stevenjb wrote: > OK, getting back to this. I think that the right thing to do really is to fail > to configure a new non-shared network from a non primary user profile. sounds good to me. What about reading/listing of non-shared networks from a non-primary user profile? For security/privacy reasons (so this doesn't necessarily apply to the settings page), I would hide any non-shared information.
https://codereview.chromium.org/242983004/diff/40001/chrome/browser/extension... File chrome/browser/extensions/api/networking_private/networking_private_api_chromeos.cc (right): https://codereview.chromium.org/242983004/diff/40001/chrome/browser/extension... chrome/browser/extensions/api/networking_private/networking_private_api_chromeos.cc:153: // for the purposes of displaying network properites. |user_id_hash| will typo: properites -> properties https://codereview.chromium.org/242983004/diff/40001/chrome/browser/extension... chrome/browser/extensions/api/networking_private/networking_private_api_chromeos.cc:155: // included. the MNCH wasn't written with empty user hashes in mind. This usage will require an audit / more tests / updated comments of MNCH. https://codereview.chromium.org/242983004/diff/40001/chromeos/login/login_sta... File chromeos/login/login_state.cc (right): https://codereview.chromium.org/242983004/diff/40001/chromeos/login/login_sta... chromeos/login/login_state.cc:77: if (type == LOGGED_IN_USER_NONE) don't we assume that we never logout? https://codereview.chromium.org/242983004/diff/40001/chromeos/login/login_sta... File chromeos/login/login_state.h (right): https://codereview.chromium.org/242983004/diff/40001/chromeos/login/login_sta... chromeos/login/login_state.h:54: // Sets the logged in state, user type, and primary user hash. Also notifies I find this interface a bit odd in the light of multiple users / logins. Could you either clarify what is supposed to be called on each login if multiprofile is available? Would it be safer to have only a single function which is to be called on the first login no matter if we use multiprofile or not (or maybe even on every login); so that it forces the caller to think about the correct arguments? Don't we still ignore logouts completely? Shouldn't the interface convey that too (at least comment)? E.g. // Sets LoggedInState to LOGGED_IN_ACTIVE. Must be called at first // login and at most once during the lifetime of this object. SetFirstLogin(LoggedInUserType type, const std::string& user_hash); // Must be called on each login after the initial login during the // same sesson. SetSecondaryLogins(const std::string& user_hash); alternatively, a single function: // Must be called on every login. Secondary logins must match the // type of the primary login. OnLogin(LoggedInUserType type, const std::string& user_hash); (not sure what a good prefix is: On-/Set-/...?)
https://codereview.chromium.org/242983004/diff/40001/chrome/browser/extension... File chrome/browser/extensions/api/networking_private/networking_private_api_chromeos.cc (right): https://codereview.chromium.org/242983004/diff/40001/chrome/browser/extension... chrome/browser/extensions/api/networking_private/networking_private_api_chromeos.cc:155: // included. On 2014/06/12 09:51:00, pneubeck wrote: > the MNCH wasn't written with empty user hashes in mind. This usage will require > an audit / more tests / updated comments of MNCH. Thinking more about it, MNCH shouldn't ever get an empty user hash so that we can ensure consistent policy enforcement. All restrictions that we want to apply in the extension API should happen here and shouldn't be able to undo what MNCH decides (what policies enforced). That means, we can either pass a boolean primary_user (together with the a non-empty hash) to the MNCH and make it multi-profile aware. Or, we add the additional restrictions here (I think I prefer this, as these restrictions seem a very extension-API specific to me and might not apply to other consumers). E.g. I think we should enforce if called from secondary profile: - GetState: always ok - CreateNetwork, SetProperties: always fail as it might lead to surprising effects because of different user policies. - GetProperties, GetManagedProperties: fail if not shared in order to not leak sensitive information / to preserve privacy - Get*Networks: maybe should drop all disconnected/invisible private networks as these are useless anyways with the above restrictions Considering how subtle this is and how much effort it would take, I would just disable the whole API for secondary profiles if there's no strong reason to do otherwise.
On 2014/06/12 06:57:29, pneubeck wrote: > On 2014/06/11 23:36:06, stevenjb wrote: > > OK, getting back to this. I think that the right thing to do really is to fail > > to configure a new non-shared network from a non primary user profile. > > sounds good to me. What about reading/listing of non-shared networks from a > non-primary user profile? For security/privacy reasons (so this doesn't > necessarily apply to the settings page), I would hide any non-shared > information. We allow connecting to non-shared networks, and this information will be available from other parts of the UI, so we definitely need to provide GetState. I suppose we could disallow Get*Properties, but I don't think it really matters one way or the other.
On 2014/06/12 10:17:48, pneubeck wrote: > https://codereview.chromium.org/242983004/diff/40001/chrome/browser/extension... > File > chrome/browser/extensions/api/networking_private/networking_private_api_chromeos.cc > (right): > > https://codereview.chromium.org/242983004/diff/40001/chrome/browser/extension... > chrome/browser/extensions/api/networking_private/networking_private_api_chromeos.cc:155: > // included. > On 2014/06/12 09:51:00, pneubeck wrote: > > the MNCH wasn't written with empty user hashes in mind. This usage will > require > > an audit / more tests / updated comments of MNCH. > > Thinking more about it, MNCH shouldn't ever get an empty user hash so that we > can ensure consistent policy enforcement. > All restrictions that we want to apply in the extension API should happen here > and shouldn't be able to undo what MNCH decides (what policies enforced). > > That means, we can either pass a boolean primary_user (together with the a > non-empty hash) to the MNCH and make it multi-profile aware. > Or, we add the additional restrictions here (I think I prefer this, as these > restrictions seem a very extension-API specific to me and might not apply to > other consumers). > > > E.g. I think we should enforce if called from secondary profile: > - GetState: always ok > - CreateNetwork, SetProperties: always fail as it might lead to surprising > effects because of different user policies. > - GetProperties, GetManagedProperties: fail if not shared in order to not leak > sensitive information / to preserve privacy > - Get*Networks: maybe should drop all disconnected/invisible private networks as > these are useless anyways with the above restrictions > > Considering how subtle this is and how much effort it would take, I would just > disable the whole API for secondary profiles if there's no strong reason to do > otherwise. When I discussed this with UX they were more concerned about confusing user experiences if someone ran an app using the API (e.g. Chromecast) from a secondary profile. I know that from your perspective policy is important, but it shouldn't be something that most users need to think about. I don't think we can disable the API entirely. In the short term I am OK with disabling Get*Properties, but I would much prefer to defer handling any policy enforcement issues to a follow up CL if you think it necessary. (Given that the system UI will always use the primary profile I am not convinced it is).
On 2014/06/12 17:33:42, stevenjb wrote: > On 2014/06/12 10:17:48, pneubeck wrote: > > > https://codereview.chromium.org/242983004/diff/40001/chrome/browser/extension... > > File > > > chrome/browser/extensions/api/networking_private/networking_private_api_chromeos.cc > > (right): > > > > > https://codereview.chromium.org/242983004/diff/40001/chrome/browser/extension... > > > chrome/browser/extensions/api/networking_private/networking_private_api_chromeos.cc:155: > > // included. > > On 2014/06/12 09:51:00, pneubeck wrote: > > > the MNCH wasn't written with empty user hashes in mind. This usage will > > require > > > an audit / more tests / updated comments of MNCH. > > > > Thinking more about it, MNCH shouldn't ever get an empty user hash so that we > > can ensure consistent policy enforcement. > > All restrictions that we want to apply in the extension API should happen here > > and shouldn't be able to undo what MNCH decides (what policies enforced). > > > > That means, we can either pass a boolean primary_user (together with the a > > non-empty hash) to the MNCH and make it multi-profile aware. > > Or, we add the additional restrictions here (I think I prefer this, as these > > restrictions seem a very extension-API specific to me and might not apply to > > other consumers). > > > > > > E.g. I think we should enforce if called from secondary profile: > > - GetState: always ok > > - CreateNetwork, SetProperties: always fail as it might lead to surprising > > effects because of different user policies. > > - GetProperties, GetManagedProperties: fail if not shared in order to not leak > > sensitive information / to preserve privacy > > - Get*Networks: maybe should drop all disconnected/invisible private networks > as > > these are useless anyways with the above restrictions > > > > Considering how subtle this is and how much effort it would take, I would just > > disable the whole API for secondary profiles if there's no strong reason to > do > > otherwise. > > When I discussed this with UX they were more concerned about confusing user > experiences if someone ran an app using the API (e.g. Chromecast) from a > secondary profile. I know that from your perspective policy is important, but it > shouldn't be something that most users need to think about. I don't think we can > disable the API entirely. > > In the short term I am OK with disabling Get*Properties, but I would much prefer > to defer handling any policy enforcement issues to a follow up CL if you think > it necessary. (Given that the system UI will always use the primary profile I am > not convinced it is). If the system UI is using the API always in the context of the primary profile, then you can ignore the policy part for now, because there shouldn't be any other consumers so far which care about policy/privacy.
please update the commit message, or disable the other mentioned functions. https://codereview.chromium.org/242983004/diff/40001/chrome/browser/extension... File chrome/browser/extensions/api/networking_private/networking_private_api_chromeos.cc (right): https://codereview.chromium.org/242983004/diff/40001/chrome/browser/extension... chrome/browser/extensions/api/networking_private/networking_private_api_chromeos.cc:154: // be empty, so no properties from the profile (e.g. proxy config) will be this comment is still wrong / misleading and brings up a good point. Is 'profile' a Shill profile or a Chrome profile? proxy config is so far not handled at all by MNCH and the extension API. As mentioned, MNCH was written with the assumption of the user hash not empty. But again, we can fix that after this commit if you like that better (then please add a todo).
https://codereview.chromium.org/242983004/diff/40001/chrome/browser/extension... File chrome/browser/extensions/api/networking_private/networking_private_api_chromeos.cc (right): https://codereview.chromium.org/242983004/diff/40001/chrome/browser/extension... chrome/browser/extensions/api/networking_private/networking_private_api_chromeos.cc:153: // for the purposes of displaying network properites. |user_id_hash| will On 2014/06/12 09:51:00, pneubeck wrote: > typo: properites -> properties Done. https://codereview.chromium.org/242983004/diff/40001/chrome/browser/extension... chrome/browser/extensions/api/networking_private/networking_private_api_chromeos.cc:154: // be empty, so no properties from the profile (e.g. proxy config) will be On 2014/06/12 18:26:18, pneubeck wrote: > this comment is still wrong / misleading and brings up a good point. > Is 'profile' a Shill profile or a Chrome profile? > proxy config is so far not handled at all by MNCH and the extension API. > As mentioned, MNCH was written with the assumption of the user hash not empty. > > But again, we can fix that after this commit if you like that better (then > please add a todo). Disallowing for now to simplify. https://codereview.chromium.org/242983004/diff/40001/chrome/browser/extension... chrome/browser/extensions/api/networking_private/networking_private_api_chromeos.cc:155: // included. On 2014/06/12 10:17:47, pneubeck wrote: > On 2014/06/12 09:51:00, pneubeck wrote: > > the MNCH wasn't written with empty user hashes in mind. This usage will > require > > an audit / more tests / updated comments of MNCH. > > Thinking more about it, MNCH shouldn't ever get an empty user hash so that we > can ensure consistent policy enforcement. > All restrictions that we want to apply in the extension API should happen here > and shouldn't be able to undo what MNCH decides (what policies enforced). > > That means, we can either pass a boolean primary_user (together with the a > non-empty hash) to the MNCH and make it multi-profile aware. > Or, we add the additional restrictions here (I think I prefer this, as these > restrictions seem a very extension-API specific to me and might not apply to > other consumers). > > > E.g. I think we should enforce if called from secondary profile: > - GetState: always ok > - CreateNetwork, SetProperties: always fail as it might lead to surprising > effects because of different user policies. > - GetProperties, GetManagedProperties: fail if not shared in order to not leak > sensitive information / to preserve privacy > - Get*Networks: maybe should drop all disconnected/invisible private networks as > these are useless anyways with the above restrictions > > Considering how subtle this is and how much effort it would take, I would just > disable the whole API for secondary profiles if there's no strong reason to do > otherwise. For now I am only disallowing calls that require a user id hash so that we can avoid an immediate audit of MNCH. If/when we expose the API publically (or expand its usage) we will want to do a more thorough security/privacy review. That said, I think that MNCSH should Do The Right Thing if an empty user id hash is passed to it (see note below). https://codereview.chromium.org/242983004/diff/40001/chrome/browser/extension... chrome/browser/extensions/api/networking_private/networking_private_api_chromeos.cc:270: std::string user_id_hash; We are already currently passing an empty user_id_hash to MNCH to indicate a shared network configuration. I think this should be handled (which may include failure if policy should disallow it) by MNCH. https://codereview.chromium.org/242983004/diff/40001/chromeos/login/login_sta... File chromeos/login/login_state.cc (right): https://codereview.chromium.org/242983004/diff/40001/chromeos/login/login_sta... chromeos/login/login_state.cc:77: if (type == LOGGED_IN_USER_NONE) On 2014/06/12 09:51:00, pneubeck wrote: > don't we assume that we never logout? Perhaps, but I would prefer to be robust here in case that were to change. https://codereview.chromium.org/242983004/diff/40001/chromeos/login/login_sta... File chromeos/login/login_state.h (right): https://codereview.chromium.org/242983004/diff/40001/chromeos/login/login_sta... chromeos/login/login_state.h:54: // Sets the logged in state, user type, and primary user hash. Also notifies On 2014/06/12 09:51:01, pneubeck wrote: > I find this interface a bit odd in the light of multiple users / logins. > > Could you either clarify what is supposed to be called on each login if > multiprofile is available? > > Would it be safer to have only a single function which is to be called on the > first login no matter if we use multiprofile or not (or maybe even on every > login); so that it forces the caller to think about the correct arguments? > > Don't we still ignore logouts completely? Shouldn't the interface convey that > too (at least comment)? > > E.g. > // Sets LoggedInState to LOGGED_IN_ACTIVE. Must be called at first > // login and at most once during the lifetime of this object. > SetFirstLogin(LoggedInUserType type, const std::string& user_hash); > > // Must be called on each login after the initial login during the > // same sesson. > SetSecondaryLogins(const std::string& user_hash); > > alternatively, a single function: > // Must be called on every login. Secondary logins must match the > // type of the primary login. > OnLogin(LoggedInUserType type, const std::string& user_hash); > > (not sure what a good prefix is: On-/Set-/...?) This should be called exactly once when a user logs in. I will add that to the comment. The reason I didn't just add "SetPrimaryUser" is that I didn't want to complicate the observer model and requiring "SetPrimaryUser" to be called before "SetLoggedInState" would be awkward. My understanding is that login/User will be getting moved (finally!) to src/chromeos at which point we should be able to get rid of this class entirely.
lgtm except the logout handling. https://codereview.chromium.org/242983004/diff/40001/chrome/browser/extension... File chrome/browser/extensions/api/networking_private/networking_private_api_chromeos.cc (right): https://codereview.chromium.org/242983004/diff/40001/chrome/browser/extension... chrome/browser/extensions/api/networking_private/networking_private_api_chromeos.cc:270: std::string user_id_hash; On 2014/06/12 20:27:51, stevenjb wrote: > We are already currently passing an empty user_id_hash to MNCH to indicate a > shared network configuration. I think this should be handled (which may include > failure if policy should disallow it) by MNCH. Whether empty user_id_hash is the right way to communicate the shared bit is debatable. But anyways, yes, this must be handled correctly by MNCH soon. https://codereview.chromium.org/242983004/diff/80001/chromeos/login/login_sta... File chromeos/login/login_state.cc (right): https://codereview.chromium.org/242983004/diff/80001/chromeos/login/login_sta... chromeos/login/login_state.cc:77: if (type == LOGGED_IN_USER_NONE) The concept of logging out does not exist in ChromeOS! Please CHECK or DCHECK but don't handle it (also don't test this). This only conveys the false information that ChromeOS does handle logouts.
https://codereview.chromium.org/242983004/diff/80001/chromeos/login/login_sta... File chromeos/login/login_state.cc (right): https://codereview.chromium.org/242983004/diff/80001/chromeos/login/login_sta... chromeos/login/login_state.cc:77: if (type == LOGGED_IN_USER_NONE) On 2014/06/13 08:51:12, pneubeck wrote: > The concept of logging out does not exist in ChromeOS! > Please CHECK or DCHECK but don't handle it (also don't test this). This only > conveys the false information that ChromeOS does handle logouts. I don't want to put a DCHECK in because I don't know what the behavior is during shutdown, especially in tests. I'll remove the clearing, that shouldn't hurt anything, but since this class merely reflects state controlled elsewhere (UserManager) I don't think it should be checking that state.
The CQ bit was checked by stevenjb@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/stevenjb@chromium.org/242983004/100001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: linux_chromium_chromeos_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
Message was sent while issue was closed.
Committed patchset #6 manually as r277103 (presubmit successful). |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
