Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2745533002:
Fixed permission check in SystemAccountManagerDelegate.updateCredentials (Closed)
DescriptionFixed permission check in SystemAccountManagerDelegate.updateCredentials
Checking hasGetAccountsPermission before calling android.AccountManager.updateCredentials
is irrelevant, because this call never required GET_ACCOUNTS permission.
Added check for MANAGE_ACCOUNTS permission that was required before
Android M. On M+ it doesn't require any special permission.
BUG=699050
Review-Url: https://codereview.chromium.org/2745533002
Cr-Commit-Position: refs/heads/master@{#456124}
Committed: https://chromium.googlesource.com/chromium/src/+/6e26494d0caa38a608ecd3c7852a58387dcaaf96
Patch Set 1 #
Total comments: 2
Patch Set 2 : Added MANAGE_ACCOUNTS check #
Total comments: 4
Messages
Total messages: 26 (16 generated)
bsazonov@chromium.org changed reviewers: + msarda@chromium.org
Mihai, please take a look.
The CQ bit was checked by bsazonov@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.
Code LGTM, however I do not really know how permissions work. Maybe get a second review from Tommy or Ganggui. Please add a link in the CL description to the document that mentions that MANAGE_ACCOUNTS permission is required.
nyquist@chromium.org changed reviewers: + nyquist@chromium.org
https://codereview.chromium.org/2745533002/diff/1/components/signin/core/brow... File components/signin/core/browser/android/java/src/org/chromium/components/signin/SystemAccountManagerDelegate.java (left): https://codereview.chromium.org/2745533002/diff/1/components/signin/core/brow... components/signin/core/browser/android/java/src/org/chromium/components/signin/SystemAccountManagerDelegate.java:164: if (!hasGetAccountsPermission()) { Can we instead check whether we have MANAGE_ACCOUNTS permission here? Same concept, just add a new method?
Description was changed from ========== Removed irrelevant GET_ACCOUNTS permission check from updateCredentials Checking hasGetAccountsPermission before calling android.AccountManager.updateCredentials is irrelevant, because this call never required GET_ACCOUNTS permission. Up to API level 22 it required MANAGE_ACCOUNTS, and on M+ it doesn't require any special permission. BUG=699050 ========== to ========== Fixed permission check in SystemAccountManagerDelegate.updateCredentials Checking hasGetAccountsPermission before calling android.AccountManager.updateCredentials is irrelevant, because this call never required GET_ACCOUNTS permission. Added check for MANAGE_ACCOUNTS permission that was required before Android M. On M+ it doesn't require any special permission. BUG=699050 ==========
Patchset #2 (id:20001) has been deleted
https://codereview.chromium.org/2745533002/diff/1/components/signin/core/brow... File components/signin/core/browser/android/java/src/org/chromium/components/signin/SystemAccountManagerDelegate.java (left): https://codereview.chromium.org/2745533002/diff/1/components/signin/core/brow... components/signin/core/browser/android/java/src/org/chromium/components/signin/SystemAccountManagerDelegate.java:164: if (!hasGetAccountsPermission()) { On 2017/03/09 22:16:55, nyquist OOO back 3-14 wrote: > Can we instead check whether we have MANAGE_ACCOUNTS permission here? Same > concept, just add a new method? Done.
The CQ bit was checked by bsazonov@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.
On 2017/03/10 17:49:17, bsazonov wrote: > https://codereview.chromium.org/2745533002/diff/1/components/signin/core/brow... > File > components/signin/core/browser/android/java/src/org/chromium/components/signin/SystemAccountManagerDelegate.java > (left): > > https://codereview.chromium.org/2745533002/diff/1/components/signin/core/brow... > components/signin/core/browser/android/java/src/org/chromium/components/signin/SystemAccountManagerDelegate.java:164: > if (!hasGetAccountsPermission()) { > On 2017/03/09 22:16:55, nyquist OOO back 3-14 wrote: > > Can we instead check whether we have MANAGE_ACCOUNTS permission here? Same > > concept, just add a new method? > > Done. Tommy, I've added the check you requested. I'm going to land this CL now (I mistakenly landed https://codereview.chromium.org/2740873004/ before this one, which is not very good). If you have any more requests regarding this CL - email me and I'll fix it.
The CQ bit was checked by bsazonov@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from msarda@chromium.org Link to the patchset: https://codereview.chromium.org/2745533002/#ps40001 (title: "Added MANAGE_ACCOUNTS check")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/03/09 14:15:48, msarda wrote: > Code LGTM, however I do not really know how permissions work. Maybe get a second > review from Tommy or Ganggui. > > Please add a link in the CL description to the document that mentions that > MANAGE_ACCOUNTS permission is required. I think that this link is probably way to big to add it to the CL description, but it's better to have it at least in the comments: https://developer.android.com/reference/android/accounts/AccountManager.html#..., java.lang.String, android.os.Bundle, android.app.Activity, android.accounts.AccountManagerCallback<android.os.Bundle>, android.os.Handler)
https://codereview.chromium.org/2745533002/diff/40001/components/signin/core/... File components/signin/core/browser/android/java/src/org/chromium/components/signin/SystemAccountManagerDelegate.java (right): https://codereview.chromium.org/2745533002/diff/40001/components/signin/core/... components/signin/core/browser/android/java/src/org/chromium/components/signin/SystemAccountManagerDelegate.java:166: if (callback != null) { if (callback == null) return; It reduces indents, etc... https://codereview.chromium.org/2745533002/diff/40001/components/signin/core/... components/signin/core/browser/android/java/src/org/chromium/components/signin/SystemAccountManagerDelegate.java:206: if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.M) { Optional nit: This can just be on one line. Fine like this too though.
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1489172524005910,
"parent_rev": "7107402deff7733287a8f0a37c2198b0a86e94bf", "commit_rev":
"6e26494d0caa38a608ecd3c7852a58387dcaaf96"}
Message was sent while issue was closed.
Description was changed from ========== Fixed permission check in SystemAccountManagerDelegate.updateCredentials Checking hasGetAccountsPermission before calling android.AccountManager.updateCredentials is irrelevant, because this call never required GET_ACCOUNTS permission. Added check for MANAGE_ACCOUNTS permission that was required before Android M. On M+ it doesn't require any special permission. BUG=699050 ========== to ========== Fixed permission check in SystemAccountManagerDelegate.updateCredentials Checking hasGetAccountsPermission before calling android.AccountManager.updateCredentials is irrelevant, because this call never required GET_ACCOUNTS permission. Added check for MANAGE_ACCOUNTS permission that was required before Android M. On M+ it doesn't require any special permission. BUG=699050 Review-Url: https://codereview.chromium.org/2745533002 Cr-Commit-Position: refs/heads/master@{#456124} Committed: https://chromium.googlesource.com/chromium/src/+/6e26494d0caa38a608ecd3c7852a... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:40001) as https://chromium.googlesource.com/chromium/src/+/6e26494d0caa38a608ecd3c7852a...
Message was sent while issue was closed.
https://codereview.chromium.org/2745533002/diff/40001/components/signin/core/... File components/signin/core/browser/android/java/src/org/chromium/components/signin/SystemAccountManagerDelegate.java (right): https://codereview.chromium.org/2745533002/diff/40001/components/signin/core/... components/signin/core/browser/android/java/src/org/chromium/components/signin/SystemAccountManagerDelegate.java:166: if (callback != null) { On 2017/03/10 19:07:45, nyquist OOO back 3-14 wrote: > if (callback == null) return; > > It reduces indents, etc... The Style Guide allows both one-line and braced if in this case, so it's up to the developer (and the code consistency). I think that returns on separate lines are easier to spot (and the rest of the file uses braced ifs anyway). If you want, I can replace it with: if (!hasManageAccountsPermission()) { if (callback == null) { return; } ThreadUtils.postOnUiThread(new Runnable() { @Override public void run() { callback.onResult(false); } }); return; } Should I? https://codereview.chromium.org/2745533002/diff/40001/components/signin/core/... components/signin/core/browser/android/java/src/org/chromium/components/signin/SystemAccountManagerDelegate.java:206: if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.M) { On 2017/03/10 19:07:45, nyquist OOO back 3-14 wrote: > Optional nit: This can just be on one line. Fine like this too though. See previous comment. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
