Issue 10823083: [Chromoting] Implement the host domain policy.

Issue 10823083: [Chromoting] Implement the host domain policy. (Closed)

Created:
8 years, 4 months ago by simonmorris

Modified:
8 years, 4 months ago

Reviewers:
Sergey Ulanov, Lambros

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.

More Reviews

Description

[Chromoting] Implement the host domain policy. A follow-up CL will add a more helpful notification of violation of this policy, in the IT2Me case. BUG=132684 Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=149539

Patch Set 1 #

Total comments: 14

Patch Set 2 : Review. #

Patch Set 3 : Fix for virtual Me2Me. #

Patch Set 4 : Sync. #

Created: 8 years, 4 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+95 lines, -24 lines)			Patch
M	remoting/host/constants.h	View	1 2	1 chunk	+6 lines, -1 line	0 comments	Download
M	remoting/host/installer/mac/PrivilegedHelperTools/org.chromium.chromoting.me2me.sh	View	1 2 3	1 chunk	+1 line, -1 line	0 comments	Download
M	remoting/host/plugin/host_script_object.h	View	1	3 chunks	+8 lines, -2 lines	0 comments	Download
M	remoting/host/plugin/host_script_object.cc	View	1	6 chunks	+47 lines, -18 lines	0 comments	Download
M	remoting/host/remoting_me2me_host.cc	View	1 2 3	4 chunks	+29 lines, -2 lines	0 comments	Download
M	remoting/tools/me2me_virtual_host.py	View	1 2 3	1 chunk	+4 lines, -0 lines	0 comments	Download

Messages

Total messages: 8 (0 generated)

Expand Messages | Collapse Messages

Sergey Ulanov

I'm not sure policy support is something we need for It2Me. http://codereview.chromium.org/10823083/diff/1/remoting/host/constants.h File remoting/host/constants.h (right): ...

8 years, 4 months ago (2012-07-31 20:59:53 UTC) #2

simonmorris

I've checked: policy support is needed for IT2Me, though Me2Me is the priority. http://codereview.chromium.org/10823083/diff/1/remoting/host/constants.h File ...

8 years, 4 months ago (2012-07-31 23:16:10 UTC) #3

I've checked: policy support is needed for IT2Me, though Me2Me is the priority.

http://codereview.chromium.org/10823083/diff/1/remoting/host/constants.h
File remoting/host/constants.h (right):

http://codereview.chromium.org/10823083/diff/1/remoting/host/constants.h#newc...
remoting/host/constants.h:27: kMaxPermanentErrorExitCode =
kInvalidHostDomainExitCode
On 2012/07/31 20:59:53, sergeyu wrote:
> here we assume that all the errors in that range are permanent and all others
> are not, but it's hard to see from the list of errors above. Can you please
> separate the list into 2 groups - to make it clear which errors are permanent
> and which are not.

Done.

http://codereview.chromium.org/10823083/diff/1/remoting/host/plugin/host_scri...
File remoting/host/plugin/host_script_object.cc (right):

http://codereview.chromium.org/10823083/diff/1/remoting/host/plugin/host_scri...
remoting/host/plugin/host_script_object.cc:82: bool EndsWith(std::string s,
std::string suffix) {
On 2012/07/31 20:59:53, sergeyu wrote:
> reuse base::EndsWith()?

Done.

http://codereview.chromium.org/10823083/diff/1/remoting/host/plugin/host_scri...
remoting/host/plugin/host_script_object.cc:930: std::string string_value;
On 2012/07/31 20:59:53, sergeyu wrote:
> move this below to where it's used. Also use a better name for it, e.g.
> domain_policy?

Done.

http://codereview.chromium.org/10823083/diff/1/remoting/host/plugin/host_scri...
remoting/host/plugin/host_script_object.cc:935: if
(policies->GetString(policy_hack::PolicyWatcher::kHostDomainPolicyName,
On 2012/07/31 20:59:53, sergeyu wrote:
> We always expect this value to be present - DCHECK if it's not there?
> Same with kNatPolicyName

PolicyWatcher only emits changed policy values, so that its clients don't have
to compare new values to old values. So this value is only guaranteed to exist
the first time OnPolicyUpdate is called.

http://codereview.chromium.org/10823083/diff/1/remoting/host/plugin/host_scri...
remoting/host/plugin/host_script_object.cc:959: policy_received_ = true;
On 2012/07/31 20:59:53, sergeyu wrote:
> This logically belongs to OnPolicyUpdate(). Then you need to acquire
> nat_policy_lock_ only when setting policy_receveid_. 
> 

I've moved that line to OnPolicyUpdate(). But I think I still need the lock
here, because the nat_traversal_enabled_ may be read on a different thread.

http://codereview.chromium.org/10823083/diff/1/remoting/host/plugin/host_scri...
remoting/host/plugin/host_script_object.cc:973: if (!host_domain.empty() &&
state_ != kStarting) {
On 2012/07/31 20:59:53, sergeyu wrote:
> hm. This will disconnect all connection even when domain hasn't changed. add
> host_domain_ != host_domain condition?

If the domain hasn't changed, the PolicyWatcher won't pass it to
OnPolicyUpdate().

http://codereview.chromium.org/10823083/diff/1/remoting/host/plugin/host_scri...
File remoting/host/plugin/host_script_object.h (right):

http://codereview.chromium.org/10823083/diff/1/remoting/host/plugin/host_scri...
remoting/host/plugin/host_script_object.h:336: std::string host_domain_;
On 2012/07/31 20:59:53, sergeyu wrote:
> call it required_host_domain_ to make it less confusing?

Done.

simonmorris

Lambros, could you just check the changes I've made for virtual Me2Me, in patch set ...

8 years, 4 months ago (2012-08-01 20:32:29 UTC) #5

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/simonmorris@chromium.org/10823083/2010

8 years, 4 months ago (2012-08-01 22:49:12 UTC) #7

Change committed as 149539

Expand Messages | Collapse Messages