Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2343073002:
Only attach GAIA ID to Mirror header/cookie for whitelisted domain. (Closed)
DescriptionOnly attach GAIA ID to Mirror header/cookie for whitelisted domain.
The GAIA ID is only necessary on Drive and should only be set there.
As the Mirror cookie doesn't have the granularity to only set it on Drive,
it is set on https://google.com instead.
BUG=647260
Committed: https://crrev.com/09509af98dc69e7c39c8729a5a023966577eaa0f
Cr-Commit-Position: refs/heads/master@{#419487}
Patch Set 1 #
Total comments: 1
Patch Set 2 : Only set GAIA Id in Drive for header #
Total comments: 4
Patch Set 3 : Fix typo in comments #
Total comments: 6
Messages
Total messages: 20 (7 generated)
bzanotti@chromium.org changed reviewers: + rogerta@chromium.org
Please take a look.
Please see below. Not sure if I misunderstood the email discussion about this change. https://codereview.chromium.org/2343073002/diff/1/components/signin/core/brow... File components/signin/core/browser/signin_header_helper.cc (right): https://codereview.chromium.org/2343073002/diff/1/components/signin/core/brow... components/signin/core/browser/signin_header_helper.cc:54: return url == kGoogleDotComURL; From the discussion, the gaia id should be included only for: https://drive.google.com https://docs.google.com https://accounts.google.com I think it might be better to rename this function to: IsUrlEligibleToIncludeGaiaId() since it used only for this purpose.
On 2016/09/16 14:34:25, Roger Tawa wrote: > Please see below. Not sure if I misunderstood the email discussion about this > change. > > https://codereview.chromium.org/2343073002/diff/1/components/signin/core/brow... > File components/signin/core/browser/signin_header_helper.cc (right): > > https://codereview.chromium.org/2343073002/diff/1/components/signin/core/brow... > components/signin/core/browser/signin_header_helper.cc:54: return url == > kGoogleDotComURL; > From the discussion, the gaia id should be included only for: > > https://drive.google.com > https://docs.google.com > https://accounts.google.com > > I think it might be better to rename this function to: > > IsUrlEligibleToIncludeGaiaId() > > since it used only for this purpose. Also, it seems the comment at https://cs.chromium.org/chromium/src/components/signin/core/browser/signin_he... is no longer applicable. Should probably remove/update this comment.
On 2016/09/16 14:35:11, Roger Tawa wrote: > On 2016/09/16 14:34:25, Roger Tawa wrote: > > Please see below. Not sure if I misunderstood the email discussion about this > > change. > > > > > https://codereview.chromium.org/2343073002/diff/1/components/signin/core/brow... > > File components/signin/core/browser/signin_header_helper.cc (right): > > > > > https://codereview.chromium.org/2343073002/diff/1/components/signin/core/brow... > > components/signin/core/browser/signin_header_helper.cc:54: return url == > > kGoogleDotComURL; > > From the discussion, the gaia id should be included only for: > > > > https://drive.google.com > > https://docs.google.com > > https://accounts.google.com > > > > I think it might be better to rename this function to: > > > > IsUrlEligibleToIncludeGaiaId() > > > > since it used only for this purpose. > > Also, it seems the comment at > https://cs.chromium.org/chromium/src/components/signin/core/browser/signin_he... > is no longer applicable. Should probably remove/update this comment. Just noticed your comment in the bug about how cookies are handled for iOS and why you used *.google.com. Can we make this tighter on the other platforms? Is there a reason not to?
On 2016/09/16 14:38:23, Roger Tawa wrote: > On 2016/09/16 14:35:11, Roger Tawa wrote: > > On 2016/09/16 14:34:25, Roger Tawa wrote: > > > Please see below. Not sure if I misunderstood the email discussion about > this > > > change. > > > > > > > > > https://codereview.chromium.org/2343073002/diff/1/components/signin/core/brow... > > > File components/signin/core/browser/signin_header_helper.cc (right): > > > > > > > > > https://codereview.chromium.org/2343073002/diff/1/components/signin/core/brow... > > > components/signin/core/browser/signin_header_helper.cc:54: return url == > > > kGoogleDotComURL; > > > From the discussion, the gaia id should be included only for: > > > > > > https://drive.google.com > > > https://docs.google.com > > > https://accounts.google.com > > > > > > I think it might be better to rename this function to: > > > > > > IsUrlEligibleToIncludeGaiaId() > > > > > > since it used only for this purpose. > > > > Also, it seems the comment at > > > https://cs.chromium.org/chromium/src/components/signin/core/browser/signin_he... > > is no longer applicable. Should probably remove/update this comment. > > Just noticed your comment in the bug about how cookies are handled for iOS and > why you used *.google.com. Can we make this tighter on the other platforms? Is > there a reason not to? First of all, my code is wrong since it won't allow subdomains. Yes, we could make things tighter on other platforms. I don't think there is a valid reason not to (maybe the code will be a little bit messier, but that's not a good reason), and I didn't really thought about this at first. I'll fix this on Monday, thanks for the quick review. :)
Description was changed from ========== Only attach GAIA ID to Mirror header/cookie for whitelisted domain. The GAIA ID is only necessary on https://google.com and should only be set there. BUG=647260 ========== to ========== Only attach GAIA ID to Mirror header/cookie for whitelisted domain. The GAIA ID is only necessary on Drive and should only be set there. As the Mirror cookie doesn't have the granularity to only set it on Drive, it is set on https://google.com instead. BUG=647260 ==========
PTAL
lgtm https://codereview.chromium.org/2343073002/diff/20001/components/signin/core/... File components/signin/core/browser/signin_header_helper.cc (right): https://codereview.chromium.org/2343073002/diff/20001/components/signin/core/... components/signin/core/browser/signin_header_helper.cc:55: // Cookie requests dont' have the granularity to only include the GAIA Id for dont' --> don't https://codereview.chromium.org/2343073002/diff/20001/components/signin/core/... components/signin/core/browser/signin_header_helper.cc:195: signin::IsUrlEligibleForXChromeConnectedHeader(request->url()) && For my own info, why do we need the condition: signin::IsUrlEligibleForXChromeConnectedHeader(request->url()) && Seems like we should always remove the header if |redirect_url| is not eligible. I'll ask Ramin.
https://codereview.chromium.org/2343073002/diff/20001/components/signin/core/... File components/signin/core/browser/signin_header_helper.cc (right): https://codereview.chromium.org/2343073002/diff/20001/components/signin/core/... components/signin/core/browser/signin_header_helper.cc:55: // Cookie requests dont' have the granularity to only include the GAIA Id for On 2016/09/19 13:08:54, Roger Tawa wrote: > dont' --> don't Done. https://codereview.chromium.org/2343073002/diff/20001/components/signin/core/... components/signin/core/browser/signin_header_helper.cc:195: signin::IsUrlEligibleForXChromeConnectedHeader(request->url()) && On 2016/09/19 13:08:54, Roger Tawa wrote: > For my own info, why do we need the condition: > > signin::IsUrlEligibleForXChromeConnectedHeader(request->url()) && > > Seems like we should always remove the header if |redirect_url| is not eligible. > I'll ask Ramin. That's a good question. I don't know, but I'm interested once you have the answer.
The CQ bit was checked by bzanotti@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rogerta@chromium.org Link to the patchset: https://codereview.chromium.org/2343073002/#ps40001 (title: "Fix typo in comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Only attach GAIA ID to Mirror header/cookie for whitelisted domain. The GAIA ID is only necessary on Drive and should only be set there. As the Mirror cookie doesn't have the granularity to only set it on Drive, it is set on https://google.com instead. BUG=647260 ========== to ========== Only attach GAIA ID to Mirror header/cookie for whitelisted domain. The GAIA ID is only necessary on Drive and should only be set there. As the Mirror cookie doesn't have the granularity to only set it on Drive, it is set on https://google.com instead. BUG=647260 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Only attach GAIA ID to Mirror header/cookie for whitelisted domain. The GAIA ID is only necessary on Drive and should only be set there. As the Mirror cookie doesn't have the granularity to only set it on Drive, it is set on https://google.com instead. BUG=647260 ========== to ========== Only attach GAIA ID to Mirror header/cookie for whitelisted domain. The GAIA ID is only necessary on Drive and should only be set there. As the Mirror cookie doesn't have the granularity to only set it on Drive, it is set on https://google.com instead. BUG=647260 Committed: https://crrev.com/09509af98dc69e7c39c8729a5a023966577eaa0f Cr-Commit-Position: refs/heads/master@{#419487} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/09509af98dc69e7c39c8729a5a023966577eaa0f Cr-Commit-Position: refs/heads/master@{#419487}
Message was sent while issue was closed.
battre@chromium.org changed reviewers: + battre@chromium.org
Message was sent while issue was closed.
Hi. Thanks a lot for taking care of this. I have added a couple of comments because the CL does not have any unittests and I want to be sure that it does what you expect. Could you please check that? Thanks, Dominic https://codereview.chromium.org/2343073002/diff/40001/components/signin/core/... File components/signin/core/browser/signin_header_helper.cc (right): https://codereview.chromium.org/2343073002/diff/40001/components/signin/core/... components/signin/core/browser/signin_header_helper.cc:52: return IsDriveOrigin(url); Just to be sure: I think you are passing the url here, not the origin. Are you sure that all of this works as expected? https://codereview.chromium.org/2343073002/diff/40001/components/signin/core/... components/signin/core/browser/signin_header_helper.cc:61: return url == kGoogleDotComURL; I think this would be false for https://www.google.com, right? Is that intended? https://codereview.chromium.org/2343073002/diff/40001/components/signin/core/... components/signin/core/browser/signin_header_helper.cc:124: // Only google.com requires the GAIA ID, don't send it to other domains. I think this is inconsistent with sending it to drive.google.com and docs.google.com as well, right?
Message was sent while issue was closed.
Thanks for the review, I've created http://crrev.com/2353923003 to address the new comments (note: still no unit tests as it seems content_settings::CookieSettings can't be faked). https://codereview.chromium.org/2343073002/diff/40001/components/signin/core/... File components/signin/core/browser/signin_header_helper.cc (right): https://codereview.chromium.org/2343073002/diff/40001/components/signin/core/... components/signin/core/browser/signin_header_helper.cc:52: return IsDriveOrigin(url); On 2016/09/20 09:01:55, battre wrote: > Just to be sure: I think you are passing the url here, not the origin. Are you > sure that all of this works as expected? That's a good point, fixed in the new CL. https://codereview.chromium.org/2343073002/diff/40001/components/signin/core/... components/signin/core/browser/signin_header_helper.cc:61: return url == kGoogleDotComURL; On 2016/09/20 09:01:55, battre wrote: > I think this would be false for https://www.google.com, right? Is that intended? I cheated a little bit here, because I know that the only code using this will actually use "https://google.com". I have fixed it to actually extract the domain and check that it is google.com. https://codereview.chromium.org/2343073002/diff/40001/components/signin/core/... components/signin/core/browser/signin_header_helper.cc:124: // Only google.com requires the GAIA ID, don't send it to other domains. On 2016/09/20 09:01:55, battre wrote: > I think this is inconsistent with sending it to http://drive.google.com and > http://docs.google.com as well, right? Correct, I forgot to update the comment. |
