Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2888053003:
Network traffic annotation added to OAuth2ApiCallFlow and its subclasses. (Closed)
|
Created:
3 years, 7 months ago by Ramin Halavati Modified:
3 years, 5 months ago Reviewers:
msramek, Michael Courage, Kyle Horimoto, Ramin H., Roger Tawa OOO till Jul 10th, sacomoto CC:
chromium-reviews, extensions-reviews_chromium.org, jlklein+watch-tether_chromium.org, tengs+watch-tether_chromium.org, hansberry+watch-tether_chromium.org, jhawkins+watch-tether_chromium.org, chromium-apps-reviews_chromium.org, lesliewatkins+watch-tether_chromium.org, khorimoto+watch-tether_chromium.org, msramek, battre, Kyle Horimoto, Tim Song Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionNetwork traffic annotation added to OAuth2ApiCallFlow and its subclasses.
Network traffic annotation is added to network requests of:
chrome/browser/extensions/api/identity/identity_get_auth_token_function.cc
components/cryptauth/cryptauth_api_call_flow.cc
components/signin/core/browser/refresh_token_annotation_request.cc
google_apis/gaia/oauth2_api_call_flow.h
google_apis/gaia/oauth2_api_call_flow.cc
BUG=656607
Review-Url: https://codereview.chromium.org/2888053003
Cr-Commit-Position: refs/heads/master@{#484474}
Committed: https://chromium.googlesource.com/chromium/src/+/13592b1a361d8faad62b9789fed6725afa91f008
Patch Set 1 #
Total comments: 20
Patch Set 2 : Annotation moved to virtual function. #Patch Set 3 : Merged. #Patch Set 4 : cryptauth_client_impl_unittest updated. #Patch Set 5 : cryptauth_api_call_flow_unittest updated. #Patch Set 6 : Merge branch 'updating_nta' into an_oauth2_api_call_flow #
Total comments: 6
Patch Set 7 : Comments addressed. #
Total comments: 73
Patch Set 8 : Annotations updated. #
Total comments: 36
Patch Set 9 : Annotation updated. #Patch Set 10 : Annotation updated. #Patch Set 11 : User authentication key added to data fields. #
Total comments: 17
Patch Set 12 : nits #Patch Set 13 : Annotations updated. #Messages
Total messages: 75 (37 generated)
rhalavati@chromium.org changed reviewers: + courage@chromium.org, rogerta@chromium.org, tengs@chromium.org
tengs@: Please review cryptauth_api_call_flow.cc. courage@: Please review identity_get_auth_token_function.cc rogerta@: Please review refresh_token_annotation_request.cc and google_apis/* We are annotating all network requests in Chromium with a new NetworkTrafficAnnotation scheme. This allows enterprise users of Chrome to audit the requests and configure Chrome in a way that meets their security policies and expectations. Furthermore, it allows us to generate better debugging data in chrome://net-internals and measure bandwidth consumption on a per-request-type basis. I've modified 3 files and added annotation template to them. Please review them and suggest the required answers for the empty parts (marked with '...'). Please note that the claims should be thorough and covering all cases. In case you believe that annotation should be passed to the modified function instead of originating from it, please tell me to change the CL accordingly. Please take a look at the protobuf scheme in: https://cs.chromium.org/chromium/src/tools/traffic_annotation/traffic_annotat... for the definition of the annotations. You can find a sample annotation in: https://cs.chromium.org/chromium/src/components/spellcheck/browser/spelling_s... Entriprise policy options are here: https://cs.chromium.org/chromium/src/out/Debug/gen/components/policy/proto/ch... And more description on enterprise policy settings is here: http://dev.chromium.org/administrators/policy-list-3 Please tell me if you need any clarifications from my side. If you want to learn more, see: https://docs.google.com/document/d/1FWEFTHzdqIA1wHUo_DuSpJjppW22otCMPCOA5NumhJU.
https://codereview.chromium.org/2888053003/diff/1/components/cryptauth/crypta... File components/cryptauth/cryptauth_api_call_flow.cc (right): https://codereview.chromium.org/2888053003/diff/1/components/cryptauth/crypta... components/cryptauth/cryptauth_api_call_flow.cc:42: description: "..." This class is a generic wrapper for all API calls we make to the CryptAuth service, so it encompasses a number of use cases. We would need to annotate each instantiation of this class (or provide some enums) in order to properly describe what each call is doing.
https://codereview.chromium.org/2888053003/diff/1/google_apis/gaia/oauth2_api... File google_apis/gaia/oauth2_api_call_flow.h (right): https://codereview.chromium.org/2888053003/diff/1/google_apis/gaia/oauth2_api... google_apis/gaia/oauth2_api_call_flow.h:36: const net::NetworkTrafficAnnotationTag& traffic_annotation); Curious why adding an arg to Start() is better than declaring a new pure virtual method like GetNetworkTrafficAnnotation() as we discussed earlier. A pure virtual method fits the pattern of how this class is configured for other parameters like URL and body and such, and probably makes it easier to fix Tim's comment about the various crypt calls.
https://codereview.chromium.org/2888053003/diff/1/chrome/browser/extensions/a... File chrome/browser/extensions/api/identity/identity_get_auth_token_function.cc (right): https://codereview.chromium.org/2888053003/diff/1/chrome/browser/extensions/a... chrome/browser/extensions/api/identity/identity_get_auth_token_function.cc:584: sender: "..." Chrome Identity API maybe? Really the sender is a Chrome App or Extension https://codereview.chromium.org/2888053003/diff/1/chrome/browser/extensions/a... chrome/browser/extensions/api/identity/identity_get_auth_token_function.cc:585: description: "..." requests a token from gaia allowing an app or extension to act as the user when calling other google APIs https://codereview.chromium.org/2888053003/diff/1/chrome/browser/extensions/a... chrome/browser/extensions/api/identity/identity_get_auth_token_function.cc:586: trigger: "..." api call from the app/extension https://codereview.chromium.org/2888053003/diff/1/chrome/browser/extensions/a... chrome/browser/extensions/api/identity/identity_get_auth_token_function.cc:587: data: "..." The data sent is the user's login token, the identity of a chrome app/extension, and a list of oauth scopes requested by the app/extension. https://codereview.chromium.org/2888053003/diff/1/chrome/browser/extensions/a... chrome/browser/extensions/api/identity/identity_get_auth_token_function.cc:588: destination: WEBSITE/GOOGLE_OWNED_SERVICE/OTHER/LOCAL talks to a google owned service (gaia) https://codereview.chromium.org/2888053003/diff/1/chrome/browser/extensions/a... chrome/browser/extensions/api/identity/identity_get_auth_token_function.cc:590: policy { I guess it's hard to split up these annotations given the way they are implemented, but it doesn't make a lot of sense for this piece of code to be declaring things like the cookie store used, since it doesn't control the cookie store used. https://codereview.chromium.org/2888053003/diff/1/chrome/browser/extensions/a... chrome/browser/extensions/api/identity/identity_get_auth_token_function.cc:591: cookies_allowed: false/true Looks like OAuth2ApiCallFlow disables cookies https://codereview.chromium.org/2888053003/diff/1/chrome/browser/extensions/a... chrome/browser/extensions/api/identity/identity_get_auth_token_function.cc:594: chrome_policy { I don't know much about chrome policy. There's a SigninAllowed policy that says it can disable chrome.identity APIs (which would include this one), but for some reason it says it is deprecated.
On 2017/05/17 22:59:16, Michael Courage wrote: > https://codereview.chromium.org/2888053003/diff/1/chrome/browser/extensions/a... > File chrome/browser/extensions/api/identity/identity_get_auth_token_function.cc > (right): > > https://codereview.chromium.org/2888053003/diff/1/chrome/browser/extensions/a... > chrome/browser/extensions/api/identity/identity_get_auth_token_function.cc:584: > sender: "..." > Chrome Identity API maybe? Really the sender is a Chrome App or Extension > > https://codereview.chromium.org/2888053003/diff/1/chrome/browser/extensions/a... > chrome/browser/extensions/api/identity/identity_get_auth_token_function.cc:585: > description: "..." > requests a token from gaia allowing an app or extension to act as the user when > calling other google APIs > > https://codereview.chromium.org/2888053003/diff/1/chrome/browser/extensions/a... > chrome/browser/extensions/api/identity/identity_get_auth_token_function.cc:586: > trigger: "..." > api call from the app/extension > > https://codereview.chromium.org/2888053003/diff/1/chrome/browser/extensions/a... > chrome/browser/extensions/api/identity/identity_get_auth_token_function.cc:587: > data: "..." > The data sent is the user's login token, the identity of a chrome app/extension, > and a list of oauth scopes requested by the app/extension. > > https://codereview.chromium.org/2888053003/diff/1/chrome/browser/extensions/a... > chrome/browser/extensions/api/identity/identity_get_auth_token_function.cc:588: > destination: WEBSITE/GOOGLE_OWNED_SERVICE/OTHER/LOCAL > talks to a google owned service (gaia) > > https://codereview.chromium.org/2888053003/diff/1/chrome/browser/extensions/a... > chrome/browser/extensions/api/identity/identity_get_auth_token_function.cc:590: > policy { > I guess it's hard to split up these annotations given the way they are > implemented, but it doesn't make a lot of sense for this piece of code to be > declaring things like the cookie store used, since it doesn't control the cookie > store used. > > https://codereview.chromium.org/2888053003/diff/1/chrome/browser/extensions/a... > chrome/browser/extensions/api/identity/identity_get_auth_token_function.cc:591: > cookies_allowed: false/true > Looks like OAuth2ApiCallFlow disables cookies > > https://codereview.chromium.org/2888053003/diff/1/chrome/browser/extensions/a... > chrome/browser/extensions/api/identity/identity_get_auth_token_function.cc:594: > chrome_policy { > I don't know much about chrome policy. There's a SigninAllowed policy that says > it can disable chrome.identity APIs (which would include this one), but for some > reason it says it is deprecated. Thank you Michael, I will updated the code after we decide if we need format change. Roger, Tim, I added annotations as input parameter rather than a virtual function due to a previous limitation in our reporting tool that no longer holds. Therefore I can convert it to virtual function, but I think it is more usable in current form for Tim's usecase. Please refer to "MakeApiCall" function's usages in "components/cryptauth/cryptauth_client_impl.cc", it seems to me that we can add annotation as an input parameter to "MakeApiCall" and have the appropriate annotation for each use case, but don't see how we can do it neatly with a virtual function, noting that the virtual function does not have an input enum. Please advise.
On 2017/05/18 07:43:45, Ramin Halavati wrote: > On 2017/05/17 22:59:16, Michael Courage wrote: > > > https://codereview.chromium.org/2888053003/diff/1/chrome/browser/extensions/a... > > File > chrome/browser/extensions/api/identity/identity_get_auth_token_function.cc > > (right): > > > > > https://codereview.chromium.org/2888053003/diff/1/chrome/browser/extensions/a... > > > chrome/browser/extensions/api/identity/identity_get_auth_token_function.cc:584: > > sender: "..." > > Chrome Identity API maybe? Really the sender is a Chrome App or Extension > > > > > https://codereview.chromium.org/2888053003/diff/1/chrome/browser/extensions/a... > > > chrome/browser/extensions/api/identity/identity_get_auth_token_function.cc:585: > > description: "..." > > requests a token from gaia allowing an app or extension to act as the user > when > > calling other google APIs > > > > > https://codereview.chromium.org/2888053003/diff/1/chrome/browser/extensions/a... > > > chrome/browser/extensions/api/identity/identity_get_auth_token_function.cc:586: > > trigger: "..." > > api call from the app/extension > > > > > https://codereview.chromium.org/2888053003/diff/1/chrome/browser/extensions/a... > > > chrome/browser/extensions/api/identity/identity_get_auth_token_function.cc:587: > > data: "..." > > The data sent is the user's login token, the identity of a chrome > app/extension, > > and a list of oauth scopes requested by the app/extension. > > > > > https://codereview.chromium.org/2888053003/diff/1/chrome/browser/extensions/a... > > > chrome/browser/extensions/api/identity/identity_get_auth_token_function.cc:588: > > destination: WEBSITE/GOOGLE_OWNED_SERVICE/OTHER/LOCAL > > talks to a google owned service (gaia) > > > > > https://codereview.chromium.org/2888053003/diff/1/chrome/browser/extensions/a... > > > chrome/browser/extensions/api/identity/identity_get_auth_token_function.cc:590: > > policy { > > I guess it's hard to split up these annotations given the way they are > > implemented, but it doesn't make a lot of sense for this piece of code to be > > declaring things like the cookie store used, since it doesn't control the > cookie > > store used. > > > > > https://codereview.chromium.org/2888053003/diff/1/chrome/browser/extensions/a... > > > chrome/browser/extensions/api/identity/identity_get_auth_token_function.cc:591: > > cookies_allowed: false/true > > Looks like OAuth2ApiCallFlow disables cookies > > > > > https://codereview.chromium.org/2888053003/diff/1/chrome/browser/extensions/a... > > > chrome/browser/extensions/api/identity/identity_get_auth_token_function.cc:594: > > chrome_policy { > > I don't know much about chrome policy. There's a SigninAllowed policy that > says > > it can disable chrome.identity APIs (which would include this one), but for > some > > reason it says it is deprecated. > > Thank you Michael, I will updated the code after we decide if we need format > change. > > Roger, Tim, > I added annotations as input parameter rather than a virtual function due to a > previous limitation in our reporting tool that no longer holds. Therefore I can > convert it to virtual function, but I think it is more usable in current form > for Tim's usecase. > Please refer to "MakeApiCall" function's usages in > "components/cryptauth/cryptauth_client_impl.cc", it seems to me that we can add > annotation as an input parameter to "MakeApiCall" and have the appropriate > annotation for each use case, but don't see how we can do it neatly with a > virtual function, noting that the virtual function does not have an input enum. > > Please advise. Hi Ramin, I'm not so sure it is that much better for Tim's use case. Note that MakeApiCall() does not call Start() itself. It starts a token fetch, and when the token is received a call to api_call_flow_->Start() is made. This means you need a new member to hold the annotation. If you are going to add a new member, may as well as add that member to CryptAuthApiCallFlow and have a setter for this member. Now CryptAuthApiCallFlow get implement the virtual method and return the appropriate value. So I guess I'm saying adding an input parameter to MakeApiCall() makes sense. But it still seems like a good idea to use a pure virtual method in OAuth2ApiCallFlow. What do you think?
On 2017/05/18 14:15:21, Roger Tawa wrote: > On 2017/05/18 07:43:45, Ramin Halavati wrote: > > On 2017/05/17 22:59:16, Michael Courage wrote: > > > > > > https://codereview.chromium.org/2888053003/diff/1/chrome/browser/extensions/a... > > > File > > chrome/browser/extensions/api/identity/identity_get_auth_token_function.cc > > > (right): > > > > > > > > > https://codereview.chromium.org/2888053003/diff/1/chrome/browser/extensions/a... > > > > > > chrome/browser/extensions/api/identity/identity_get_auth_token_function.cc:584: > > > sender: "..." > > > Chrome Identity API maybe? Really the sender is a Chrome App or Extension > > > > > > > > > https://codereview.chromium.org/2888053003/diff/1/chrome/browser/extensions/a... > > > > > > chrome/browser/extensions/api/identity/identity_get_auth_token_function.cc:585: > > > description: "..." > > > requests a token from gaia allowing an app or extension to act as the user > > when > > > calling other google APIs > > > > > > > > > https://codereview.chromium.org/2888053003/diff/1/chrome/browser/extensions/a... > > > > > > chrome/browser/extensions/api/identity/identity_get_auth_token_function.cc:586: > > > trigger: "..." > > > api call from the app/extension > > > > > > > > > https://codereview.chromium.org/2888053003/diff/1/chrome/browser/extensions/a... > > > > > > chrome/browser/extensions/api/identity/identity_get_auth_token_function.cc:587: > > > data: "..." > > > The data sent is the user's login token, the identity of a chrome > > app/extension, > > > and a list of oauth scopes requested by the app/extension. > > > > > > > > > https://codereview.chromium.org/2888053003/diff/1/chrome/browser/extensions/a... > > > > > > chrome/browser/extensions/api/identity/identity_get_auth_token_function.cc:588: > > > destination: WEBSITE/GOOGLE_OWNED_SERVICE/OTHER/LOCAL > > > talks to a google owned service (gaia) > > > > > > > > > https://codereview.chromium.org/2888053003/diff/1/chrome/browser/extensions/a... > > > > > > chrome/browser/extensions/api/identity/identity_get_auth_token_function.cc:590: > > > policy { > > > I guess it's hard to split up these annotations given the way they are > > > implemented, but it doesn't make a lot of sense for this piece of code to be > > > declaring things like the cookie store used, since it doesn't control the > > cookie > > > store used. > > > > > > > > > https://codereview.chromium.org/2888053003/diff/1/chrome/browser/extensions/a... > > > > > > chrome/browser/extensions/api/identity/identity_get_auth_token_function.cc:591: > > > cookies_allowed: false/true > > > Looks like OAuth2ApiCallFlow disables cookies > > > > > > > > > https://codereview.chromium.org/2888053003/diff/1/chrome/browser/extensions/a... > > > > > > chrome/browser/extensions/api/identity/identity_get_auth_token_function.cc:594: > > > chrome_policy { > > > I don't know much about chrome policy. There's a SigninAllowed policy that > > says > > > it can disable chrome.identity APIs (which would include this one), but for > > some > > > reason it says it is deprecated. > > > > Thank you Michael, I will updated the code after we decide if we need format > > change. > > > > Roger, Tim, > > I added annotations as input parameter rather than a virtual function due to a > > previous limitation in our reporting tool that no longer holds. Therefore I > can > > convert it to virtual function, but I think it is more usable in current form > > for Tim's usecase. > > Please refer to "MakeApiCall" function's usages in > > "components/cryptauth/cryptauth_client_impl.cc", it seems to me that we can > add > > annotation as an input parameter to "MakeApiCall" and have the appropriate > > annotation for each use case, but don't see how we can do it neatly with a > > virtual function, noting that the virtual function does not have an input > enum. > > > > Please advise. > > Hi Ramin, > > I'm not so sure it is that much better for Tim's use case. Note that > MakeApiCall() does not call Start() itself. It starts a token fetch, and when > the token is received a call to api_call_flow_->Start() is made. This means you > need a new member to hold the annotation. If you are going to add a new member, > may as well as add that member to CryptAuthApiCallFlow and have a setter for > this member. Now CryptAuthApiCallFlow get implement the virtual method and > return the appropriate value. > > So I guess I'm saying adding an input parameter to MakeApiCall() makes sense. > But it still seems like a good idea to use a pure virtual method in > OAuth2ApiCallFlow. What do you think? Hi Roger, Thank you very much, I insist that I don't have any preference towards either case, I just want to find the best implementation. I think that if we define the virtual function, the 6 different annotations should be defined in CryptAuthApiCallFlow, and MakApiCall functions would need to pass an enum to choose between them (which should be stored for the actual call). But if we go on with passing annotation as an argument to MakeApiCall (and store it for the actual call in OnAccessTokenFetched), we can have the annotations close to where the call can be made (in MakeApiCall functions). We try to keep annotations as close as possible to the actual code that triggers the call, so that if something changes, there would be higher change of noticing the required change in annotation. But I am OK either case.
Annotations moved to a virtual function. I also used newer helper functions for annotations (not landed yet, but will do in a day or two) which allow defining part of annotation in one function and the rest of it in another. Using this, we can have all common parts (like disabling cookies) in oauth2_api_call_flow.cc and the other parts where the calls are made. I transferred Michael's annotation for identity_get_auth_token_function.cc to oauth2_mint_token_flow.cc. Please suggest corrections for the transferred annotation and values for the rest. These are the files needing values: components/cryptauth/cryptauth_client_impl.cc components/signin/core/browser/refresh_token_annotation_request.cc google_apis/gaia/oauth2_api_call_flow.cc google_apis/gaia/oauth2_mint_token_flow.cc https://codereview.chromium.org/2888053003/diff/1/chrome/browser/extensions/a... File chrome/browser/extensions/api/identity/identity_get_auth_token_function.cc (right): https://codereview.chromium.org/2888053003/diff/1/chrome/browser/extensions/a... chrome/browser/extensions/api/identity/identity_get_auth_token_function.cc:584: sender: "..." On 2017/05/17 22:59:16, Michael Courage wrote: > Chrome Identity API maybe? Really the sender is a Chrome App or Extension |sender| should represent this module, which is creating the request. I think "Chrome Identity API" is good. https://codereview.chromium.org/2888053003/diff/1/chrome/browser/extensions/a... chrome/browser/extensions/api/identity/identity_get_auth_token_function.cc:585: description: "..." On 2017/05/17 22:59:16, Michael Courage wrote: > requests a token from gaia allowing an app or extension to act as the user when > calling other google APIs Done. https://codereview.chromium.org/2888053003/diff/1/chrome/browser/extensions/a... chrome/browser/extensions/api/identity/identity_get_auth_token_function.cc:586: trigger: "..." On 2017/05/17 22:59:16, Michael Courage wrote: > api call from the app/extension Done. https://codereview.chromium.org/2888053003/diff/1/chrome/browser/extensions/a... chrome/browser/extensions/api/identity/identity_get_auth_token_function.cc:587: data: "..." On 2017/05/17 22:59:16, Michael Courage wrote: > The data sent is the user's login token, the identity of a chrome app/extension, > and a list of oauth scopes requested by the app/extension. Done. https://codereview.chromium.org/2888053003/diff/1/chrome/browser/extensions/a... chrome/browser/extensions/api/identity/identity_get_auth_token_function.cc:588: destination: WEBSITE/GOOGLE_OWNED_SERVICE/OTHER/LOCAL On 2017/05/17 22:59:16, Michael Courage wrote: > talks to a google owned service (gaia) Done. https://codereview.chromium.org/2888053003/diff/1/chrome/browser/extensions/a... chrome/browser/extensions/api/identity/identity_get_auth_token_function.cc:590: policy { On 2017/05/17 22:59:16, Michael Courage wrote: > I guess it's hard to split up these annotations given the way they are > implemented, but it doesn't make a lot of sense for this piece of code to be > declaring things like the cookie store used, since it doesn't control the cookie > store used. Acknowledged. https://codereview.chromium.org/2888053003/diff/1/chrome/browser/extensions/a... chrome/browser/extensions/api/identity/identity_get_auth_token_function.cc:591: cookies_allowed: false/true On 2017/05/17 22:59:16, Michael Courage wrote: > Looks like OAuth2ApiCallFlow disables cookies Done. https://codereview.chromium.org/2888053003/diff/1/chrome/browser/extensions/a... chrome/browser/extensions/api/identity/identity_get_auth_token_function.cc:594: chrome_policy { On 2017/05/17 22:59:16, Michael Courage wrote: > I don't know much about chrome policy. There's a SigninAllowed policy that says > it can disable chrome.identity APIs (which would include this one), but for some > reason it says it is deprecated. Acknowledged. https://codereview.chromium.org/2888053003/diff/1/components/cryptauth/crypta... File components/cryptauth/cryptauth_api_call_flow.cc (right): https://codereview.chromium.org/2888053003/diff/1/components/cryptauth/crypta... components/cryptauth/cryptauth_api_call_flow.cc:42: description: "..." On 2017/05/17 19:10:37, Tim Song wrote: > This class is a generic wrapper for all API calls we make to the CryptAuth > service, so it encompasses a number of use cases. We would need to annotate each > instantiation of this class (or provide some enums) in order to properly > describe what each call is doing. Done. https://codereview.chromium.org/2888053003/diff/1/google_apis/gaia/oauth2_api... File google_apis/gaia/oauth2_api_call_flow.h (right): https://codereview.chromium.org/2888053003/diff/1/google_apis/gaia/oauth2_api... google_apis/gaia/oauth2_api_call_flow.h:36: const net::NetworkTrafficAnnotationTag& traffic_annotation); On 2017/05/17 19:26:12, Roger Tawa wrote: > Curious why adding an arg to Start() is better than declaring a new pure virtual > method like GetNetworkTrafficAnnotation() as we discussed earlier. A pure > virtual method fits the pattern of how this class is configured for other > parameters like URL and body and such, and probably makes it easier to fix Tim's > comment about the various crypt calls. Done.
The CQ bit was checked by rhalavati@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
The CQ bit was checked by rhalavati@chromium.org to run a CQ dry run
Patchset #2 (id:20001) has been deleted
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
lgtm Thanks Ramin for your patience and addressing my concerns.
On 2017/05/24 18:01:06, Roger Tawa wrote: > lgtm > > Thanks Ramin for your patience and addressing my concerns. Thank you Roger. Now we need the annotation values for these files: components/cryptauth/cryptauth_client_impl.cc components/signin/core/browser/refresh_token_annotation_request.cc google_apis/gaia/oauth2_api_call_flow.cc google_apis/gaia/oauth2_mint_token_flow.cc
The CQ bit was checked by rhalavati@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by rhalavati@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by rhalavati@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Hi Ramin, Filled in what I know. I'm not familiar with crypto code so you'll need to find an owner for that. Thanks. https://codereview.chromium.org/2888053003/diff/120001/components/signin/core... File components/signin/core/browser/refresh_token_annotation_request.cc (right): https://codereview.chromium.org/2888053003/diff/120001/components/signin/core... components/signin/core/browser/refresh_token_annotation_request.cc:197: } semantics { sender: "Account Fetcher Service" description: "Sends request to /IssueToken endpoint with device_id to backfill device info for refresh tokens issued pre-M38." trigger: "When refreshing account information in AccountFetcherService. On chrome startup and at most once per day." data: "Oauth2 access token for user, device_id for chrome profile, chrome's client id, chrome version number" destination: GOOGLE_OWNED_SERVICE } policy { setting: "none" chrome_policy { [POLICY_NAME] { policy_options {mode: MANDATORY/RECOMMENDED/UNSET} [POLICY_NAME]: none } } policy_exception_justification: "Not implemented" https://codereview.chromium.org/2888053003/diff/120001/google_apis/gaia/oauth... File google_apis/gaia/oauth2_api_call_flow.cc (right): https://codereview.chromium.org/2888053003/diff/120001/google_apis/gaia/oauth... google_apis/gaia/oauth2_api_call_flow.cc:97: 0, CreateApiCallUrl(), request_type, this, traffic_annotation); The annotation comes from GetNetworkTrafficAnnotationTag(). Why does it need to be completed here? Assuming the derived class that implements GetNetworkTrafficAnnotationTag() has no policy controls, there are no further policy controls here. https://codereview.chromium.org/2888053003/diff/120001/google_apis/gaia/oauth... File google_apis/gaia/oauth2_mint_token_flow.cc (right): https://codereview.chromium.org/2888053003/diff/120001/google_apis/gaia/oauth... google_apis/gaia/oauth2_mint_token_flow.cc:308: } This is not controllable by policy. Although if you disable chrome sign in by policy, this feature is indirectly disabled.
rhalavati@chromium.org changed reviewers: + khorimoto@chromium.org
Thank you Roger, comments addressed, please verify. Kyle, We are annotating all network requests in Chromium with a new NetworkTrafficAnnotation scheme. This allows enterprise users of Chrome to audit the requests and configure Chrome in a way that meets their security policies and expectations. Furthermore, it allows us to generate better debugging data in chrome://net-internals and measure bandwidth consumption on a per-request-type basis. I've modified cryptauth_client_impl.cc and added several annotation templates to it. Please review them and suggest the required answers for the empty parts (marked with '...'). Please note that the claims should be thorough and covering all cases. In case you believe that annotation should be passed to the modified function instead of originating from it, please tell me to change the CL accordingly. Please take a look at the protobuf scheme in: https://cs.chromium.org/chromium/src/tools/traffic_annotation/traffic_annotat... for the definition of the annotations. You can find a sample annotation in: https://cs.chromium.org/chromium/src/components/spellcheck/browser/spelling_s... Entriprise policy options are here: https://cs.chromium.org/chromium/src/out/Debug/gen/components/policy/proto/ch... And more description on enterprise policy settings is here: http://dev.chromium.org/administrators/policy-list-3 Please tell me if you need any clarifications from my side. If you want to learn more, see: https://docs.google.com/document/d/1FWEFTHzdqIA1wHUo_DuSpJjppW22otCMPCOA5NumhJU. https://codereview.chromium.org/2888053003/diff/120001/components/signin/core... File components/signin/core/browser/refresh_token_annotation_request.cc (right): https://codereview.chromium.org/2888053003/diff/120001/components/signin/core... components/signin/core/browser/refresh_token_annotation_request.cc:197: } On 2017/05/26 13:46:10, Roger Tawa wrote: > semantics { > sender: "Account Fetcher Service" > description: "Sends request to /IssueToken endpoint with device_id to > backfill device info for refresh tokens issued pre-M38." > trigger: "When refreshing account information in AccountFetcherService. > On chrome startup and at most once per day." > data: "Oauth2 access token for user, device_id for chrome profile, > chrome's client id, chrome version number" > destination: GOOGLE_OWNED_SERVICE > } > policy { > setting: "none" > chrome_policy { > [POLICY_NAME] { > policy_options {mode: MANDATORY/RECOMMENDED/UNSET} > [POLICY_NAME]: none > } > } > policy_exception_justification: "Not implemented" Done. https://codereview.chromium.org/2888053003/diff/120001/google_apis/gaia/oauth... File google_apis/gaia/oauth2_api_call_flow.cc (right): https://codereview.chromium.org/2888053003/diff/120001/google_apis/gaia/oauth... google_apis/gaia/oauth2_api_call_flow.cc:97: 0, CreateApiCallUrl(), request_type, this, traffic_annotation); On 2017/05/26 13:46:10, Roger Tawa wrote: > The annotation comes from GetNetworkTrafficAnnotationTag(). Why does it need to > be completed here? Assuming the derived class that implements > GetNetworkTrafficAnnotationTag() has no policy controls, there are no further > policy controls here. At least cookies are set here. Is it OK if I remove the other parts and just leave them for completion here? Our goal is to keep annotations as close as possible to the code, so that they won't be left not-updated if code changes. https://codereview.chromium.org/2888053003/diff/120001/google_apis/gaia/oauth... File google_apis/gaia/oauth2_mint_token_flow.cc (right): https://codereview.chromium.org/2888053003/diff/120001/google_apis/gaia/oauth... google_apis/gaia/oauth2_mint_token_flow.cc:308: } On 2017/05/26 13:46:11, Roger Tawa wrote: > This is not controllable by policy. Although if you disable chrome sign in by > policy, this feature is indirectly disabled. Done, please verify.
lgtm
On 2017/05/29 14:09:18, Roger Tawa wrote: > lgtm Roger, Kyle, Tim is OOO for around 2 weeks, can you suggest another reviewer?
khorimoto@chromium.org changed reviewers: + sacomoto@chromium.org - tengs@chromium.org
sacomoto@, would you mind reviewing this please? Thanks!
https://codereview.chromium.org/2888053003/diff/140001/components/cryptauth/c... File components/cryptauth/cryptauth_client_impl.cc (right): https://codereview.chromium.org/2888053003/diff/140001/components/cryptauth/c... components/cryptauth/cryptauth_client_impl.cc:63: CryptAuthClientImpl::~CryptAuthClientImpl() { For all requests: destination: GOOGLE_OWNED_SERVICE https://codereview.chromium.org/2888053003/diff/140001/components/cryptauth/c... components/cryptauth/cryptauth_client_impl.cc:66: void CryptAuthClientImpl::GetMyDevices( Currently, this request (and the SendDeviceSyncTickle) can be triggered both by EasyUnlock and Magic Tether. In the future, we expect other services to trigger this service. So, I don't exactly what to put in the "trigger", "setting" and "chrome_policy". To give a bit more context. This request list the user's devices registered on CryptAuth. CryptAuth contains, among other informations, the public key associated to each device. Those features (EasyUnlock and Magic Tether) use theses keys to authenticate other devices when performing some local communication (e.g. Bluetooth Low Energy). In the future, we expect other features to use the similar methods (and keys) when performing these local communications. So, this request might be have several different triggers. https://codereview.chromium.org/2888053003/diff/140001/components/cryptauth/c... components/cryptauth/cryptauth_client_impl.cc:71: net::DefinePartialNetworkTrafficAnnotation("...", "oauth2_api_call_flow", cryptauth_get_my_devices https://codereview.chromium.org/2888053003/diff/140001/components/cryptauth/c... components/cryptauth/cryptauth_client_impl.cc:99: net::DefinePartialNetworkTrafficAnnotation("...", "oauth2_api_call_flow", cryptauth_find_eligible_unlock_devices https://codereview.chromium.org/2888053003/diff/140001/components/cryptauth/c... components/cryptauth/cryptauth_client_impl.cc:102: sender: "..." CryptAuth Device Manager. https://codereview.chromium.org/2888053003/diff/140001/components/cryptauth/c... components/cryptauth/cryptauth_client_impl.cc:103: description: "..." Gets the list of mobile devices that can be used by Smart Lock to unlock the current device. https://codereview.chromium.org/2888053003/diff/140001/components/cryptauth/c... components/cryptauth/cryptauth_client_impl.cc:104: trigger: "..." This request is sent when the user starts the Smart Lock setup flow. https://codereview.chromium.org/2888053003/diff/140001/components/cryptauth/c... components/cryptauth/cryptauth_client_impl.cc:105: data: "..." The device's public key. https://codereview.chromium.org/2888053003/diff/140001/components/cryptauth/c... components/cryptauth/cryptauth_client_impl.cc:109: setting: "..." This request will only be send if the user explicitly tries to enable Smart Lock (EasyUnlock), i.e. starts the setup flow. https://codereview.chromium.org/2888053003/diff/140001/components/cryptauth/c... components/cryptauth/cryptauth_client_impl.cc:111: [POLICY_NAME] { EasyUnlockAllowed https://codereview.chromium.org/2888053003/diff/140001/components/cryptauth/c... components/cryptauth/cryptauth_client_impl.cc:112: policy_options {mode: MANDATORY/RECOMMENDED/UNSET} MANDATORY. https://codereview.chromium.org/2888053003/diff/140001/components/cryptauth/c... components/cryptauth/cryptauth_client_impl.cc:113: [POLICY_NAME]: ... //(value to disable it) EasyUnlockAllowed: false https://codereview.chromium.org/2888053003/diff/140001/components/cryptauth/c... components/cryptauth/cryptauth_client_impl.cc:127: net::DefinePartialNetworkTrafficAnnotation("...", "oauth2_api_call_flow", cryptauth_device_sync_tickle https://codereview.chromium.org/2888053003/diff/140001/components/cryptauth/c... components/cryptauth/cryptauth_client_impl.cc:155: net::DefinePartialNetworkTrafficAnnotation("...", "oauth2_api_call_flow", cryptauth_toggle_easyunlock https://codereview.chromium.org/2888053003/diff/140001/components/cryptauth/c... components/cryptauth/cryptauth_client_impl.cc:158: sender: "..." CryptAuth Device Manager https://codereview.chromium.org/2888053003/diff/140001/components/cryptauth/c... components/cryptauth/cryptauth_client_impl.cc:159: description: "..." Enables Smart Lock (EasyUnlock) for the current device. https://codereview.chromium.org/2888053003/diff/140001/components/cryptauth/c... components/cryptauth/cryptauth_client_impl.cc:160: trigger: "..." This request is send after the user goes through the EasyUnlock setup flow. https://codereview.chromium.org/2888053003/diff/140001/components/cryptauth/c... components/cryptauth/cryptauth_client_impl.cc:161: data: "..." The device public key. https://codereview.chromium.org/2888053003/diff/140001/components/cryptauth/c... components/cryptauth/cryptauth_client_impl.cc:165: setting: "..." This will only be send if the user explicitly enables Smart Lock (EasyUnlock), i.e. successfully complete the setup flow. https://codereview.chromium.org/2888053003/diff/140001/components/cryptauth/c... components/cryptauth/cryptauth_client_impl.cc:167: [POLICY_NAME] { EasyUnlockAllowed https://codereview.chromium.org/2888053003/diff/140001/components/cryptauth/c... components/cryptauth/cryptauth_client_impl.cc:168: policy_options {mode: MANDATORY/RECOMMENDED/UNSET} MANDATORY https://codereview.chromium.org/2888053003/diff/140001/components/cryptauth/c... components/cryptauth/cryptauth_client_impl.cc:169: [POLICY_NAME]: ... //(value to disable it) EasyUnlockAllowed: false https://codereview.chromium.org/2888053003/diff/140001/components/cryptauth/c... components/cryptauth/cryptauth_client_impl.cc:183: net::DefinePartialNetworkTrafficAnnotation("...", "oauth2_api_call_flow", cryptauth_enrollment_flow_setup https://codereview.chromium.org/2888053003/diff/140001/components/cryptauth/c... components/cryptauth/cryptauth_client_impl.cc:186: sender: "..." CryptAuth Enrollment Manager https://codereview.chromium.org/2888053003/diff/140001/components/cryptauth/c... components/cryptauth/cryptauth_client_impl.cc:187: description: "..." Starts the CryptAuth registration flow. https://codereview.chromium.org/2888053003/diff/140001/components/cryptauth/c... components/cryptauth/cryptauth_client_impl.cc:188: trigger: "..." Occurs periodically, at least once a month. https://codereview.chromium.org/2888053003/diff/140001/components/cryptauth/c... components/cryptauth/cryptauth_client_impl.cc:189: data: "..." Various device information (public key, bluetooth MAC address, model, OS version, screen size, manufacturer, has screen lock enabled). https://codereview.chromium.org/2888053003/diff/140001/components/cryptauth/c... components/cryptauth/cryptauth_client_impl.cc:193: setting: "..." This feature cannot be disabled by settings. However, this request is made only for signed-in users. https://codereview.chromium.org/2888053003/diff/140001/components/cryptauth/c... components/cryptauth/cryptauth_client_impl.cc:194: chrome_policy { There is no policy covering this request. https://codereview.chromium.org/2888053003/diff/140001/components/cryptauth/c... components/cryptauth/cryptauth_client_impl.cc:211: net::DefinePartialNetworkTrafficAnnotation("...", "oauth2_api_call_flow", cryptauth_enrollment_flow_finish https://codereview.chromium.org/2888053003/diff/140001/components/cryptauth/c... components/cryptauth/cryptauth_client_impl.cc:214: sender: "..." CryptAuth Enrollment Manager https://codereview.chromium.org/2888053003/diff/140001/components/cryptauth/c... components/cryptauth/cryptauth_client_impl.cc:215: description: "..." Finishes the CryptAuth registration flow. https://codereview.chromium.org/2888053003/diff/140001/components/cryptauth/c... components/cryptauth/cryptauth_client_impl.cc:216: trigger: "..." Occurs periodically, at least once a month. https://codereview.chromium.org/2888053003/diff/140001/components/cryptauth/c... components/cryptauth/cryptauth_client_impl.cc:221: setting: "..." This feature cannot be disabled by settings. However, this request is made only for signed-in users. https://codereview.chromium.org/2888053003/diff/140001/components/cryptauth/c... components/cryptauth/cryptauth_client_impl.cc:222: chrome_policy { There is no policy covering this request.
The CQ bit was checked by rhalavati@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thank you Gustavo, all comments addressed, please review. I moved two annotations to caller functions in the following files based on your suggestion. Would you annotate them or I should add another reviewer? components/cryptauth/cryptauth_device_manager.cc components/proximity_auth/webui/reachable_phone_flow.cc https://codereview.chromium.org/2888053003/diff/140001/components/cryptauth/c... File components/cryptauth/cryptauth_client_impl.cc (right): https://codereview.chromium.org/2888053003/diff/140001/components/cryptauth/c... components/cryptauth/cryptauth_client_impl.cc:63: CryptAuthClientImpl::~CryptAuthClientImpl() { On 2017/06/15 06:13:23, sacomoto wrote: > For all requests: > > destination: GOOGLE_OWNED_SERVICE Done. https://codereview.chromium.org/2888053003/diff/140001/components/cryptauth/c... components/cryptauth/cryptauth_client_impl.cc:66: void CryptAuthClientImpl::GetMyDevices( On 2017/06/15 06:13:19, sacomoto wrote: > Currently, this request (and the SendDeviceSyncTickle) can be triggered both by > EasyUnlock and Magic Tether. In the future, we expect other services to trigger > this service. So, I don't exactly what to put in the "trigger", "setting" and > "chrome_policy". > > To give a bit more context. This request list the user's devices registered on > CryptAuth. CryptAuth contains, among other informations, the public key > associated to each device. Those features (EasyUnlock and Magic Tether) use > theses keys to authenticate other devices when performing some local > communication (e.g. Bluetooth Low Energy). > > In the future, we expect other features to use the similar methods (and keys) > when performing these local communications. So, this request might be have > several different triggers. I moved the annotation to the caller function so that it can be given as an argument and have precise values. https://codereview.chromium.org/2888053003/diff/140001/components/cryptauth/c... components/cryptauth/cryptauth_client_impl.cc:71: net::DefinePartialNetworkTrafficAnnotation("...", "oauth2_api_call_flow", On 2017/06/15 06:13:23, sacomoto wrote: > cryptauth_get_my_devices Acknowledged. https://codereview.chromium.org/2888053003/diff/140001/components/cryptauth/c... components/cryptauth/cryptauth_client_impl.cc:99: net::DefinePartialNetworkTrafficAnnotation("...", "oauth2_api_call_flow", On 2017/06/15 06:13:15, sacomoto wrote: > cryptauth_find_eligible_unlock_devices Done. https://codereview.chromium.org/2888053003/diff/140001/components/cryptauth/c... components/cryptauth/cryptauth_client_impl.cc:102: sender: "..." On 2017/06/15 06:13:11, sacomoto wrote: > CryptAuth Device Manager. Done. https://codereview.chromium.org/2888053003/diff/140001/components/cryptauth/c... components/cryptauth/cryptauth_client_impl.cc:103: description: "..." On 2017/06/15 06:13:12, sacomoto wrote: > Gets the list of mobile devices that can be used by Smart Lock to unlock the > current device. Done. https://codereview.chromium.org/2888053003/diff/140001/components/cryptauth/c... components/cryptauth/cryptauth_client_impl.cc:104: trigger: "..." On 2017/06/15 06:13:11, sacomoto wrote: > This request is sent when the user starts the Smart Lock setup flow. Done. https://codereview.chromium.org/2888053003/diff/140001/components/cryptauth/c... components/cryptauth/cryptauth_client_impl.cc:105: data: "..." On 2017/06/15 06:13:11, sacomoto wrote: > The device's public key. Done. https://codereview.chromium.org/2888053003/diff/140001/components/cryptauth/c... components/cryptauth/cryptauth_client_impl.cc:109: setting: "..." On 2017/06/15 06:13:10, sacomoto wrote: > This request will only be send if the user explicitly tries to enable Smart Lock > (EasyUnlock), i.e. starts the setup flow. Done. https://codereview.chromium.org/2888053003/diff/140001/components/cryptauth/c... components/cryptauth/cryptauth_client_impl.cc:111: [POLICY_NAME] { On 2017/06/15 06:13:23, sacomoto wrote: > EasyUnlockAllowed Done. https://codereview.chromium.org/2888053003/diff/140001/components/cryptauth/c... components/cryptauth/cryptauth_client_impl.cc:112: policy_options {mode: MANDATORY/RECOMMENDED/UNSET} On 2017/06/15 06:13:11, sacomoto wrote: > MANDATORY. We are removing this field as it's always MANDATORY. https://codereview.chromium.org/2888053003/diff/140001/components/cryptauth/c... components/cryptauth/cryptauth_client_impl.cc:113: [POLICY_NAME]: ... //(value to disable it) On 2017/06/15 06:13:17, sacomoto wrote: > EasyUnlockAllowed: false Done. https://codereview.chromium.org/2888053003/diff/140001/components/cryptauth/c... components/cryptauth/cryptauth_client_impl.cc:127: net::DefinePartialNetworkTrafficAnnotation("...", "oauth2_api_call_flow", On 2017/06/15 06:13:11, sacomoto wrote: > cryptauth_device_sync_tickle Acknowledged. https://codereview.chromium.org/2888053003/diff/140001/components/cryptauth/c... components/cryptauth/cryptauth_client_impl.cc:155: net::DefinePartialNetworkTrafficAnnotation("...", "oauth2_api_call_flow", On 2017/06/15 06:13:14, sacomoto wrote: > cryptauth_toggle_easyunlock Done. https://codereview.chromium.org/2888053003/diff/140001/components/cryptauth/c... components/cryptauth/cryptauth_client_impl.cc:158: sender: "..." On 2017/06/15 06:13:11, sacomoto wrote: > CryptAuth Device Manager Done. https://codereview.chromium.org/2888053003/diff/140001/components/cryptauth/c... components/cryptauth/cryptauth_client_impl.cc:159: description: "..." On 2017/06/15 06:13:11, sacomoto wrote: > Enables Smart Lock (EasyUnlock) for the current device. Done. https://codereview.chromium.org/2888053003/diff/140001/components/cryptauth/c... components/cryptauth/cryptauth_client_impl.cc:160: trigger: "..." On 2017/06/15 06:13:16, sacomoto wrote: > This request is send after the user goes through the EasyUnlock setup flow. Done. https://codereview.chromium.org/2888053003/diff/140001/components/cryptauth/c... components/cryptauth/cryptauth_client_impl.cc:161: data: "..." On 2017/06/15 06:13:11, sacomoto wrote: > The device public key. Done. https://codereview.chromium.org/2888053003/diff/140001/components/cryptauth/c... components/cryptauth/cryptauth_client_impl.cc:165: setting: "..." On 2017/06/15 06:13:15, sacomoto wrote: > This will only be send if the user explicitly enables Smart Lock (EasyUnlock), > i.e. successfully complete the setup flow. Done. https://codereview.chromium.org/2888053003/diff/140001/components/cryptauth/c... components/cryptauth/cryptauth_client_impl.cc:167: [POLICY_NAME] { On 2017/06/15 06:13:10, sacomoto wrote: > EasyUnlockAllowed Done. https://codereview.chromium.org/2888053003/diff/140001/components/cryptauth/c... components/cryptauth/cryptauth_client_impl.cc:168: policy_options {mode: MANDATORY/RECOMMENDED/UNSET} On 2017/06/15 06:13:21, sacomoto wrote: > MANDATORY Acknowledged. https://codereview.chromium.org/2888053003/diff/140001/components/cryptauth/c... components/cryptauth/cryptauth_client_impl.cc:169: [POLICY_NAME]: ... //(value to disable it) On 2017/06/15 06:13:10, sacomoto wrote: > EasyUnlockAllowed: false Done. https://codereview.chromium.org/2888053003/diff/140001/components/cryptauth/c... components/cryptauth/cryptauth_client_impl.cc:183: net::DefinePartialNetworkTrafficAnnotation("...", "oauth2_api_call_flow", On 2017/06/15 06:13:11, sacomoto wrote: > cryptauth_enrollment_flow_setup Done. https://codereview.chromium.org/2888053003/diff/140001/components/cryptauth/c... components/cryptauth/cryptauth_client_impl.cc:186: sender: "..." On 2017/06/15 06:13:23, sacomoto wrote: > CryptAuth Enrollment Manager Done. https://codereview.chromium.org/2888053003/diff/140001/components/cryptauth/c... components/cryptauth/cryptauth_client_impl.cc:187: description: "..." On 2017/06/15 06:13:20, sacomoto wrote: > Starts the CryptAuth registration flow. Done. https://codereview.chromium.org/2888053003/diff/140001/components/cryptauth/c... components/cryptauth/cryptauth_client_impl.cc:188: trigger: "..." On 2017/06/15 06:13:10, sacomoto wrote: > Occurs periodically, at least once a month. Done. https://codereview.chromium.org/2888053003/diff/140001/components/cryptauth/c... components/cryptauth/cryptauth_client_impl.cc:189: data: "..." On 2017/06/15 06:13:23, sacomoto wrote: > Various device information (public key, bluetooth MAC address, model, OS > version, screen size, manufacturer, has screen lock enabled). Done. https://codereview.chromium.org/2888053003/diff/140001/components/cryptauth/c... components/cryptauth/cryptauth_client_impl.cc:193: setting: "..." On 2017/06/15 06:13:18, sacomoto wrote: > This feature cannot be disabled by settings. However, this request is made only > for signed-in users. Done. https://codereview.chromium.org/2888053003/diff/140001/components/cryptauth/c... components/cryptauth/cryptauth_client_impl.cc:194: chrome_policy { On 2017/06/15 06:13:10, sacomoto wrote: > There is no policy covering this request. How about 'SigninAllowed'? https://codereview.chromium.org/2888053003/diff/140001/components/cryptauth/c... components/cryptauth/cryptauth_client_impl.cc:211: net::DefinePartialNetworkTrafficAnnotation("...", "oauth2_api_call_flow", On 2017/06/15 06:13:23, sacomoto wrote: > cryptauth_enrollment_flow_finish Done. https://codereview.chromium.org/2888053003/diff/140001/components/cryptauth/c... components/cryptauth/cryptauth_client_impl.cc:214: sender: "..." On 2017/06/15 06:13:11, sacomoto wrote: > CryptAuth Enrollment Manager Done. https://codereview.chromium.org/2888053003/diff/140001/components/cryptauth/c... components/cryptauth/cryptauth_client_impl.cc:215: description: "..." On 2017/06/15 06:13:23, sacomoto wrote: > Finishes the CryptAuth registration flow. Done. https://codereview.chromium.org/2888053003/diff/140001/components/cryptauth/c... components/cryptauth/cryptauth_client_impl.cc:216: trigger: "..." On 2017/06/15 06:13:22, sacomoto wrote: > Occurs periodically, at least once a month. Done. https://codereview.chromium.org/2888053003/diff/140001/components/cryptauth/c... components/cryptauth/cryptauth_client_impl.cc:217: data: "..." None? https://codereview.chromium.org/2888053003/diff/140001/components/cryptauth/c... components/cryptauth/cryptauth_client_impl.cc:221: setting: "..." On 2017/06/15 06:13:10, sacomoto wrote: > This feature cannot be disabled by settings. However, this request is made only > for signed-in users. Done. https://codereview.chromium.org/2888053003/diff/140001/components/cryptauth/c... components/cryptauth/cryptauth_client_impl.cc:222: chrome_policy { On 2017/06/15 06:13:23, sacomoto wrote: > There is no policy covering this request. Acknowledged.
https://codereview.chromium.org/2888053003/diff/140001/components/cryptauth/c... File components/cryptauth/cryptauth_client_impl.cc (right): https://codereview.chromium.org/2888053003/diff/140001/components/cryptauth/c... components/cryptauth/cryptauth_client_impl.cc:194: chrome_policy { On 2017/06/16 09:48:17, Ramin Halavati wrote: > On 2017/06/15 06:13:10, sacomoto wrote: > > There is no policy covering this request. > > How about 'SigninAllowed'? Yes, you are right. This policy would disable all API calls in this class. https://codereview.chromium.org/2888053003/diff/160001/components/cryptauth/c... File components/cryptauth/cryptauth_device_manager.cc (right): https://codereview.chromium.org/2888053003/diff/160001/components/cryptauth/c... components/cryptauth/cryptauth_device_manager.cc:510: void CryptAuthDeviceManager::OnSyncRequested( There are essentially two way this method can be called (and then trigger an GetMyDevices API call): periodic or forced. In both cases, the only difference in the annotation should be in the "trigger" value. For the periodic one, we already have all the information to another the request. But this is not true for the forced sync. Any user of this class can call the |ForceSyncNow| method and trigger an API call. For the forced sync case, I think there are two alternative here: 1) Be a little bit more vague, and set the ""trigger" to forced sync whenever |invocation_reason != INVOCATION_REASON_PERIODIC && invocation_reason != INVOCATION_REASON_INITIALIZATION|. 2) Let the clients of this class set the "trigger" value. Concretely, add a |trigger| parameter to the |ForceSyncNow| method. https://codereview.chromium.org/2888053003/diff/160001/components/proximity_a... File components/proximity_auth/webui/reachable_phone_flow.cc (right): https://codereview.chromium.org/2888053003/diff/160001/components/proximity_a... components/proximity_auth/webui/reachable_phone_flow.cc:48: net::DefinePartialNetworkTrafficAnnotation("...", "oauth2_api_call_flow", cryptauth_device_sync_tickle https://codereview.chromium.org/2888053003/diff/160001/components/proximity_a... components/proximity_auth/webui/reachable_phone_flow.cc:51: sender: "..." EasyUnlock Debug UI https://codereview.chromium.org/2888053003/diff/160001/components/proximity_a... components/proximity_auth/webui/reachable_phone_flow.cc:52: description: "..." Triggers a sync on all other device (for the same user) registered on CryptAuth. https://codereview.chromium.org/2888053003/diff/160001/components/proximity_a... components/proximity_auth/webui/reachable_phone_flow.cc:53: trigger: "..." User manually open the EasyUnlock debug UI. https://codereview.chromium.org/2888053003/diff/160001/components/proximity_a... components/proximity_auth/webui/reachable_phone_flow.cc:54: data: "..." The device public key. https://codereview.chromium.org/2888053003/diff/160001/components/proximity_a... components/proximity_auth/webui/reachable_phone_flow.cc:58: setting: "..." This feature cannot be disabled in settings, but this request will only be sent if the user opens the EasyUnlock debug UI. https://codereview.chromium.org/2888053003/diff/160001/components/proximity_a... components/proximity_auth/webui/reachable_phone_flow.cc:60: [POLICY_NAME] { SigninAllowed https://codereview.chromium.org/2888053003/diff/160001/components/proximity_a... components/proximity_auth/webui/reachable_phone_flow.cc:62: [POLICY_NAME]: ... //(value to disable it) SigninAllowed: false
Thank you Gustavo, all comments addressed, please review. https://codereview.chromium.org/2888053003/diff/140001/components/cryptauth/c... File components/cryptauth/cryptauth_client_impl.cc (right): https://codereview.chromium.org/2888053003/diff/140001/components/cryptauth/c... components/cryptauth/cryptauth_client_impl.cc:194: chrome_policy { On 2017/06/19 18:25:38, sacomoto wrote: > On 2017/06/16 09:48:17, Ramin Halavati wrote: > > On 2017/06/15 06:13:10, sacomoto wrote: > > > There is no policy covering this request. > > > > How about 'SigninAllowed'? > > Yes, you are right. This policy would disable all API calls in this class. Acknowledged. https://codereview.chromium.org/2888053003/diff/160001/components/cryptauth/c... File components/cryptauth/cryptauth_device_manager.cc (right): https://codereview.chromium.org/2888053003/diff/160001/components/cryptauth/c... components/cryptauth/cryptauth_device_manager.cc:510: void CryptAuthDeviceManager::OnSyncRequested( On 2017/06/19 18:25:38, sacomoto wrote: > There are essentially two way this method can be called (and then trigger an > GetMyDevices API call): periodic or forced. In both cases, the only difference > in the annotation should be in the "trigger" value. > > For the periodic one, we already have all the information to another the > request. But this is not true for the forced sync. Any user of this class can > call the |ForceSyncNow| method and trigger an API call. > > For the forced sync case, I think there are two alternative here: > 1) Be a little bit more vague, and set the ""trigger" to forced sync whenever > |invocation_reason != INVOCATION_REASON_PERIODIC && invocation_reason != > INVOCATION_REASON_INITIALIZATION|. > 2) Let the clients of this class set the "trigger" value. Concretely, add a > |trigger| parameter to the |ForceSyncNow| method. I think it would be good if we give a general description of both trigger situations here, like 'Periodically every 24h, or by API request'. We don't have the required structure to define more than 2 level partial annotations now, and I don't feel that we need to expand the structures because of this case as the data and policy are very much the same and it is very improbable that one would like to stop this request based on a specific trigger case. But if you believe so, I can start it that way. https://codereview.chromium.org/2888053003/diff/160001/components/proximity_a... File components/proximity_auth/webui/reachable_phone_flow.cc (right): https://codereview.chromium.org/2888053003/diff/160001/components/proximity_a... components/proximity_auth/webui/reachable_phone_flow.cc:48: net::DefinePartialNetworkTrafficAnnotation("...", "oauth2_api_call_flow", On 2017/06/19 18:25:39, sacomoto wrote: > cryptauth_device_sync_tickle Done. https://codereview.chromium.org/2888053003/diff/160001/components/proximity_a... components/proximity_auth/webui/reachable_phone_flow.cc:51: sender: "..." On 2017/06/19 18:25:39, sacomoto wrote: > EasyUnlock Debug UI Done. https://codereview.chromium.org/2888053003/diff/160001/components/proximity_a... components/proximity_auth/webui/reachable_phone_flow.cc:52: description: "..." On 2017/06/19 18:25:39, sacomoto wrote: > Triggers a sync on all other device (for the same user) registered on CryptAuth. Done. https://codereview.chromium.org/2888053003/diff/160001/components/proximity_a... components/proximity_auth/webui/reachable_phone_flow.cc:53: trigger: "..." On 2017/06/19 18:25:39, sacomoto wrote: > User manually open the EasyUnlock debug UI. Done. https://codereview.chromium.org/2888053003/diff/160001/components/proximity_a... components/proximity_auth/webui/reachable_phone_flow.cc:54: data: "..." On 2017/06/19 18:25:39, sacomoto wrote: > The device public key. Done. https://codereview.chromium.org/2888053003/diff/160001/components/proximity_a... components/proximity_auth/webui/reachable_phone_flow.cc:58: setting: "..." On 2017/06/19 18:25:39, sacomoto wrote: > This feature cannot be disabled in settings, but this request will only be sent > if the user opens the EasyUnlock debug UI. Done. https://codereview.chromium.org/2888053003/diff/160001/components/proximity_a... components/proximity_auth/webui/reachable_phone_flow.cc:60: [POLICY_NAME] { On 2017/06/19 18:25:38, sacomoto wrote: > SigninAllowed Done. https://codereview.chromium.org/2888053003/diff/160001/components/proximity_a... components/proximity_auth/webui/reachable_phone_flow.cc:62: [POLICY_NAME]: ... //(value to disable it) On 2017/06/19 18:25:39, sacomoto wrote: > SigninAllowed: false Done.
https://codereview.chromium.org/2888053003/diff/160001/components/cryptauth/c... File components/cryptauth/cryptauth_device_manager.cc (right): https://codereview.chromium.org/2888053003/diff/160001/components/cryptauth/c... components/cryptauth/cryptauth_device_manager.cc:510: void CryptAuthDeviceManager::OnSyncRequested( On 2017/06/20 05:35:34, Ramin Halavati wrote: > On 2017/06/19 18:25:38, sacomoto wrote: > > There are essentially two way this method can be called (and then trigger an > > GetMyDevices API call): periodic or forced. In both cases, the only difference > > in the annotation should be in the "trigger" value. > > > > For the periodic one, we already have all the information to another the > > request. But this is not true for the forced sync. Any user of this class can > > call the |ForceSyncNow| method and trigger an API call. > > > > For the forced sync case, I think there are two alternative here: > > 1) Be a little bit more vague, and set the ""trigger" to forced sync whenever > > |invocation_reason != INVOCATION_REASON_PERIODIC && invocation_reason != > > INVOCATION_REASON_INITIALIZATION|. > > 2) Let the clients of this class set the "trigger" value. Concretely, add a > > |trigger| parameter to the |ForceSyncNow| method. > > I think it would be good if we give a general description of both trigger > situations here, like 'Periodically every 24h, or by API request'. > We don't have the required structure to define more than 2 level partial > annotations now, and I don't feel that we need to expand the structures because > of this case as the data and policy are very much the same and it is very > improbable that one would like to stop this request based on a specific trigger > case. But if you believe so, I can start it that way. I'm fine with the general description. https://codereview.chromium.org/2888053003/diff/160001/components/cryptauth/c... components/cryptauth/cryptauth_device_manager.cc:546: net::DefinePartialNetworkTrafficAnnotation("...", "oauth2_api_call_flow", cryptauth_get_my_devices https://codereview.chromium.org/2888053003/diff/160001/components/cryptauth/c... components/cryptauth/cryptauth_device_manager.cc:549: sender: "..." CryptAuth Device Manager https://codereview.chromium.org/2888053003/diff/160001/components/cryptauth/c... components/cryptauth/cryptauth_device_manager.cc:550: description: "..." Gets a list of the devices registered (for the same user) on CryptAuth https://codereview.chromium.org/2888053003/diff/160001/components/cryptauth/c... components/cryptauth/cryptauth_device_manager.cc:551: trigger: "..." Periodically every 24h, or by API request https://codereview.chromium.org/2888053003/diff/160001/components/cryptauth/c... components/cryptauth/cryptauth_device_manager.cc:552: data: "..." None https://codereview.chromium.org/2888053003/diff/160001/components/cryptauth/c... components/cryptauth/cryptauth_device_manager.cc:556: setting: "..." This feature cannot be disabled in settings. https://codereview.chromium.org/2888053003/diff/160001/components/cryptauth/c... components/cryptauth/cryptauth_device_manager.cc:558: [POLICY_NAME] { SigninAllowed https://codereview.chromium.org/2888053003/diff/160001/components/cryptauth/c... components/cryptauth/cryptauth_device_manager.cc:560: [POLICY_NAME]: ... //(value to disable it) SigninAllowed: false
Thank you, comments addressed, please review. https://codereview.chromium.org/2888053003/diff/160001/components/cryptauth/c... File components/cryptauth/cryptauth_device_manager.cc (right): https://codereview.chromium.org/2888053003/diff/160001/components/cryptauth/c... components/cryptauth/cryptauth_device_manager.cc:510: void CryptAuthDeviceManager::OnSyncRequested( On 2017/06/20 09:48:18, sacomoto wrote: > On 2017/06/20 05:35:34, Ramin Halavati wrote: > > On 2017/06/19 18:25:38, sacomoto wrote: > > > There are essentially two way this method can be called (and then trigger an > > > GetMyDevices API call): periodic or forced. In both cases, the only > difference > > > in the annotation should be in the "trigger" value. > > > > > > For the periodic one, we already have all the information to another the > > > request. But this is not true for the forced sync. Any user of this class > can > > > call the |ForceSyncNow| method and trigger an API call. > > > > > > For the forced sync case, I think there are two alternative here: > > > 1) Be a little bit more vague, and set the ""trigger" to forced sync > whenever > > > |invocation_reason != INVOCATION_REASON_PERIODIC && invocation_reason != > > > INVOCATION_REASON_INITIALIZATION|. > > > 2) Let the clients of this class set the "trigger" value. Concretely, add a > > > |trigger| parameter to the |ForceSyncNow| method. > > > > I think it would be good if we give a general description of both trigger > > situations here, like 'Periodically every 24h, or by API request'. > > We don't have the required structure to define more than 2 level partial > > annotations now, and I don't feel that we need to expand the structures > because > > of this case as the data and policy are very much the same and it is very > > improbable that one would like to stop this request based on a specific > trigger > > case. But if you believe so, I can start it that way. > > I'm fine with the general description. Acknowledged. https://codereview.chromium.org/2888053003/diff/160001/components/cryptauth/c... components/cryptauth/cryptauth_device_manager.cc:546: net::DefinePartialNetworkTrafficAnnotation("...", "oauth2_api_call_flow", On 2017/06/20 09:48:18, sacomoto wrote: > cryptauth_get_my_devices Done. https://codereview.chromium.org/2888053003/diff/160001/components/cryptauth/c... components/cryptauth/cryptauth_device_manager.cc:549: sender: "..." On 2017/06/20 09:48:18, sacomoto wrote: > CryptAuth Device Manager Done. https://codereview.chromium.org/2888053003/diff/160001/components/cryptauth/c... components/cryptauth/cryptauth_device_manager.cc:550: description: "..." On 2017/06/20 09:48:18, sacomoto wrote: > Gets a list of the devices registered (for the same user) on CryptAuth Done. https://codereview.chromium.org/2888053003/diff/160001/components/cryptauth/c... components/cryptauth/cryptauth_device_manager.cc:551: trigger: "..." On 2017/06/20 09:48:18, sacomoto wrote: > Periodically every 24h, or by API request Done. https://codereview.chromium.org/2888053003/diff/160001/components/cryptauth/c... components/cryptauth/cryptauth_device_manager.cc:552: data: "..." On 2017/06/20 09:48:18, sacomoto wrote: > None If sign-in is required, shouldn't this be user authentication token? Also for other requests that are dependent on sign-in. https://codereview.chromium.org/2888053003/diff/160001/components/cryptauth/c... components/cryptauth/cryptauth_device_manager.cc:556: setting: "..." On 2017/06/20 09:48:18, sacomoto wrote: > This feature cannot be disabled in settings. Done. https://codereview.chromium.org/2888053003/diff/160001/components/cryptauth/c... components/cryptauth/cryptauth_device_manager.cc:558: [POLICY_NAME] { On 2017/06/20 09:48:18, sacomoto wrote: > SigninAllowed Done. https://codereview.chromium.org/2888053003/diff/160001/components/cryptauth/c... components/cryptauth/cryptauth_device_manager.cc:560: [POLICY_NAME]: ... //(value to disable it) On 2017/06/20 09:48:18, sacomoto wrote: > SigninAllowed: false Done.
lgtm
On 2017/06/20 11:34:12, sacomoto wrote: > lgtm Thank you. Just a missed comment: Should I add "User authentication token" to |data| part of all requests that are based on sign-in as in cryptauth_device_manager.cc?
On 2017/06/20 11:36:29, Ramin Halavati wrote: > On 2017/06/20 11:34:12, sacomoto wrote: > > lgtm > > Thank you. Just a missed comment: > > Should I add "User authentication token" to |data| part of all requests that are > based on sign-in as in cryptauth_device_manager.cc? Sorry. Yes, we always send the user authentication token.
rhalavati@chromium.org changed reviewers: + msramek@chromium.org
Thank you Gustavo. Martin, Do you have any comments?
https://codereview.chromium.org/2888053003/diff/220001/components/cryptauth/c... File components/cryptauth/cryptauth_client_impl.cc (right): https://codereview.chromium.org/2888053003/diff/220001/components/cryptauth/c... components/cryptauth/cryptauth_client_impl.cc:96: "only be send if the user explicitly tries to enable Smart Lock " typo: sent https://codereview.chromium.org/2888053003/diff/220001/components/cryptauth/c... components/cryptauth/cryptauth_client_impl.cc:157: description: "Starts the CryptAuth registration flow." Since this cannot be disabled, could you expand the description to explain the motivation why this must be done regularly? https://codereview.chromium.org/2888053003/diff/220001/components/cryptauth/c... components/cryptauth/cryptauth_client_impl.cc:188: description: "Finishes the CryptAuth registration flow." Ditto here (I understand that this is related to the previous one). https://codereview.chromium.org/2888053003/diff/220001/components/cryptauth/c... File components/cryptauth/cryptauth_device_manager.cc (right): https://codereview.chromium.org/2888053003/diff/220001/components/cryptauth/c... components/cryptauth/cryptauth_device_manager.cc:551: "Gets a list of the devices registered (for the same user) on " Ditto here. https://codereview.chromium.org/2888053003/diff/220001/components/cryptauth/c... components/cryptauth/cryptauth_device_manager.cc:558: setting: "This feature cannot be disabled in settings." nit: Consistency. The previous annotation tended to say "However, ...". https://codereview.chromium.org/2888053003/diff/220001/components/signin/core... File components/signin/core/browser/refresh_token_annotation_request.cc (right): https://codereview.chromium.org/2888053003/diff/220001/components/signin/core... components/signin/core/browser/refresh_token_annotation_request.cc:183: "Sends request to /IssueToken endpoint with device_id to backfill " What is exactly is a device ID? https://codereview.chromium.org/2888053003/diff/220001/google_apis/gaia/oauth... File google_apis/gaia/oauth2_mint_token_flow.cc (right): https://codereview.chromium.org/2888053003/diff/220001/google_apis/gaia/oauth... google_apis/gaia/oauth2_mint_token_flow.cc:292: trigger: "API call from the app/extension" nit: Period at the end.
Thanks Martin, Gustavo, Could you please answer Martin's questions on the reasons behind regular calls in cryptauth_client_impl.cc and cryptauth_device_manager.cc? Also as Roger is OOO, can you comment on Martin's question regarding device_id in refresh_token_annotation_request.cc? https://codereview.chromium.org/2888053003/diff/220001/components/cryptauth/c... File components/cryptauth/cryptauth_client_impl.cc (right): https://codereview.chromium.org/2888053003/diff/220001/components/cryptauth/c... components/cryptauth/cryptauth_client_impl.cc:96: "only be send if the user explicitly tries to enable Smart Lock " On 2017/06/28 08:40:55, msramek (slow) wrote: > typo: sent Done. https://codereview.chromium.org/2888053003/diff/220001/components/cryptauth/c... File components/cryptauth/cryptauth_device_manager.cc (right): https://codereview.chromium.org/2888053003/diff/220001/components/cryptauth/c... components/cryptauth/cryptauth_device_manager.cc:558: setting: "This feature cannot be disabled in settings." On 2017/06/28 08:40:55, msramek (slow) wrote: > nit: Consistency. The previous annotation tended to say "However, ...". Done. https://codereview.chromium.org/2888053003/diff/220001/google_apis/gaia/oauth... File google_apis/gaia/oauth2_mint_token_flow.cc (right): https://codereview.chromium.org/2888053003/diff/220001/google_apis/gaia/oauth... google_apis/gaia/oauth2_mint_token_flow.cc:292: trigger: "API call from the app/extension" On 2017/06/28 08:40:55, msramek (slow) wrote: > nit: Period at the end. Done.
https://codereview.chromium.org/2888053003/diff/220001/components/cryptauth/c... File components/cryptauth/cryptauth_client_impl.cc (right): https://codereview.chromium.org/2888053003/diff/220001/components/cryptauth/c... components/cryptauth/cryptauth_client_impl.cc:157: description: "Starts the CryptAuth registration flow." On 2017/06/28 08:40:55, msramek (slow) wrote: > Since this cannot be disabled, could you expand the description to explain the > motivation why this must be done regularly? The periodic re-enrollments are part of the CryptAuth protocol, if the device doesn't re-enroll for more than XX (> 45) days then it gets removed from the server. They have two main functions: update the device information and rotate the master symmetric key negotiated during the enrollment. https://codereview.chromium.org/2888053003/diff/220001/components/cryptauth/c... components/cryptauth/cryptauth_client_impl.cc:188: description: "Finishes the CryptAuth registration flow." On 2017/06/28 08:40:55, msramek (slow) wrote: > Ditto here (I understand that this is related to the previous one). See the previous comment. https://codereview.chromium.org/2888053003/diff/220001/components/signin/core... File components/signin/core/browser/refresh_token_annotation_request.cc (right): https://codereview.chromium.org/2888053003/diff/220001/components/signin/core... components/signin/core/browser/refresh_token_annotation_request.cc:183: "Sends request to /IssueToken endpoint with device_id to backfill " On 2017/06/28 08:40:55, msramek (slow) wrote: > What is exactly is a device ID? I think this is a randomly generated ID scoped to single signin. That it's refreshed for every new sign in. This is generated here: https://cs.chromium.org/chromium/src/components/signin/core/browser/signin_cl...
Thank you Gustavo. Martin, Annotations updated, please review. https://codereview.chromium.org/2888053003/diff/220001/components/cryptauth/c... File components/cryptauth/cryptauth_client_impl.cc (right): https://codereview.chromium.org/2888053003/diff/220001/components/cryptauth/c... components/cryptauth/cryptauth_client_impl.cc:157: description: "Starts the CryptAuth registration flow." On 2017/06/30 10:09:54, sacomoto wrote: > On 2017/06/28 08:40:55, msramek (slow) wrote: > > Since this cannot be disabled, could you expand the description to explain the > > motivation why this must be done regularly? > > The periodic re-enrollments are part of the CryptAuth protocol, if the device > doesn't re-enroll for more than XX (> 45) days then it gets removed from the > server. They have two main functions: update the device information and rotate > the master symmetric key negotiated during the enrollment. Done. https://codereview.chromium.org/2888053003/diff/220001/components/cryptauth/c... components/cryptauth/cryptauth_client_impl.cc:188: description: "Finishes the CryptAuth registration flow." On 2017/06/30 10:09:54, sacomoto wrote: > On 2017/06/28 08:40:55, msramek (slow) wrote: > > Ditto here (I understand that this is related to the previous one). > > See the previous comment. Done. https://codereview.chromium.org/2888053003/diff/220001/components/cryptauth/c... File components/cryptauth/cryptauth_device_manager.cc (right): https://codereview.chromium.org/2888053003/diff/220001/components/cryptauth/c... components/cryptauth/cryptauth_device_manager.cc:551: "Gets a list of the devices registered (for the same user) on " On 2017/06/28 08:40:55, msramek (slow) wrote: > Ditto here. Acknowledged. https://codereview.chromium.org/2888053003/diff/220001/components/signin/core... File components/signin/core/browser/refresh_token_annotation_request.cc (right): https://codereview.chromium.org/2888053003/diff/220001/components/signin/core... components/signin/core/browser/refresh_token_annotation_request.cc:183: "Sends request to /IssueToken endpoint with device_id to backfill " On 2017/06/30 10:09:54, sacomoto wrote: > On 2017/06/28 08:40:55, msramek (slow) wrote: > > What is exactly is a device ID? > > I think this is a randomly generated ID scoped to single signin. That it's > refreshed for every new sign in. This is generated here: > https://cs.chromium.org/chromium/src/components/signin/core/browser/signin_cl... > Done.
LGTM, thanks for the clarifications!
The CQ bit was checked by rhalavati@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rogerta@chromium.org, sacomoto@chromium.org Link to the patchset: https://codereview.chromium.org/2888053003/#ps260001 (title: "Annotations updated.")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
Thank you Martin. Kyle, Could you please give owner's approval on components/cryptauth/* and components/proximity_auth/*
lgtm
The CQ bit was checked by rhalavati@google.com
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by rhalavati@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 260001, "attempt_start_ts": 1499317720254330,
"parent_rev": "73d4a6acb1d7516a5288928fb28648a59821f6fe", "commit_rev":
"13592b1a361d8faad62b9789fed6725afa91f008"}
Message was sent while issue was closed.
Description was changed from ========== Network traffic annotation added to OAuth2ApiCallFlow and its subclasses. Network traffic annotation is added to network requests of: chrome/browser/extensions/api/identity/identity_get_auth_token_function.cc components/cryptauth/cryptauth_api_call_flow.cc components/signin/core/browser/refresh_token_annotation_request.cc google_apis/gaia/oauth2_api_call_flow.h google_apis/gaia/oauth2_api_call_flow.cc BUG=656607 ========== to ========== Network traffic annotation added to OAuth2ApiCallFlow and its subclasses. Network traffic annotation is added to network requests of: chrome/browser/extensions/api/identity/identity_get_auth_token_function.cc components/cryptauth/cryptauth_api_call_flow.cc components/signin/core/browser/refresh_token_annotation_request.cc google_apis/gaia/oauth2_api_call_flow.h google_apis/gaia/oauth2_api_call_flow.cc BUG=656607 Review-Url: https://codereview.chromium.org/2888053003 Cr-Commit-Position: refs/heads/master@{#484474} Committed: https://chromium.googlesource.com/chromium/src/+/13592b1a361d8faad62b9789fed6... ==========
Message was sent while issue was closed.
Committed patchset #13 (id:260001) as https://chromium.googlesource.com/chromium/src/+/13592b1a361d8faad62b9789fed6... |
