Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10171020:
Preference Pane for chromoting on Mac (Closed)
|
Created:
8 years, 8 months ago by Lambros Modified:
8 years, 7 months ago CC:
chromium-reviews, jamiewalch+watch_chromium.org, dcaiafa+watch_chromium.org, simonmorris+watch_chromium.org, hclam+watch_chromium.org, wez+watch_chromium.org, amit, sanjeevr, garykac+watch_chromium.org, lambroslambrou+watch_chromium.org, alexeypa+watch_chromium.org, sergeyu+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionPreference Pane for chromoting on Mac
This adds a new System Preference pane, which is shown when a user configures
the Chromoting Host via the web-app.
This is basically functional, but is not yet properly integrated with the
flow of the web-app. I will prepare a separate CL that implements
notifications from the pref-pane back to the web-app.
BUG=120903, 121749
TEST=Manual
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=135934
Patch Set 1 #
Total comments: 68
Patch Set 2 : Address comments #Patch Set 3 : Address comments and fix plist MainNibFile entry #
Total comments: 16
Patch Set 4 : Address comments. #
Total comments: 2
Patch Set 5 : Fix branding #Patch Set 6 : rebase #Patch Set 7 : Define Me2MePreferencePane in header file #Patch Set 8 : Save xib in IBuilder 3.2 #Patch Set 9 : rebase, applying changes from http://codereview.chromium.org/10383003/ #Patch Set 10 : Save xib in IBuilder 3.2.6 #
Messages
Total messages: 24 (0 generated)
garykac: installer changes jamiewalch: everything else
driving by. http://codereview.chromium.org/10171020/diff/1/remoting/host/me2me_preference... File remoting/host/me2me_preference_pane-Info.plist (right): http://codereview.chromium.org/10171020/diff/1/remoting/host/me2me_preference... remoting/host/me2me_preference_pane-Info.plist:26: <string>Copyright © 2012 COPYRIGHT_BY All rights reserved.</string> Fix. http://codereview.chromium.org/10171020/diff/1/remoting/host/me2me_preference... File remoting/host/me2me_preference_pane.mm (right): http://codereview.chromium.org/10171020/diff/1/remoting/host/me2me_preference... remoting/host/me2me_preference_pane.mm:121: [center addObserver:self You have to release this observer. http://codereview.chromium.org/10171020/diff/1/remoting/host/me2me_preference... remoting/host/me2me_preference_pane.mm:341: if (fclose(pipe) != 0) { Shouldn't you close the pipe even if input_data.empty()?
Installer LGTM http://codereview.chromium.org/10171020/diff/1/remoting/host/me2me_preference... File remoting/host/me2me_preference_pane-Info.plist (right): http://codereview.chromium.org/10171020/diff/1/remoting/host/me2me_preference... remoting/host/me2me_preference_pane-Info.plist:26: <string>Copyright © 2012 COPYRIGHT_BY All rights reserved.</string> On 2012/04/27 17:06:00, dcaiafa wrote: > Fix. This is correct. COPYRIGHT_BY is replaced with Chromium or Google depending on the branding. http://codereview.chromium.org/10171020/diff/1/remoting/host/me2me_preference... File remoting/host/me2me_preference_pane.mm (right): http://codereview.chromium.org/10171020/diff/1/remoting/host/me2me_preference... remoting/host/me2me_preference_pane.mm:38: const char kHelperTool[] = kConfigDir kServiceName ".me2me.sh"; At some point, it would be nice to have all these filenames/paths defined in one place for all of the host tools to use.
I think it would be worth getting someone more familiar with Mac code to review this. Maybe dmaclach@ or aharper@ might be willing to take a look? http://codereview.chromium.org/10171020/diff/1/remoting/host/installer/mac/Ch... File remoting/host/installer/mac/ChromotingHostService.packproj (right): http://codereview.chromium.org/10171020/diff/1/remoting/host/installer/mac/Ch... remoting/host/installer/mac/ChromotingHostService.packproj:237: </array> For auto-generated content like this, it's often helpful to attach an in-line comment to the CL explaining what it does. http://codereview.chromium.org/10171020/diff/1/remoting/host/me2me_preference... File remoting/host/me2me_preference_pane-Info.plist (right): http://codereview.chromium.org/10171020/diff/1/remoting/host/me2me_preference... remoting/host/me2me_preference_pane-Info.plist:26: <string>Copyright © 2012 COPYRIGHT_BY All rights reserved.</string> Does the copyright string get substituted at some point? http://codereview.chromium.org/10171020/diff/1/remoting/host/me2me_preference... File remoting/host/me2me_preference_pane.mm (right): http://codereview.chromium.org/10171020/diff/1/remoting/host/me2me_preference... remoting/host/me2me_preference_pane.mm:45: } I don't think this is a problem, because we present the contents to the user to confirm before writing the config, but make sure you've considered the implications of using a predictable file-name for this. http://codereview.chromium.org/10171020/diff/1/remoting/host/me2me_preference... remoting/host/me2me_preference_pane.mm:69: IBOutlet SFAuthorizationView* authorization_view_; This is probably my ObjC ignorance, but why no @synthesize for these? http://codereview.chromium.org/10171020/diff/1/remoting/host/me2me_preference... remoting/host/me2me_preference_pane.mm:91: - (void)onNewConfigFile:(NSNotification *)notification; Nit: No space before *, here and elsewhere. http://codereview.chromium.org/10171020/diff/1/remoting/host/me2me_preference... remoting/host/me2me_preference_pane.mm:92: - (void)onTimer:(NSTimer*)timer; onTimer is a rather generic name. Can you name it after what it does, rather than how it's triggered? http://codereview.chromium.org/10171020/diff/1/remoting/host/me2me_preference... remoting/host/me2me_preference_pane.mm:105: InputData:(const std::string&)input_data; Nit: s/InputData/inputData/ http://codereview.chromium.org/10171020/diff/1/remoting/host/me2me_preference... remoting/host/me2me_preference_pane.mm:133: [self updateConfigStatus]; Why do this here? Don't you only expect a new config when the notification is received, or is this to cope with the not-already-running case? http://codereview.chromium.org/10171020/diff/1/remoting/host/me2me_preference... remoting/host/me2me_preference_pane.mm:189: LOG(ERROR) << "Failed to send message to launchd"; Is logging an error sufficient here (and similarly elsewhere)? Shouldn't we notify the user? http://codereview.chromium.org/10171020/diff/1/remoting/host/me2me_preference... remoting/host/me2me_preference_pane.mm:194: // assume the command was successful. The word "assume" makes me nervous. Why do we need to assume anything if we have a successful return code? What else could go wrong, and is there no way we could detect it and return an error code? http://codereview.chromium.org/10171020/diff/1/remoting/host/me2me_preference... remoting/host/me2me_preference_pane.mm:237: NSLog(@"Failed to get path of configuration data."); You're mixing LOG with NSLog; please stick to one or the other. http://codereview.chromium.org/10171020/diff/1/remoting/host/me2me_preference... remoting/host/me2me_preference_pane.mm:241: return; No log here? If this is called even when there's no reason to believe there's a new config, then that's probably correct; otherwise we should log an error. http://codereview.chromium.org/10171020/diff/1/remoting/host/me2me_preference... remoting/host/me2me_preference_pane.mm:249: have_new_config_ = NO; This should be set at the top of the function, since this isn't the only path that results in not having a new file. http://codereview.chromium.org/10171020/diff/1/remoting/host/me2me_preference... remoting/host/me2me_preference_pane.mm:278: const char* command = is_service_running_ ? "--save-config" : "--enable"; It doesn't need to be part of this CL, but I think it would be better if we always use --enable and make the script do the right thing if the service is already running. That avoids the race condition. http://codereview.chromium.org/10171020/diff/1/remoting/host/me2me_preference... remoting/host/me2me_preference_pane.mm:284: have_new_config_ = NO; Shouldn't we clear this earlier? If we get an error writing the config, we're not going to try again, so we effectively don't have a new config. http://codereview.chromium.org/10171020/diff/1/remoting/host/me2me_preference... remoting/host/me2me_preference_pane.mm:287: // its configuration. This also feels like it would be better done by the script, but perhaps not in this CL. http://codereview.chromium.org/10171020/diff/1/remoting/host/me2me_preference... remoting/host/me2me_preference_pane.mm:299: InputData:(const std::string&)input_data { Nit: s/InputData/inputData/ and align colons. http://codereview.chromium.org/10171020/diff/1/remoting/host/me2me_preference... remoting/host/me2me_preference_pane.mm:323: OSSTATUS_LOG(ERROR, status) << "AuthorizationExecuteWithPrivileges"; Yet another type of log? Any reason this needs to be different? http://codereview.chromium.org/10171020/diff/1/remoting/host/me2me_preference... remoting/host/me2me_preference_pane.mm:338: LOG(ERROR) << "Failed to write data to child process"; Close and return NO? http://codereview.chromium.org/10171020/diff/1/remoting/host/me2me_preference... remoting/host/me2me_preference_pane.mm:341: if (fclose(pipe) != 0) { On 2012/04/27 17:06:00, dcaiafa wrote: > Shouldn't you close the pipe even if input_data.empty()? +1 http://codereview.chromium.org/10171020/diff/1/remoting/host/me2me_preference... remoting/host/me2me_preference_pane.mm:342: PLOG(ERROR) << "fclose"; Return NO? http://codereview.chromium.org/10171020/diff/1/remoting/host/plugin/daemon_co... File remoting/host/plugin/daemon_controller_mac.cc (right): http://codereview.chromium.org/10171020/diff/1/remoting/host/plugin/daemon_co... remoting/host/plugin/daemon_controller_mac.cc:17: #include "base/mac/foundation_util.h" Given your comment below, is this still needed? http://codereview.chromium.org/10171020/diff/1/remoting/host/plugin/daemon_co... remoting/host/plugin/daemon_controller_mac.cc:35: const int NSLibraryDirectory = 5; It might be worth investigating the discrepancy as a follow-up. OS version differences perhaps?
http://codereview.chromium.org/10171020/diff/1/remoting/host/me2me_preference... File remoting/host/me2me_preference_pane.mm (right): http://codereview.chromium.org/10171020/diff/1/remoting/host/me2me_preference... remoting/host/me2me_preference_pane.mm:69: IBOutlet SFAuthorizationView* authorization_view_; Because these are instance variables, not properties. You could create properties (properties are just syntactic sugar for accessors) for these fields, but there is no reason to for these. On 2012/04/27 18:21:04, Jamie wrote: > This is probably my ObjC ignorance, but why no @synthesize for these?
http://codereview.chromium.org/10171020/diff/1/remoting/host/me2me_preference... File remoting/host/me2me_preference_pane-Info.plist (right): http://codereview.chromium.org/10171020/diff/1/remoting/host/me2me_preference... remoting/host/me2me_preference_pane-Info.plist:26: <string>Copyright © 2012 COPYRIGHT_BY All rights reserved.</string> On 2012/04/27 18:21:04, Jamie wrote: > Does the copyright string get substituted at some point? The INFOPLIST_PREPROCESS setting (in the .gyp file) performs the substitution before the plist is added to the bundle.
aharper: PTAL at the .mm file if you have time (otherwise feel free to punt this). I know the UI is horrible, this is just a first cut. We'll get some UX guys to look at this, but for now we're happy to have something functional. Please look for any style nits - I'm new to Mac and ObjC in general. http://codereview.chromium.org/10171020/diff/1/remoting/host/installer/mac/Ch... File remoting/host/installer/mac/ChromotingHostService.packproj (right): http://codereview.chromium.org/10171020/diff/1/remoting/host/installer/mac/Ch... remoting/host/installer/mac/ChromotingHostService.packproj:237: </array> On 2012/04/27 18:21:04, Jamie wrote: > For auto-generated content like this, it's often helpful to attach an in-line > comment to the CL explaining what it does. Fair point :) This just adds the prefPane bundle to the installer. http://codereview.chromium.org/10171020/diff/1/remoting/host/me2me_preference... File remoting/host/me2me_preference_pane-Info.plist (right): http://codereview.chromium.org/10171020/diff/1/remoting/host/me2me_preference... remoting/host/me2me_preference_pane-Info.plist:26: <string>Copyright © 2012 COPYRIGHT_BY All rights reserved.</string> On 2012/04/27 18:21:04, Jamie wrote: > Does the copyright string get substituted at some point? Yes, although that's non-obvious just looking at this change :) http://codereview.chromium.org/10171020/diff/1/remoting/host/me2me_preference... File remoting/host/me2me_preference_pane.mm (right): http://codereview.chromium.org/10171020/diff/1/remoting/host/me2me_preference... remoting/host/me2me_preference_pane.mm:38: const char kHelperTool[] = kConfigDir kServiceName ".me2me.sh"; On 2012/04/27 17:14:09, garykac wrote: > At some point, it would be nice to have all these filenames/paths defined in one > place for all of the host tools to use. Agreed, though I'd prefer to wait until this PrefPane is well-established first :) http://codereview.chromium.org/10171020/diff/1/remoting/host/me2me_preference... remoting/host/me2me_preference_pane.mm:45: } On 2012/04/27 18:21:04, Jamie wrote: > I don't think this is a problem, because we present the contents to the user to > confirm before writing the config, but make sure you've considered the > implications of using a predictable file-name for this. Mac OS temp directories are private per-user, so we should be OK. http://codereview.chromium.org/10171020/diff/1/remoting/host/me2me_preference... remoting/host/me2me_preference_pane.mm:91: - (void)onNewConfigFile:(NSNotification *)notification; On 2012/04/27 18:21:04, Jamie wrote: > Nit: No space before *, here and elsewhere. Done. http://codereview.chromium.org/10171020/diff/1/remoting/host/me2me_preference... remoting/host/me2me_preference_pane.mm:92: - (void)onTimer:(NSTimer*)timer; On 2012/04/27 18:21:04, Jamie wrote: > onTimer is a rather generic name. Can you name it after what it does, rather > than how it's triggered? Done. http://codereview.chromium.org/10171020/diff/1/remoting/host/me2me_preference... remoting/host/me2me_preference_pane.mm:105: InputData:(const std::string&)input_data; On 2012/04/27 18:21:04, Jamie wrote: > Nit: s/InputData/inputData/ Done. http://codereview.chromium.org/10171020/diff/1/remoting/host/me2me_preference... remoting/host/me2me_preference_pane.mm:121: [center addObserver:self On 2012/04/27 17:06:00, dcaiafa wrote: > You have to release this observer. Done. http://codereview.chromium.org/10171020/diff/1/remoting/host/me2me_preference... remoting/host/me2me_preference_pane.mm:133: [self updateConfigStatus]; On 2012/04/27 18:21:04, Jamie wrote: > Why do this here? Don't you only expect a new config when the notification is > received, or is this to cope with the not-already-running case? Yes, it's to cope with the not-already-running case. The NPAPI plugin launches the pref-pane and immediately broadcasts an NSNotification. But the launch API is async - the plugin has no way of knowing when the pref-pane has finished launching. With this scheme, the pref-pane never misses a notification. Either it starts running (in which case, willSelect is triggered), or it is already running (in which case, it will get the broadcast NSNotification from the plugin). http://codereview.chromium.org/10171020/diff/1/remoting/host/me2me_preference... remoting/host/me2me_preference_pane.mm:189: LOG(ERROR) << "Failed to send message to launchd"; On 2012/04/27 18:21:04, Jamie wrote: > Is logging an error sufficient here (and similarly elsewhere)? Shouldn't we > notify the user? I expect this particular error would hardly ever happen, but it's a fair point - maybe we should have some generic way of telling the user something went wrong (without having to localize a zillion different messages)? Added a simple "showError" method that currently pops up an alert box. http://codereview.chromium.org/10171020/diff/1/remoting/host/me2me_preference... remoting/host/me2me_preference_pane.mm:194: // assume the command was successful. On 2012/04/27 18:21:04, Jamie wrote: > The word "assume" makes me nervous. Why do we need to assume anything if we have > a successful return code? What else could go wrong, and is there no way we could > detect it and return an error code? The thing that could go wrong (theoretically) is, getting a response that isn't even an error-code. AFAIK, there is no documented API contract saying: "launchd will always send, on success, a response of type DATA_ERRNO with errno == 0". But I take the point that anything unexpected should be treated as a potential error, so I've updated the logic here. http://codereview.chromium.org/10171020/diff/1/remoting/host/me2me_preference... remoting/host/me2me_preference_pane.mm:237: NSLog(@"Failed to get path of configuration data."); On 2012/04/27 18:21:04, Jamie wrote: > You're mixing LOG with NSLog; please stick to one or the other. Done. http://codereview.chromium.org/10171020/diff/1/remoting/host/me2me_preference... remoting/host/me2me_preference_pane.mm:241: return; On 2012/04/27 18:21:04, Jamie wrote: > No log here? If this is called even when there's no reason to believe there's a > new config, then that's probably correct; otherwise we should log an error. This gets called whenever the pref-pane is opened. This could be from the standard Mac OS "System Preferences" GUI, so there wouldn't be a config file in that case. http://codereview.chromium.org/10171020/diff/1/remoting/host/me2me_preference... remoting/host/me2me_preference_pane.mm:249: have_new_config_ = NO; On 2012/04/27 18:21:04, Jamie wrote: > This should be set at the top of the function, since this isn't the only path > that results in not having a new file. I've documented the intent of this function, clarifying that it's meant to read any new config file, but not to forget any config data it previously read (I've explained the reasoning in my comment). I've also tweaked the implementation so that it never destroys a loaded config (that is: I create a temporary new config object, try to read the file, then "swap" it in only if successful). http://codereview.chromium.org/10171020/diff/1/remoting/host/me2me_preference... remoting/host/me2me_preference_pane.mm:278: const char* command = is_service_running_ ? "--save-config" : "--enable"; On 2012/04/27 18:21:04, Jamie wrote: > It doesn't need to be part of this CL, but I think it would be better if we > always use --enable and make the script do the right thing if the service is > already running. That avoids the race condition. This would be nicer. Unfortunately, I don't think the helper script can easily get the PID of the running launchd agent, because the helper runs as root, and the agent runs as the currently-logged-in user (so the agent PID would not appear when the helper runs "launchctl list" or its C-API-equivalent). I guess I'd have to try it to know if this would work. http://codereview.chromium.org/10171020/diff/1/remoting/host/me2me_preference... remoting/host/me2me_preference_pane.mm:284: have_new_config_ = NO; On 2012/04/27 18:21:04, Jamie wrote: > Shouldn't we clear this earlier? If we get an error writing the config, we're > not going to try again, so we effectively don't have a new config. Well, the user could push the Apply button again :) Are we really sure we don't want the user to be able to retry this operation? http://codereview.chromium.org/10171020/diff/1/remoting/host/me2me_preference... remoting/host/me2me_preference_pane.mm:287: // its configuration. On 2012/04/27 18:21:04, Jamie wrote: > This also feels like it would be better done by the script, but perhaps not in > this CL. Same problem as before (although if we could somehow get the PID, I'm pretty sure we can manage to send a SIGHUP to it :) http://codereview.chromium.org/10171020/diff/1/remoting/host/me2me_preference... remoting/host/me2me_preference_pane.mm:299: InputData:(const std::string&)input_data { On 2012/04/27 18:21:04, Jamie wrote: > Nit: s/InputData/inputData/ and align colons. Done. http://codereview.chromium.org/10171020/diff/1/remoting/host/me2me_preference... remoting/host/me2me_preference_pane.mm:323: OSSTATUS_LOG(ERROR, status) << "AuthorizationExecuteWithPrivileges"; On 2012/04/27 18:21:04, Jamie wrote: > Yet another type of log? Any reason this needs to be different? I've ditched NSLog, so I think we're OK now. http://codereview.chromium.org/10171020/diff/1/remoting/host/me2me_preference... remoting/host/me2me_preference_pane.mm:338: LOG(ERROR) << "Failed to write data to child process"; On 2012/04/27 18:21:04, Jamie wrote: > Close and return NO? Sadly, no, because we still want to call waitpid to avoid a zombie. I've improved this, PTAL. http://codereview.chromium.org/10171020/diff/1/remoting/host/me2me_preference... remoting/host/me2me_preference_pane.mm:341: if (fclose(pipe) != 0) { On 2012/04/27 17:06:00, dcaiafa wrote: > Shouldn't you close the pipe even if input_data.empty()? Ooh, well spotted! That would leak the FILE* if not the file-descriptor as well. Done, and expanded the comment. http://codereview.chromium.org/10171020/diff/1/remoting/host/me2me_preference... remoting/host/me2me_preference_pane.mm:342: PLOG(ERROR) << "fclose"; On 2012/04/27 18:21:04, Jamie wrote: > Return NO? Done. http://codereview.chromium.org/10171020/diff/1/remoting/host/plugin/daemon_co... File remoting/host/plugin/daemon_controller_mac.cc (right): http://codereview.chromium.org/10171020/diff/1/remoting/host/plugin/daemon_co... remoting/host/plugin/daemon_controller_mac.cc:17: #include "base/mac/foundation_util.h" On 2012/04/27 18:21:04, Jamie wrote: > Given your comment below, is this still needed? It's needed for base::mac::GetLocalDirectory(). http://codereview.chromium.org/10171020/diff/1/remoting/host/plugin/daemon_co... remoting/host/plugin/daemon_controller_mac.cc:35: const int NSLibraryDirectory = 5; On 2012/04/27 18:21:04, Jamie wrote: > It might be worth investigating the discrepancy as a follow-up. OS version > differences perhaps? The definition in base/mac/foundation_util.h is int or int64, which mismatches the "enum" definition in both SDK 10.5 and 10.6. Seems to cause a problem only for C/C++, not for ObjC, which is probably why it hasn't been noticed before.
http://codereview.chromium.org/10171020/diff/1/remoting/host/me2me_preference... File remoting/host/me2me_preference_pane.mm (right): http://codereview.chromium.org/10171020/diff/1/remoting/host/me2me_preference... remoting/host/me2me_preference_pane.mm:139: [timer_ release]; nil timer_ http://codereview.chromium.org/10171020/diff/1/remoting/host/me2me_preference... remoting/host/me2me_preference_pane.mm:159: return; Should this be an error alert? http://codereview.chromium.org/10171020/diff/1/remoting/host/me2me_preference... remoting/host/me2me_preference_pane.mm:161: if (!IsPinValid(pin, host_id, host_secret_hash)) { I'm not seeing how this validation ties the email address in the prefpane to the host information in the hash. Is the only link that they came from the same config you read originally? http://codereview.chromium.org/10171020/diff/1/remoting/host/me2me_preference... remoting/host/me2me_preference_pane.mm:165: [alert runModal]; This runs application modal, which is probably inappropirate. Use beginSheetModalForWindow:modalDelegate:didEndSelector:contextInfo: so you are a sheet on system preferences http://codereview.chromium.org/10171020/diff/1/remoting/host/me2me_preference... remoting/host/me2me_preference_pane.mm:181: NSLog(@"Failed to run the helper tool"); Should this be an error alert? http://codereview.chromium.org/10171020/diff/1/remoting/host/me2me_preference... remoting/host/me2me_preference_pane.mm:300: if (!file_util::VerifyPathControlledByAdmin(FilePath(kHelperTool))) { This method name made me nervous, because the task it presents is basically impossible to do properly in general. To confirm I've talked to the Chrome folks, and this method isn’t actually intended for security use. In your usage this is only used for /Library/PrivilegedHelperTools so I'd drop this check.
http://codereview.chromium.org/10171020/diff/1/remoting/host/me2me_preference... File remoting/host/me2me_preference_pane.mm (right): http://codereview.chromium.org/10171020/diff/1/remoting/host/me2me_preference... remoting/host/me2me_preference_pane.mm:139: [timer_ release]; On 2012/04/30 21:23:32, aharper wrote: > nil timer_ Done. http://codereview.chromium.org/10171020/diff/1/remoting/host/me2me_preference... remoting/host/me2me_preference_pane.mm:159: return; On 2012/04/30 21:23:32, aharper wrote: > Should this be an error alert? Done. http://codereview.chromium.org/10171020/diff/1/remoting/host/me2me_preference... remoting/host/me2me_preference_pane.mm:161: if (!IsPinValid(pin, host_id, host_secret_hash)) { On 2012/04/30 21:23:32, aharper wrote: > I'm not seeing how this validation ties the email address in the prefpane to the > host information in the hash. Is the only link that they came from the same > config you read originally? Almost. The host registration was done using the OAuth credentials in the config file, which are tied to the e-mail address shown in the prefpane. http://codereview.chromium.org/10171020/diff/1/remoting/host/me2me_preference... remoting/host/me2me_preference_pane.mm:165: [alert runModal]; On 2012/04/30 21:23:32, aharper wrote: > This runs application modal, which is probably inappropirate. Use > beginSheetModalForWindow:modalDelegate:didEndSelector:contextInfo: so you are a > sheet on system preferences Done. http://codereview.chromium.org/10171020/diff/1/remoting/host/me2me_preference... remoting/host/me2me_preference_pane.mm:181: NSLog(@"Failed to run the helper tool"); On 2012/04/30 21:23:32, aharper wrote: > Should this be an error alert? Done. http://codereview.chromium.org/10171020/diff/1/remoting/host/me2me_preference... remoting/host/me2me_preference_pane.mm:300: if (!file_util::VerifyPathControlledByAdmin(FilePath(kHelperTool))) { On 2012/04/30 21:23:32, aharper wrote: > This method name made me nervous, because the task it presents is basically > impossible to do properly in general. To confirm I've talked to the Chrome > folks, and this method isn’t actually intended for security use. > > In your usage this is only used for /Library/PrivilegedHelperTools so I'd drop > this check. > This check was added in response to a comment made by another reviewer: http://codereview.chromium.org/9837031/diff/16002/remoting/host/plugin/daemon... I'm happy to drop it if you still think it's more harm than good. We aim to replace this with a launchd helper tool as we discussed, so we won't need this check anyway.
> http://codereview.chromium.org/10171020/diff/1/remoting/host/me2me_preference... > remoting/host/me2me_preference_pane.mm:161: if (!IsPinValid(pin, > host_id, host_secret_hash)) { > On 2012/04/30 21:23:32, aharper wrote: >> I'm not seeing how this validation ties the email address in the > prefpane to the >> host information in the hash. Is the only link that they came from the > same >> config you read originally? > Almost. The host registration was done using the OAuth credentials in > the config file, which are tied to the e-mail address shown in the > prefpane. How is the email linked to the arguments to IsPinValid()? Could I write a config with a good email and a malicious host_id and host_secret_hash? You're presenting email in UI as a proxy value for the actual secure values. This CL doesn't make it clear if email is really unique for host_id and host_secret_hash. A comment explaining why that linkage is trustable would probably clarify. > This check was added in response to a comment made by another reviewer: > http://codereview.chromium.org/9837031/diff/16002/remoting/host/plugin/daemon... > > I'm happy to drop it if you still think it's more harm than good. The major concern is that the method doesn't actually do what the original commenter suggests and thus its misleading. First, it’s a naive implementation that wasn't intended for security use. Second, the even the permissions it does check are incorrect for /Library/PrivilegedHelperTools (admin group writable is _wrong_ for PrivilegedHelperTools). > We > aim to replace this with a launchd helper tool as we discussed, so we > won't need this check anyway. This is the correct fix, so I'd drop it, mark the current implementation as wrong (with a comment) and move on to the real fix. Alex
On 2 May 2012 14:06, Alex Harper <aharper@chromium.org> wrote: >> http://codereview.chromium.org/10171020/diff/1/remoting/host/me2me_preference... >> remoting/host/me2me_preference_pane.mm:161: if (!IsPinValid(pin, >> host_id, host_secret_hash)) { >> On 2012/04/30 21:23:32, aharper wrote: >>> I'm not seeing how this validation ties the email address in the >> prefpane to the >>> host information in the hash. Is the only link that they came from the >> same >>> config you read originally? >> Almost. The host registration was done using the OAuth credentials in >> the config file, which are tied to the e-mail address shown in the >> prefpane. > > How is the email linked to the arguments to IsPinValid()? Could I write a config with a good email and a malicious host_id and host_secret_hash? You're presenting email in UI as a proxy value for the actual secure values. This CL doesn't make it clear if email is really unique for host_id and host_secret_hash. A comment explaining why that linkage is trustable would probably clarify. The email address and XMPP credentials passed by the caller must match, otherwise the host won't successfully authenticate to Talk. If the caller supplies a bogus host_secret_hash then the preference panel will fail the PIN verification. If the caller supplies a bogus host-Id and host private-key then the host can be made to register with the cloud under the wrong host-Id, thereby leaking information on whether the host is online or not, and potentially leaking the user's Talk Id. The host would still only accept connections from clients the user is logged-in to, and still require the PIN to authenticate those connections, though. >> This check was added in response to a comment made by another reviewer: >> http://codereview.chromium.org/9837031/diff/16002/remoting/host/plugin/daemon... >> >> I'm happy to drop it if you still think it's more harm than good. > > The major concern is that the method doesn't actually do what the original commenter suggests and thus its misleading. First, it’s a naive implementation that wasn't intended for security use. Second, the even the permissions it does check are incorrect for /Library/PrivilegedHelperTools (admin group writable is _wrong_ for PrivilegedHelperTools). > >> We >> aim to replace this with a launchd helper tool as we discussed, so we >> won't need this check anyway. > > This is the correct fix, so I'd drop it, mark the current implementation as wrong (with a comment) and move on to the real fix. > > Alex > >
http://codereview.chromium.org/10171020/diff/16002/remoting/host/me2me_prefer... File remoting/host/me2me_preference_pane.mm (right): http://codereview.chromium.org/10171020/diff/16002/remoting/host/me2me_prefer... remoting/host/me2me_preference_pane.mm:148: [[NSTimer scheduledTimerWithTimeInterval:2.0 How costly is the refresh operation? 2s is quite a short period of time if it involves disk access, for example. Longer term, could we avoid polling using another distributed notification? http://codereview.chromium.org/10171020/diff/16002/remoting/host/me2me_prefer... remoting/host/me2me_preference_pane.mm:200: [self updateAuthorizationStatus]; Can this fail? If so, shouldn't we return early? http://codereview.chromium.org/10171020/diff/16002/remoting/host/me2me_prefer... remoting/host/me2me_preference_pane.mm:292: [status_message_ setStringValue:@"Me2Me is enabled"]; Is this user-facing? If so then it should say Chrome Remote Desktop. Ideally it should be branded if possible. http://codereview.chromium.org/10171020/diff/16002/remoting/host/me2me_prefer... remoting/host/me2me_preference_pane.mm:301: << remoting::kXmppLoginConfigPath; No user-visible error here? Also, you've already started updating the UI here. It would be better to validate the config first, before updating the UI. You can delete if it it's invalid so that it only gets reported once. http://codereview.chromium.org/10171020/diff/16002/remoting/host/me2me_prefer... remoting/host/me2me_preference_pane.mm:372: return NO; Will this show "An unexpected error occurred" if the user cancels the dialog? That's not great UX, but it doesn't need fixing in this CL. http://codereview.chromium.org/10171020/diff/16002/remoting/host/me2me_prefer... remoting/host/me2me_preference_pane.mm:392: fclose(pipe); Can this be merged with the general error-detection logic below so that there's only one call to fclose? If not, then please be consistent w.r.t. braces on single-line ifs.
My question wasn't clear in the review. wez and I had a phone call, and I believe we both now understand my concern about what the user is validating in the UI not matching what is actually being validated. I'll let him explain it further, since he actually understands what's being passed in the config :) Alex
http://codereview.chromium.org/10171020/diff/1/remoting/host/me2me_preference... File remoting/host/me2me_preference_pane.mm (right): http://codereview.chromium.org/10171020/diff/1/remoting/host/me2me_preference... remoting/host/me2me_preference_pane.mm:161: if (!IsPinValid(pin, host_id, host_secret_hash)) { On 2012/05/02 19:05:28, Lambros wrote: > On 2012/04/30 21:23:32, aharper wrote: > > I'm not seeing how this validation ties the email address in the prefpane to > the > > host information in the hash. Is the only link that they came from the same > > config you read originally? > Almost. The host registration was done using the OAuth credentials in the > config file, which are tied to the e-mail address shown in the prefpane. Alex's point is that we're displaying the email address and prompting for the PIN, while the email address is tied to the OAuth credentials - so there is nothing tying the email address, and the rest of the configuration to the PIN, as such. It's a fair point. We should take a hash of the stringified configuration dictionary, combined with the PIN, and verify that rather than the host secret hash, so that the entire configuration, including the email address, is protected by the PIN.
On 2012/05/03 00:13:43, Wez wrote: > http://codereview.chromium.org/10171020/diff/1/remoting/host/me2me_preference... > File remoting/host/me2me_preference_pane.mm (right): > > http://codereview.chromium.org/10171020/diff/1/remoting/host/me2me_preference... > remoting/host/me2me_preference_pane.mm:161: if (!IsPinValid(pin, host_id, > host_secret_hash)) { > On 2012/05/02 19:05:28, Lambros wrote: > > On 2012/04/30 21:23:32, aharper wrote: > > > I'm not seeing how this validation ties the email address in the prefpane to > > the > > > host information in the hash. Is the only link that they came from the same > > > config you read originally? > > Almost. The host registration was done using the OAuth credentials in the > > config file, which are tied to the e-mail address shown in the prefpane. > > Alex's point is that we're displaying the email address and prompting for the > PIN, while the email address is tied to the OAuth credentials - so there is > nothing tying the email address, and the rest of the configuration to the PIN, > as such. It's a fair point. > > We should take a hash of the stringified configuration dictionary, combined with > the PIN, and verify that rather than the host secret hash, so that the entire > configuration, including the email address, is protected by the PIN. OK, to avoid delaying the improvements this CL makes, let's land it without a full configuration hash, and follow-up with CLs to add the hashing and verification, taking care not to break compatibility while we have version-skew between app and host components.
http://codereview.chromium.org/10171020/diff/1/remoting/host/me2me_preference... File remoting/host/me2me_preference_pane.mm (right): http://codereview.chromium.org/10171020/diff/1/remoting/host/me2me_preference... remoting/host/me2me_preference_pane.mm:300: if (!file_util::VerifyPathControlledByAdmin(FilePath(kHelperTool))) { On 2012/05/02 19:05:28, Lambros wrote: > On 2012/04/30 21:23:32, aharper wrote: > > This method name made me nervous, because the task it presents is basically > > impossible to do properly in general. To confirm I've talked to the Chrome > > folks, and this method isn’t actually intended for security use. > > > > In your usage this is only used for /Library/PrivilegedHelperTools so I'd drop > > this check. > > > This check was added in response to a comment made by another reviewer: > http://codereview.chromium.org/9837031/diff/16002/remoting/host/plugin/daemon... > > I'm happy to drop it if you still think it's more harm than good. We aim to > replace this with a launchd helper tool as we discussed, so we won't need this > check anyway. Dropped, and added TODO comment. http://codereview.chromium.org/10171020/diff/16002/remoting/host/me2me_prefer... File remoting/host/me2me_preference_pane.mm (right): http://codereview.chromium.org/10171020/diff/16002/remoting/host/me2me_prefer... remoting/host/me2me_preference_pane.mm:148: [[NSTimer scheduledTimerWithTimeInterval:2.0 On 2012/05/02 22:30:24, Jamie wrote: > How costly is the refresh operation? 2s is quite a short period of time if it > involves disk access, for example. I don't think there's any disk access to worry about. We talk to launchd over a Unix domain socket, then ask it for the PID corresponding to a named service. That should be a lookup in a short list in memory. And I don't see any extra logging in the system console. > > Longer term, could we avoid polling using another distributed notification? Possibly. We could poll only when we're expecting the state to change shortly (such as when the user hits "Disable" or "Apply"). Also, the service couldn't guarantee to broadcast in the case of unclean shutdown, so we'd have to deal with that anyway. http://codereview.chromium.org/10171020/diff/16002/remoting/host/me2me_prefer... remoting/host/me2me_preference_pane.mm:200: [self updateAuthorizationStatus]; On 2012/05/02 22:30:24, Jamie wrote: > Can this fail? If so, shouldn't we return early? Done. http://codereview.chromium.org/10171020/diff/16002/remoting/host/me2me_prefer... remoting/host/me2me_preference_pane.mm:292: [status_message_ setStringValue:@"Me2Me is enabled"]; On 2012/05/02 22:30:24, Jamie wrote: > Is this user-facing? If so then it should say Chrome Remote Desktop. Ideally it > should be branded if possible. Changed to Chrome Remote Desktop for now. Needs to be localized as well as branded, so maybe this can wait? http://codereview.chromium.org/10171020/diff/16002/remoting/host/me2me_prefer... remoting/host/me2me_preference_pane.mm:301: << remoting::kXmppLoginConfigPath; On 2012/05/02 22:30:24, Jamie wrote: > No user-visible error here? Also, you've already started updating the UI here. > It would be better to validate the config first, before updating the UI. You can > delete if it it's invalid so that it only gets reported once. Done. http://codereview.chromium.org/10171020/diff/16002/remoting/host/me2me_prefer... remoting/host/me2me_preference_pane.mm:372: return NO; On 2012/05/02 22:30:24, Jamie wrote: > Will this show "An unexpected error occurred" if the user cancels the dialog? It shouldn't do. If the user cancels the elevation prompt, then |is_pane_unlocked_| should remain false, and this method shouldn't get called. http://codereview.chromium.org/10171020/diff/16002/remoting/host/me2me_prefer... remoting/host/me2me_preference_pane.mm:392: fclose(pipe); On 2012/05/02 22:30:24, Jamie wrote: > Can this be merged with the general error-detection logic below so that there's > only one call to fclose? I couldn't see an easy way to do it. > > If not, then please be consistent w.r.t. braces on single-line ifs. Done
http://codereview.chromium.org/10171020/diff/16002/remoting/host/me2me_prefer... File remoting/host/me2me_preference_pane.mm (right): http://codereview.chromium.org/10171020/diff/16002/remoting/host/me2me_prefer... remoting/host/me2me_preference_pane.mm:292: [status_message_ setStringValue:@"Me2Me is enabled"]; On 2012/05/03 01:47:04, Lambros wrote: > On 2012/05/02 22:30:24, Jamie wrote: > > Is this user-facing? If so then it should say Chrome Remote Desktop. Ideally > it > > should be branded if possible. > Changed to Chrome Remote Desktop for now. Needs to be localized as well as > branded, so maybe this can wait? It should just be an ifdef. If it's more complex than that then it can wait, but if it's easy it might as well go in now. http://codereview.chromium.org/10171020/diff/16002/remoting/host/me2me_prefer... remoting/host/me2me_preference_pane.mm:301: << remoting::kXmppLoginConfigPath; On 2012/05/03 01:47:04, Lambros wrote: > On 2012/05/02 22:30:24, Jamie wrote: > > No user-visible error here? Also, you've already started updating the UI here. > > It would be better to validate the config first, before updating the UI. You > can > > delete if it it's invalid so that it only gets reported once. > > Done. Perhaps I'm missing something, but I think both of my points are still valid with the new code. http://codereview.chromium.org/10171020/diff/26002/remoting/host/me2me_prefer... File remoting/host/me2me_preference_pane.mm (right): http://codereview.chromium.org/10171020/diff/26002/remoting/host/me2me_prefer... remoting/host/me2me_preference_pane.mm:313: DCHECK(result); DCHECK doesn't feel right here, since you're validating external data. Failure should be reported in some way in Release mode too.
http://codereview.chromium.org/10171020/diff/16002/remoting/host/me2me_prefer... File remoting/host/me2me_preference_pane.mm (right): http://codereview.chromium.org/10171020/diff/16002/remoting/host/me2me_prefer... remoting/host/me2me_preference_pane.mm:292: [status_message_ setStringValue:@"Me2Me is enabled"]; On 2012/05/03 16:32:25, Jamie wrote: > On 2012/05/03 01:47:04, Lambros wrote: > > On 2012/05/02 22:30:24, Jamie wrote: > > > Is this user-facing? If so then it should say Chrome Remote Desktop. Ideally > > it > > > should be branded if possible. > > Changed to Chrome Remote Desktop for now. Needs to be localized as well as > > branded, so maybe this can wait? > > It should just be an ifdef. If it's more complex than that then it can wait, but > if it's easy it might as well go in now. Done. http://codereview.chromium.org/10171020/diff/16002/remoting/host/me2me_prefer... remoting/host/me2me_preference_pane.mm:301: << remoting::kXmppLoginConfigPath; On 2012/05/03 16:32:25, Jamie wrote: > On 2012/05/03 01:47:04, Lambros wrote: > > On 2012/05/02 22:30:24, Jamie wrote: > > > No user-visible error here? Also, you've already started updating the UI > here. > > > It would be better to validate the config first, before updating the UI. You > > can > > > delete if it it's invalid so that it only gets reported once. > > > > Done. > > Perhaps I'm missing something, but I think both of my points are still valid > with the new code. The new code moves the config validation out of updateUI, and into the readNewConfig method. So: The config is thrown away if invalid, and not kept in |config_|. The user is alerted in readNewConfig if an invalid config is read. The config is validated "before updating the UI", as requested. The invalid config is only reported once, and not every time updateUI is called. The updateUI method assumes |config_| is either valid or nil, so the DCHECK is appropriate when pulling out the strings. I think this is better than moving the validation to the top of updateUI - it means the config is either valid or nil, and this invariant holds independently of when updateUI gets called. http://codereview.chromium.org/10171020/diff/26002/remoting/host/me2me_prefer... File remoting/host/me2me_preference_pane.mm (right): http://codereview.chromium.org/10171020/diff/26002/remoting/host/me2me_prefer... remoting/host/me2me_preference_pane.mm:313: DCHECK(result); On 2012/05/03 16:32:26, Jamie wrote: > DCHECK doesn't feel right here, since you're validating external data. Failure > should be reported in some way in Release mode too. Added comment.
lgtm once Alex has signed off on the Mac-specific stuff.
> lgtm once Alex has signed off on the Mac-specific stuff. LGTM assuming future changes already discussed.
dcaiafa: please could you check patch-set 9 looks OK ?
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/lambroslambrou@chromium.org/10171020/4...
Try job failure for 10171020-45009 (retry) (retry) on win_rel for step "browser_tests". It's a second try, previously, step "browser_tests" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
