Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2633363002:
Add request and response messages related to AD Play user enrollment (Closed)
|
Created:
3 years, 11 months ago by Marton Hunyady Modified:
3 years, 11 months ago CC:
chromium-reviews Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd request and response messages related to AD Play user enrollment.
The ActiveDirectoryEnrollPlayUser RPC is used when a user logs in to
the device for the first time.
The ReportUserActivity RPC is called regularly to indicate if a user
account is used on the device, making it possible to garbage collect
accounts which are no longer present on the device.
BUG=680953
Review-Url: https://codereview.chromium.org/2633363002
Cr-Commit-Position: refs/heads/master@{#445787}
Committed: https://chromium.googlesource.com/chromium/src/+/655534106723e235955ff0b4aec4cadddbd638ab
Patch Set 1 #Patch Set 2 : Naming and comment changes. #Patch Set 3 : Add new HTTP error code #Patch Set 4 : Add ReportUserActivity #Patch Set 5 : Change comment phrasing. #Patch Set 6 : Rename to ActiveDirectoryPlayActivity #
Total comments: 2
Patch Set 7 : Add comment about user_id usage. #
Total comments: 10
Patch Set 8 : Comment phrasing changes. #Messages
Total messages: 20 (13 generated)
Description was changed from ========== Add request and response message of the ActiveDirectoryArcLogin RPC. BUG=680953 ========== to ========== Add request and response messages related to AD Play user enrollment. The ActiveDirectoryEnrollPlayUser RPC is used when a user logs in to the device for the first time. The ReportUserActivity RPC is called regularly to indicate if a user account is used on the device, making it possible to garbage collect accounts which are no longer present on the device. BUG=680953 ==========
hunyadym@chromium.org changed reviewers: + tnagel@chromium.org
hunyadym@chromium.org changed reviewers: + dkalin@chromium.org
The CQ bit was checked by hunyadym@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_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) android_cronet on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...) cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
https://codereview.chromium.org/2633363002/diff/80002/components/policy/proto... File components/policy/proto/device_management_backend.proto (right): https://codereview.chromium.org/2633363002/diff/80002/components/policy/proto... components/policy/proto/device_management_backend.proto:1203: // The user id which identifies the user enrolled by this token. Nit: I'd suggest to mention in the comment that this user id is opaque to the client and cannot be used for anything except an activity request.
https://codereview.chromium.org/2633363002/diff/80002/components/policy/proto... File components/policy/proto/device_management_backend.proto (right): https://codereview.chromium.org/2633363002/diff/80002/components/policy/proto... components/policy/proto/device_management_backend.proto:1203: // The user id which identifies the user enrolled by this token. On 2017/01/24 16:59:02, Thiemo Nagel (slow) wrote: > Nit: I'd suggest to mention in the comment that this user id is opaque to the > client and cannot be used for anything except an activity request. Done.
The CQ bit was checked by hunyadym@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...
Lgtm with optional comments. https://codereview.chromium.org/2633363002/diff/110001/components/policy/prot... File components/policy/proto/device_management_backend.proto (right): https://codereview.chromium.org/2633363002/diff/110001/components/policy/prot... components/policy/proto/device_management_backend.proto:1189: // Gets an enrollment token to a Managed Google Play Account for using it with Optional nit: Suggest "Managed Google Play Account" --> "managed Google Play account" as "Google Play" is a proper noun, but "managed" and "account" are probably not part of that. https://codereview.chromium.org/2633363002/diff/110001/components/policy/prot... components/policy/proto/device_management_backend.proto:1196: // that ARC is not enabled for the domain. Optional nit: Suggest to add 4 blanks before "that". https://codereview.chromium.org/2633363002/diff/110001/components/policy/prot... components/policy/proto/device_management_backend.proto:1198: // Google Play accounts. Optional nit: Suggest to add 4 blanks before "Google". https://codereview.chromium.org/2633363002/diff/110001/components/policy/prot... components/policy/proto/device_management_backend.proto:1209: // Reports that a Managed Google Play account is used. This makes it possible to Optional nit: Suggest merging both sentences: "Reports activity for a managed Google Play account to inhibit garbage collection." https://codereview.chromium.org/2633363002/diff/110001/components/policy/prot... components/policy/proto/device_management_backend.proto:1212: // The user id which identifies the user. Optional nit: Suggest to refer to the ActiveDirectoryEnrollPlayUser request to explain where that user id is coming from.
https://codereview.chromium.org/2633363002/diff/110001/components/policy/prot... File components/policy/proto/device_management_backend.proto (right): https://codereview.chromium.org/2633363002/diff/110001/components/policy/prot... components/policy/proto/device_management_backend.proto:1189: // Gets an enrollment token to a Managed Google Play Account for using it with On 2017/01/24 18:10:21, Thiemo Nagel (slow) wrote: > Optional nit: Suggest "Managed Google Play Account" --> "managed Google Play > account" as "Google Play" is a proper noun, but "managed" and "account" are > probably not part of that. Done. https://codereview.chromium.org/2633363002/diff/110001/components/policy/prot... components/policy/proto/device_management_backend.proto:1196: // that ARC is not enabled for the domain. On 2017/01/24 18:10:21, Thiemo Nagel (slow) wrote: > Optional nit: Suggest to add 4 blanks before "that". Done. https://codereview.chromium.org/2633363002/diff/110001/components/policy/prot... components/policy/proto/device_management_backend.proto:1198: // Google Play accounts. On 2017/01/24 18:10:21, Thiemo Nagel (slow) wrote: > Optional nit: Suggest to add 4 blanks before "Google". Done. https://codereview.chromium.org/2633363002/diff/110001/components/policy/prot... components/policy/proto/device_management_backend.proto:1209: // Reports that a Managed Google Play account is used. This makes it possible to On 2017/01/24 18:10:21, Thiemo Nagel (slow) wrote: > Optional nit: Suggest merging both sentences: "Reports activity for a managed > Google Play account to inhibit garbage collection." Changed the comment to "Reports that a managed Google Play account is used. This makes the garbage collection of accounts possible by reporting the ones which are still in use." https://codereview.chromium.org/2633363002/diff/110001/components/policy/prot... components/policy/proto/device_management_backend.proto:1212: // The user id which identifies the user. On 2017/01/24 18:10:21, Thiemo Nagel (slow) wrote: > Optional nit: Suggest to refer to the ActiveDirectoryEnrollPlayUser request to > explain where that user id is coming from. Done.
The CQ bit was checked by hunyadym@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tnagel@chromium.org Link to the patchset: https://codereview.chromium.org/2633363002/#ps130001 (title: "Comment phrasing changes.")
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": 130001, "attempt_start_ts": 1485282332187920,
"parent_rev": "a972d37e1b2a724300e3104f947f8acc5829951b", "commit_rev":
"655534106723e235955ff0b4aec4cadddbd638ab"}
Message was sent while issue was closed.
Description was changed from ========== Add request and response messages related to AD Play user enrollment. The ActiveDirectoryEnrollPlayUser RPC is used when a user logs in to the device for the first time. The ReportUserActivity RPC is called regularly to indicate if a user account is used on the device, making it possible to garbage collect accounts which are no longer present on the device. BUG=680953 ========== to ========== Add request and response messages related to AD Play user enrollment. The ActiveDirectoryEnrollPlayUser RPC is used when a user logs in to the device for the first time. The ReportUserActivity RPC is called regularly to indicate if a user account is used on the device, making it possible to garbage collect accounts which are no longer present on the device. BUG=680953 Review-Url: https://codereview.chromium.org/2633363002 Cr-Commit-Position: refs/heads/master@{#445787} Committed: https://chromium.googlesource.com/chromium/src/+/655534106723e235955ff0b4aec4... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:130001) as https://chromium.googlesource.com/chromium/src/+/655534106723e235955ff0b4aec4... |
