CrossOriginAccessControl: separate access checks and error message generation

CrossOriginAccessControl: separate access checks and error message generation

The checking and construction of legible error messages can with benefit
be separated, having the security access checks return an error status
which then can be used to generate a suitable error message.

Along with separating the two, switch to using StringBuilders.

R=
BUG=

Review-Url: https://codereview.chromium.org/2616323002
Cr-Commit-Position: refs/heads/master@{#442236}
Committed: https://chromium.googlesource.com/chromium/src/+/302263bedf6549ed0225fa3f8e32895faf815a81

[sof, 2017-01-08 12:21:09]

The CQ bit was checked by sigbjornf@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2017-01-08 12:21:14]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[sof, 2017-01-08 13:53:27]

The CQ bit was checked by sigbjornf@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2017-01-08 13:53:34]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-08 15:03:31]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-08 15:03:31]

Dry run: This issue passed the CQ dry run.

[sof, 2017-01-08 15:22:50]

sigbjornf@opera.com changed reviewers:
+ mkwst@chromium.org, tyoshino@chromium.org

[sof, 2017-01-08 15:22:51]

please take a look.

[Mike West, 2017-01-09 09:02:22]

LGTM % some nits.

Thanks for this refactoring! It's much nicer this way!

https://codereview.chromium.org/2616323002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/fetch/CrossOriginAccessControl.cpp (right):

https://codereview.chromium.org/2616323002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fetch/CrossOriginAccessControl.cpp:373:
builder.append("'https://mikewest.github.io/cors-rfc1918/').");
Would you mind changing `mikewest` to `wicg`? I apparently forgot to update this
when we started incubation...

https://codereview.chromium.org/2616323002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fetch/CrossOriginAccessControl.cpp:386: " defined
in 'https://mikewest.github.io/cors-rfc1918/').");
Nit: Here too.

https://codereview.chromium.org/2616323002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fetch/CrossOriginAccessControl.cpp:459: "'
contains userinfo, which is disallowed for cross-origin "
Nit: If you don't mind changing the tests as well, "userinfo" is fairly obtuse.
This would probably be better spelled "a username and password".

https://codereview.chromium.org/2616323002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/fetch/CrossOriginAccessControl.h (right):

https://codereview.chromium.org/2616323002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fetch/CrossOriginAccessControl.h:76: //
Experimental Access-Control-Allow-External error conditions:
Nit: Can you add a link to https://wicg.github.io/cors-rfc1918/#headers?

https://codereview.chromium.org/2616323002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fetch/CrossOriginAccessControl.h:111: //
according to CORS. That is,
Nit: s/,/:/?

https://codereview.chromium.org/2616323002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fetch/CrossOriginAccessControl.h:129: 
Nit: Remove the newline?

[sof, 2017-01-09 09:31:17]

The CQ bit was checked by sigbjornf@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2017-01-09 09:31:38]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[sof, 2017-01-09 09:36:49]

Thanks for the review; hopefully this "builder" based construction won't be too
verbose to work with.

https://codereview.chromium.org/2616323002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/fetch/CrossOriginAccessControl.cpp (right):

https://codereview.chromium.org/2616323002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fetch/CrossOriginAccessControl.cpp:373:
builder.append("'https://mikewest.github.io/cors-rfc1918/').");
On 2017/01/09 09:02:22, Mike West (sloooooow) wrote:
> Would you mind changing `mikewest` to `wicg`? I apparently forgot to update
this
> when we started incubation...

Updated.

https://codereview.chromium.org/2616323002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fetch/CrossOriginAccessControl.cpp:386: " defined
in 'https://mikewest.github.io/cors-rfc1918/').");
On 2017/01/09 09:02:22, Mike West (sloooooow) wrote:
> Nit: Here too.

Done.

https://codereview.chromium.org/2616323002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fetch/CrossOriginAccessControl.cpp:459: "'
contains userinfo, which is disallowed for cross-origin "
On 2017/01/09 09:02:22, Mike West (sloooooow) wrote:
> Nit: If you don't mind changing the tests as well, "userinfo" is fairly
obtuse.
> This would probably be better spelled "a username and password".

Right, no need to save a few bytes involving that term. Will sync expectations
based on what trybots tell me.

https://codereview.chromium.org/2616323002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/fetch/CrossOriginAccessControl.h (right):

https://codereview.chromium.org/2616323002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fetch/CrossOriginAccessControl.h:76: //
Experimental Access-Control-Allow-External error conditions:
On 2017/01/09 09:02:22, Mike West (sloooooow) wrote:
> Nit: Can you add a link to https://wicg.github.io/cors-rfc1918/#headers?

Makes sense, added.

https://codereview.chromium.org/2616323002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fetch/CrossOriginAccessControl.h:111: //
according to CORS. That is,
On 2017/01/09 09:02:22, Mike West (sloooooow) wrote:
> Nit: s/,/:/?

Done.

https://codereview.chromium.org/2616323002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/fetch/CrossOriginAccessControl.h:129: 
On 2017/01/09 09:02:22, Mike West (sloooooow) wrote:
> Nit: Remove the newline?

Done.

[sof, 2017-01-09 10:48:22]

The CQ bit was checked by sigbjornf@opera.com

[sof, 2017-01-09 10:48:23]

The patchset sent to the CQ was uploaded after l-g-t-m from mkwst@chromium.org
Link to the patchset: https://codereview.chromium.org/2616323002/#ps60001
(title: "sync expectation")

[commit-bot: I haz the power, 2017-01-09 10:48:33]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-09 12:14:41]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1483958902880470,
"parent_rev": "f5f1407adee2b18b59b33fb8d74739dcc27d8c0b", "commit_rev":
"302263bedf6549ed0225fa3f8e32895faf815a81"}

[commit-bot: I haz the power, 2017-01-09 12:15:06]

Description was changed from

==========
CrossOriginAccessControl: separate access checks and error message generation

The checking and construction of legible error messages can with benefit
be separated, having the security access checks return an error status
which then can be used to generate a suitable error message.

Along with separating the two, switch to using StringBuilders.

R=
BUG=
==========

to

==========
CrossOriginAccessControl: separate access checks and error message generation

The checking and construction of legible error messages can with benefit
be separated, having the security access checks return an error status
which then can be used to generate a suitable error message.

Along with separating the two, switch to using StringBuilders.

R=
BUG=

Review-Url: https://codereview.chromium.org/2616323002
Cr-Commit-Position: refs/heads/master@{#442236}
Committed:
https://chromium.googlesource.com/chromium/src/+/302263bedf6549ed0225fa3f8e32...
==========

[commit-bot: I haz the power, 2017-01-09 12:15:07]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/302263bedf6549ed0225fa3f8e32...