Add DOMAIN_MISMATCH message end to end

This change is to add DOMAIN_MISMATCH message end to end.

Currently if host client domain policy has been enabled, and client with a
different domain tries to access a host in it2me scenario, invalid access code
will be shown, but indeed the error message should be domain mismatch.

BUG=611544
Committed: https://crrev.com/49b8bb897e86d171f9a5379086d9dec6812549a2
Cr-Commit-Position: refs/heads/master@{#395215}

[Hzj_jie, 2016-05-19 18:02:28]

Patchset #1 (id:1) has been deleted

[Hzj_jie, 2016-05-19 18:02:40]

Patchset #2 (id:40001) has been deleted

[Hzj_jie, 2016-05-19 18:02:43]

Patchset #1 (id:20001) has been deleted

[Hzj_jie, 2016-05-19 19:22:16]

zijiehe@chromium.org changed reviewers:
+ kelvinp@chromium.org

[Hzj_jie, 2016-05-19 19:22:17]

This change is to add DOMAIN_MISMATCH message end to end.

Currently if host client domain policy has been enabled, and client with a
different domain tries to access a host in it2me scenario, invalid access code
will be shown, but indeed the error message should be domain mismatch.

[Hzj_jie, 2016-05-19 19:27:02]

Description was changed from

==========
Fix b611544

BUG=611544
==========

to

==========
This change is to add DOMAIN_MISMATCH message end to end.

Currently if host client domain policy has been enabled, and client with a
different domain tries to access a host in it2me scenario, invalid access code
will be shown, but indeed the error message should be domain mismatch.

BUG=611544
==========

[kelvinp, 2016-05-19 22:33:56]

kelvinp@chromium.org changed reviewers:
+ lambroslambrou@chromium.org

[kelvinp, 2016-05-19 22:39:36]

lgtm

The fact that we need to change 18 files for adding an error code suggests that
there are rooms to simplify our error code conversions in the future.

[Hzj_jie, 2016-05-19 22:57:32]

zijiehe@chromium.org changed reviewers:
+ nicholss@chromium.org

[Hzj_jie, 2016-05-19 23:05:10]

Add Scott for iOS client related change, though it should not be impacted.

[Sergey Ulanov, 2016-05-20 00:07:04]

sergeyu@chromium.org changed reviewers:
+ sergeyu@chromium.org

[Sergey Ulanov, 2016-05-20 00:07:05]

https://codereview.chromium.org/1987163002/diff/80001/remoting/protocol/errors.h
File remoting/protocol/errors.h (right):

https://codereview.chromium.org/1987163002/diff/80001/remoting/protocol/error...
remoting/protocol/errors.h:27: UNKNOWN_ERROR,
please keep UNKNOWN_ERROR the last error in the list.

https://codereview.chromium.org/1987163002/diff/80001/remoting/protocol/error...
remoting/protocol/errors.h:28: DOMAIN_MISMATCH,
I don't think DOMAIN_MISMATCH is the best name here. What if we allow multiple
domains?
maybe something like INVALID_ACCOUNT. That will be usable for any access-control
lists on the host side (not only domain policy).

https://codereview.chromium.org/1987163002/diff/80001/remoting/protocol/jingl...
File remoting/protocol/jingle_messages.cc (right):

https://codereview.chromium.org/1987163002/diff/80001/remoting/protocol/jingl...
remoting/protocol/jingle_messages.cc:47: { JingleMessage::DOMAIN_MISMATCH,
"domain-mismatch" },
The XMPP spec defines list of the reasons, and "domain-mismatch" isn't there.
Technically this error is closed to <security-error/> reason, but I don't think
we want to use it here either.
What I suggest doing instead is adding a separate tag in the <reason>, to
specify an error code from error.h. So generated reason would look as follows:
 <jingle:reason>
   <jingle:decline/>
   <crd:error-code>DOMAIN_MISTMATCH</crd:error-code>
 </jingle:reason>

(notice different namespace for error-code)
Then going forward on the receiver side we may even ignore the offical reason
and just use <error-code> when it's present.
This will scale better for adding new error codes going forward as we won't need
to add new reason for each new error code.

https://codereview.chromium.org/1987163002/diff/80001/remoting/resources/remo...
File remoting/resources/remoting_strings.grd (right):

https://codereview.chromium.org/1987163002/diff/80001/remoting/resources/remo...
remoting/resources/remoting_strings.grd:758: Client domain does not match host
domain restriction.
This message seems to be too specific (it talks about domain) and may be hard to
understand. I don't think we use "domain" or "host" anywhere in UI. Something
like "Security policies on the remote computer do not allow connections for your
account." would be better IMO.

[Sergey Ulanov, 2016-05-20 00:10:35]

On 2016/05/19 22:39:36, kelvinp wrote:
> lgtm
> 
> The fact that we need to change 18 files for adding an error code suggests
that
> there are rooms to simplify our error code conversions in the future.

yep. see my comments in jingle_messages.cc . We will need to update WebRTC
client as well to handle error-code.

[Hzj_jie, 2016-05-20 00:33:56]

On 2016/05/20 00:07:05, Sergey Ulanov wrote:
>
https://codereview.chromium.org/1987163002/diff/80001/remoting/protocol/errors.h
> File remoting/protocol/errors.h (right):
> 
>
https://codereview.chromium.org/1987163002/diff/80001/remoting/protocol/error...
> remoting/protocol/errors.h:27: UNKNOWN_ERROR,
> please keep UNKNOWN_ERROR the last error in the list.
> 
>
https://codereview.chromium.org/1987163002/diff/80001/remoting/protocol/error...
> remoting/protocol/errors.h:28: DOMAIN_MISMATCH,
> I don't think DOMAIN_MISMATCH is the best name here. What if we allow multiple
> domains?
> maybe something like INVALID_ACCOUNT. That will be usable for any
access-control
> lists on the host side (not only domain policy).
> 
>
https://codereview.chromium.org/1987163002/diff/80001/remoting/protocol/jingl...
> File remoting/protocol/jingle_messages.cc (right):
> 
>
https://codereview.chromium.org/1987163002/diff/80001/remoting/protocol/jingl...
> remoting/protocol/jingle_messages.cc:47: { JingleMessage::DOMAIN_MISMATCH,
> "domain-mismatch" },
> The XMPP spec defines list of the reasons, and "domain-mismatch" isn't there.
> Technically this error is closed to <security-error/> reason, but I don't
think
> we want to use it here either.
> What I suggest doing instead is adding a separate tag in the <reason>, to
> specify an error code from error.h. So generated reason would look as follows:
>  <jingle:reason>
>    <jingle:decline/>
>    <crd:error-code>DOMAIN_MISTMATCH</crd:error-code>
>  </jingle:reason>
> 
> (notice different namespace for error-code)
> Then going forward on the receiver side we may even ignore the offical reason
> and just use <error-code> when it's present.
> This will scale better for adding new error codes going forward as we won't
need
> to add new reason for each new error code.
> 
>
https://codereview.chromium.org/1987163002/diff/80001/remoting/resources/remo...
> File remoting/resources/remoting_strings.grd (right):
> 
>
https://codereview.chromium.org/1987163002/diff/80001/remoting/resources/remo...
> remoting/resources/remoting_strings.grd:758: Client domain does not match host
> domain restriction.
> This message seems to be too specific (it talks about domain) and may be hard
to
> understand. I don't think we use "domain" or "host" anywhere in UI. Something
> like "Security policies on the remote computer do not allow connections for
your
> account." would be better IMO.

I just realized jingle is a public message instead of chromoting specific one. I
will continue work on this change.

[Hzj_jie, 2016-05-20 19:04:04]

On 2016/05/20 00:33:56, Hzj_jie wrote:
> On 2016/05/20 00:07:05, Sergey Ulanov wrote:
> >
>
https://codereview.chromium.org/1987163002/diff/80001/remoting/protocol/errors.h
> > File remoting/protocol/errors.h (right):
> > 
> >
>
https://codereview.chromium.org/1987163002/diff/80001/remoting/protocol/error...
> > remoting/protocol/errors.h:27: UNKNOWN_ERROR,
> > please keep UNKNOWN_ERROR the last error in the list.
> > 
> >
>
https://codereview.chromium.org/1987163002/diff/80001/remoting/protocol/error...
> > remoting/protocol/errors.h:28: DOMAIN_MISMATCH,
> > I don't think DOMAIN_MISMATCH is the best name here. What if we allow
multiple
> > domains?
> > maybe something like INVALID_ACCOUNT. That will be usable for any
> access-control
> > lists on the host side (not only domain policy).
> > 
> >
>
https://codereview.chromium.org/1987163002/diff/80001/remoting/protocol/jingl...
> > File remoting/protocol/jingle_messages.cc (right):
> > 
> >
>
https://codereview.chromium.org/1987163002/diff/80001/remoting/protocol/jingl...
> > remoting/protocol/jingle_messages.cc:47: { JingleMessage::DOMAIN_MISMATCH,
> > "domain-mismatch" },
> > The XMPP spec defines list of the reasons, and "domain-mismatch" isn't
there.
> > Technically this error is closed to <security-error/> reason, but I don't
> think
> > we want to use it here either.
> > What I suggest doing instead is adding a separate tag in the <reason>, to
> > specify an error code from error.h. So generated reason would look as
follows:
> >  <jingle:reason>
> >    <jingle:decline/>
> >    <crd:error-code>DOMAIN_MISTMATCH</crd:error-code>
> >  </jingle:reason>
> > 
> > (notice different namespace for error-code)
> > Then going forward on the receiver side we may even ignore the offical
reason
> > and just use <error-code> when it's present.
> > This will scale better for adding new error codes going forward as we won't
> need
> > to add new reason for each new error code.
> > 
> >
>
https://codereview.chromium.org/1987163002/diff/80001/remoting/resources/remo...
> > File remoting/resources/remoting_strings.grd (right):
> > 
> >
>
https://codereview.chromium.org/1987163002/diff/80001/remoting/resources/remo...
> > remoting/resources/remoting_strings.grd:758: Client domain does not match
host
> > domain restriction.
> > This message seems to be too specific (it talks about domain) and may be
hard
> to
> > understand. I don't think we use "domain" or "host" anywhere in UI.
Something
> > like "Security policies on the remote computer do not allow connections for
> your
> > account." would be better IMO.
> 
> I just realized jingle is a public message instead of chromoting specific one.
I
> will continue work on this change.

According to Sergey's comments, I will do following changes,
1. Rename DOMAIN_MISMATCH into INVALID_ACCOUNT
2. Use Jingle security-error instead of domain-mismatch for a short term
solution
3. Update localization string to remove domain / host keywords

[Hzj_jie, 2016-05-20 22:38:57]

Kelvin, would you please LGTM again?

[kelvinp, 2016-05-20 22:44:12]

https://codereview.chromium.org/1987163002/diff/80001/remoting/protocol/jingl...
File remoting/protocol/jingle_messages.cc (right):

https://codereview.chromium.org/1987163002/diff/80001/remoting/protocol/jingl...
remoting/protocol/jingle_messages.cc:47: { JingleMessage::DOMAIN_MISMATCH,
"domain-mismatch" },
On 2016/05/20 00:07:05, Sergey Ulanov wrote:
> The XMPP spec defines list of the reasons, and "domain-mismatch" isn't there.
> Technically this error is closed to <security-error/> reason, but I don't
think
> we want to use it here either.
> What I suggest doing instead is adding a separate tag in the <reason>, to
> specify an error code from error.h. So generated reason would look as follows:
>  <jingle:reason>
>    <jingle:decline/>
>    <crd:error-code>DOMAIN_MISTMATCH</crd:error-code>
>  </jingle:reason>
> 
> (notice different namespace for error-code)
> Then going forward on the receiver side we may even ignore the offical reason
> and just use <error-code> when it's present.
> This will scale better for adding new error codes going forward as we won't
need
> to add new reason for each new error code.

I don't think you have addressed Sergey's comment here.

[kelvinp, 2016-05-20 23:19:17]

On 2016/05/20 22:44:12, kelvinp wrote:
>
https://codereview.chromium.org/1987163002/diff/80001/remoting/protocol/jingl...
> File remoting/protocol/jingle_messages.cc (right):
> 
>
https://codereview.chromium.org/1987163002/diff/80001/remoting/protocol/jingl...
> remoting/protocol/jingle_messages.cc:47: { JingleMessage::DOMAIN_MISMATCH,
> "domain-mismatch" },
> On 2016/05/20 00:07:05, Sergey Ulanov wrote:
> > The XMPP spec defines list of the reasons, and "domain-mismatch" isn't
there.
> > Technically this error is closed to <security-error/> reason, but I don't
> think
> > we want to use it here either.
> > What I suggest doing instead is adding a separate tag in the <reason>, to
> > specify an error code from error.h. So generated reason would look as
follows:
> >  <jingle:reason>
> >    <jingle:decline/>
> >    <crd:error-code>DOMAIN_MISTMATCH</crd:error-code>
> >  </jingle:reason>
> > 
> > (notice different namespace for error-code)
> > Then going forward on the receiver side we may even ignore the offical
reason
> > and just use <error-code> when it's present.
> > This will scale better for adding new error codes going forward as we won't
> need
> > to add new reason for each new error code.
> 
> I don't think you have addressed Sergey's comment here.

As spoken offline, please add a TODO statement with sergey's suggestion.
lgtm

[Hzj_jie, 2016-05-20 23:53:03]

The CQ bit was checked by zijiehe@chromium.org

[Hzj_jie, 2016-05-20 23:53:03]

The patchset sent to the CQ was uploaded after l-g-t-m from kelvinp@chromium.org
Link to the patchset: https://codereview.chromium.org/1987163002/#ps140001
(title: "Add TODO in Jingle session")

[commit-bot: I haz the power, 2016-05-20 23:53:24]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1987163002/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1987163002/140001

[commit-bot: I haz the power, 2016-05-21 00:24:29]

Description was changed from

==========
This change is to add DOMAIN_MISMATCH message end to end.

Currently if host client domain policy has been enabled, and client with a
different domain tries to access a host in it2me scenario, invalid access code
will be shown, but indeed the error message should be domain mismatch.

BUG=611544
==========

to

==========
This change is to add DOMAIN_MISMATCH message end to end.

Currently if host client domain policy has been enabled, and client with a
different domain tries to access a host in it2me scenario, invalid access code
will be shown, but indeed the error message should be domain mismatch.

BUG=611544
==========

[commit-bot: I haz the power, 2016-05-21 00:24:31]

Committed patchset #5 (id:140001)

[commit-bot: I haz the power, 2016-05-21 00:26:41]

Description was changed from

==========
This change is to add DOMAIN_MISMATCH message end to end.

Currently if host client domain policy has been enabled, and client with a
different domain tries to access a host in it2me scenario, invalid access code
will be shown, but indeed the error message should be domain mismatch.

BUG=611544
==========

to

==========
This change is to add DOMAIN_MISMATCH message end to end.

Currently if host client domain policy has been enabled, and client with a
different domain tries to access a host in it2me scenario, invalid access code
will be shown, but indeed the error message should be domain mismatch.

BUG=611544

Committed: https://crrev.com/49b8bb897e86d171f9a5379086d9dec6812549a2
Cr-Commit-Position: refs/heads/master@{#395215}
==========

[commit-bot: I haz the power, 2016-05-21 00:26:43]

Patchset 5 (id:??) landed as
https://crrev.com/49b8bb897e86d171f9a5379086d9dec6812549a2
Cr-Commit-Position: refs/heads/master@{#395215}

[Sergey Ulanov, 2016-05-31 12:56:10]

> According to Sergey's comments, I will do following changes,
> 1. Rename DOMAIN_MISMATCH into INVALID_ACCOUNT
> 2. Use Jingle security-error instead of domain-mismatch for a short term
> solution
> 3. Update localization string to remove domain / host keywords

Sorry if it wasn't clear from my previous comments. I don't think we gain
anything from adding security-error reason here. It's better to just add the
error tag. Since this change didn't make it to the M52 branch I suggest removing
security-error and just use <error-code> instead.

[Hzj_jie, 2016-05-31 17:51:50]

Hi, Sergey,
Yes, Kelvin and me are both clear with your comments. I am now working on
crd:error-code solution. But considering it's not directly related to the
issue itself, i.e. we won't give a meaning message to the end users if
policy does not allow remote connection from a different domain, combining
two changes will eventually make the code review be large and not focus.

On Tue, May 31, 2016 at 5:56 AM, <sergeyu@chromium.org> wrote:

> > According to Sergey's comments, I will do following changes,
> > 1. Rename DOMAIN_MISMATCH into INVALID_ACCOUNT
> > 2. Use Jingle security-error instead of domain-mismatch for a short term
> > solution
> > 3. Update localization string to remove domain / host keywords
>
> Sorry if it wasn't clear from my previous comments. I don't think we gain
> anything from adding security-error reason here. It's better to just add
> the
> error tag. Since this change didn't make it to the M52 branch I suggest
> removing
> security-error and just use <error-code> instead.
>
> https://codereview.chromium.org/1987163002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.