Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1508833003:
Fix permission checking for negotiate auth on Android (Closed)
DescriptionFix permission checking for negotiate auth on Android
Removed unnecessary permission checks that were requiring a permission
even on M where they are not needed
Updated the WebViewShell manifest to properly support negotiate auth
BUG=533513
Committed: https://crrev.com/0b9b4aefc8954c096ac7f5178aa75fbf5b62ce52
Cr-Commit-Position: refs/heads/master@{#368455}
Patch Set 1 #Patch Set 2 : #
Total comments: 8
Patch Set 3 : address review comments, fix some issues #
Total comments: 11
Patch Set 4 : Document more, make logging more explicit #
Total comments: 2
Patch Set 5 : Removed the dependent patchset #
Total comments: 2
Messages
Total messages: 28 (5 generated)
dgn@chromium.org changed reviewers: + asanka@chromium.org, sgurun@chromium.org
sgurun@chromium.org: Please review changes in WebviewShell's manifest asanka@chromium.org: Please review changes in HttpNegotiateAuthenticator
https://codereview.chromium.org/1508833003/diff/20001/net/android/java/src/or... File net/android/java/src/org/chromium/net/HttpNegotiateAuthenticator.java (right): https://codereview.chromium.org/1508833003/diff/20001/net/android/java/src/or... net/android/java/src/org/chromium/net/HttpNegotiateAuthenticator.java:228: if (!hasPermission(applicationContext, "android.permission.MANAGE_ACCOUNTS")) { so reading the documentation just below, you don't need to check for the permission if build version >= M, just like above right?
https://codereview.chromium.org/1508833003/diff/20001/net/android/java/src/or... File net/android/java/src/org/chromium/net/HttpNegotiateAuthenticator.java (right): https://codereview.chromium.org/1508833003/diff/20001/net/android/java/src/or... net/android/java/src/org/chromium/net/HttpNegotiateAuthenticator.java:228: if (!hasPermission(applicationContext, "android.permission.MANAGE_ACCOUNTS")) { On 2015/12/07 22:24:16, sgurun wrote: > so reading the documentation just below, you don't need to check for the > permission if build version >= M, just like above right? In general, the check would not be needed, indeed. However, as explained in the comment, if the current application does not manage the particular account type, it can't retrieve any of them. What happens then is that the user would be prompted to add an account, that account would somehow end up being used for the current request, and then it would be discarded because the app can't save it. It would be okay to check the API lvl for the WebView case, but here that's the Chrome case (when we have access to an activity), and we know that we won't manage whatever third party account would be used for Kerberos auth. So I think it's better to fail gracefully if someone removed the Contacts permission from Chrome. Or maybe I should request it? I'm not sure where that permission is currently used. Sync? Requesting it out of the sync context would require some UX review and UI work since we need to explain why we need that permission. Shouldn't it be up to the administrator to make sure the permission is granted? I don't know if they can force it via device policy manager
Apologies. I don't think I'm a good person to review this change. This is mostly about getting Android permissions right and I don't consider myself knowledgeable in this area.
nyquist@chromium.org changed reviewers: + nyquist@chromium.org, tedchoc@chromium.org
tedchoc: Could you also have a look at this?
https://codereview.chromium.org/1508833003/diff/20001/net/android/java/src/or... File net/android/java/src/org/chromium/net/HttpNegotiateAuthenticator.java (right): https://codereview.chromium.org/1508833003/diff/20001/net/android/java/src/or... net/android/java/src/org/chromium/net/HttpNegotiateAuthenticator.java:214: // API >= 23 Requires the GET_ACCOUNTS permission or returns only the accounts This is now part of CONTACTS group https://codereview.chromium.org/1508833003/diff/20001/net/android/java/src/or... net/android/java/src/org/chromium/net/HttpNegotiateAuthenticator.java:221: requestData.accountManager.getAccountsByTypeAndFeatures( Please remind me again; if you don't have the CONTACTS permission, does this fail or does it give you an empty result? https://codereview.chromium.org/1508833003/diff/20001/net/android/java/src/or... net/android/java/src/org/chromium/net/HttpNegotiateAuthenticator.java:228: if (!hasPermission(applicationContext, "android.permission.MANAGE_ACCOUNTS")) { On 2015/12/08 11:00:37, dgn wrote: > On 2015/12/07 22:24:16, sgurun wrote: > > so reading the documentation just below, you don't need to check for the > > permission if build version >= M, just like above right? > > In general, the check would not be needed, indeed. However, as explained in the > comment, if the current application does not manage the particular account type, > it can't retrieve any of them. What happens then is that the user would be > prompted to add an account, that account would somehow end up being used for the > current request, and then it would be discarded because the app can't save it. > > It would be okay to check the API lvl for the WebView case, but here that's the > Chrome case (when we have access to an activity), and we know that we won't > manage whatever third party account would be used for Kerberos auth. So I think > it's better to fail gracefully if someone removed the Contacts permission from > Chrome. > > Or maybe I should request it? I'm not sure where that permission is currently > used. Sync? Requesting it out of the sync context would require some UX review > and UI work since we need to explain why we need that permission. Shouldn't it > be up to the administrator to make sure the permission is granted? I don't know > if they can force it via device policy manager Chrome currently has CONTACTS permission, but it's never requested for Android M. The reason why it's still part of the manifest for SDK 23+ is that we haven't changed the manifest to conditionally remove it. You can not assume we have the CONTACTS permission in Chrome. For "com.google" account type though, official builds can get tokens, but you'd have to have access to org.chromium.chrome.browser.ChromeApplication#createAccountManagerDelegate(), which is not the case for //net. However for other parts of the code that live outside of //chrome, we inject that when the application is started: https://chromium.googlesource.com/chromium/src/+/61153f138c728fb3e71cb2b8ff55... Could something similar be used for //net? Or is this code always unrelated to "com.google" account types?
https://codereview.chromium.org/1508833003/diff/20001/net/android/java/src/or... File net/android/java/src/org/chromium/net/HttpNegotiateAuthenticator.java (right): https://codereview.chromium.org/1508833003/diff/20001/net/android/java/src/or... net/android/java/src/org/chromium/net/HttpNegotiateAuthenticator.java:214: // API >= 23 Requires the GET_ACCOUNTS permission or returns only the accounts On 2015/12/09 03:38:13, nyquist wrote: > This is now part of CONTACTS group Updated the comment. https://codereview.chromium.org/1508833003/diff/20001/net/android/java/src/or... net/android/java/src/org/chromium/net/HttpNegotiateAuthenticator.java:221: requestData.accountManager.getAccountsByTypeAndFeatures( On 2015/12/09 03:38:13, nyquist wrote: > Please remind me again; if you don't have the CONTACTS permission, does this > fail or does it give you an empty result? It returns an empty result on M, throws a runtime exception before. I tried to describe it in the comment above and in the callback code. Should I add something else here? https://codereview.chromium.org/1508833003/diff/20001/net/android/java/src/or... net/android/java/src/org/chromium/net/HttpNegotiateAuthenticator.java:228: if (!hasPermission(applicationContext, "android.permission.MANAGE_ACCOUNTS")) { On 2015/12/09 03:38:13, nyquist wrote: > On 2015/12/08 11:00:37, dgn wrote: > > On 2015/12/07 22:24:16, sgurun wrote: > > > so reading the documentation just below, you don't need to check for the > > > permission if build version >= M, just like above right? > > > > In general, the check would not be needed, indeed. However, as explained in > the > > comment, if the current application does not manage the particular account > type, > > it can't retrieve any of them. What happens then is that the user would be > > prompted to add an account, that account would somehow end up being used for > the > > current request, and then it would be discarded because the app can't save it. > > > > It would be okay to check the API lvl for the WebView case, but here that's > the > > Chrome case (when we have access to an activity), and we know that we won't > > manage whatever third party account would be used for Kerberos auth. So I > think > > it's better to fail gracefully if someone removed the Contacts permission from > > Chrome. > > > > Or maybe I should request it? I'm not sure where that permission is currently > > used. Sync? Requesting it out of the sync context would require some UX review > > and UI work since we need to explain why we need that permission. Shouldn't it > > be up to the administrator to make sure the permission is granted? I don't > know > > if they can force it via device policy manager > > Chrome currently has CONTACTS permission, but it's never requested for Android > M. The reason why it's still part of the manifest for SDK 23+ is that we haven't > changed the manifest to conditionally remove it. You can not assume we have the > CONTACTS permission in Chrome. > > For "com.google" account type though, official builds can get tokens, but you'd > have to have access to > org.chromium.chrome.browser.ChromeApplication#createAccountManagerDelegate(), > which is not the case for //net. > > However for other parts of the code that live outside of //chrome, we inject > that when the application is started: > https://chromium.googlesource.com/chromium/src/+/61153f138c728fb3e71cb2b8ff55... > > Could something similar be used for //net? > Or is this code always unrelated to "com.google" account types? This code is unrelated to "com.google" account types, yes. So I'll let this check in, which will show a ERR_MISCONFIGURED_AUTH_ENVIRONMENT error if users try to access the page. Administrators can look at the logs to find more details. I'm not sure what amount of details I should put in the logs though. I probably should just put more details on the explanation page on chromium.org. https://codereview.chromium.org/1508833003/diff/40001/net/android/java/src/or... File net/android/java/src/org/chromium/net/HttpNegotiateAuthenticator.java (right): https://codereview.chromium.org/1508833003/diff/40001/net/android/java/src/or... net/android/java/src/org/chromium/net/HttpNegotiateAuthenticator.java:305: } else { Split the check here, MANAGE_ACCOUNTS has been removed on M, and GET_ACCOUNTS is the one that is checked instead when trying to retrieve accounts (like above)
lgtm
https://codereview.chromium.org/1508833003/diff/40001/net/android/java/src/or... File net/android/java/src/org/chromium/net/HttpNegotiateAuthenticator.java (right): https://codereview.chromium.org/1508833003/diff/40001/net/android/java/src/or... net/android/java/src/org/chromium/net/HttpNegotiateAuthenticator.java:101: Log.e(TAG, "USE_CREDENTIALS permission not granted. Aborting authentication."); nit: why is this log an error while the one above is a warning? error is probably appropriate since functionality will not work in the case. https://codereview.chromium.org/1508833003/diff/40001/net/android/java/src/or... net/android/java/src/org/chromium/net/HttpNegotiateAuthenticator.java:314: int permissionResult = it seems that anytime hasPermission fails, you abort the authentication. put a Log.e here indicating failure, and get rid of the others above.
https://codereview.chromium.org/1508833003/diff/40001/net/android/java/src/or... File net/android/java/src/org/chromium/net/HttpNegotiateAuthenticator.java (right): https://codereview.chromium.org/1508833003/diff/40001/net/android/java/src/or... net/android/java/src/org/chromium/net/HttpNegotiateAuthenticator.java:101: Log.e(TAG, "USE_CREDENTIALS permission not granted. Aborting authentication."); On 2015/12/09 21:35:40, sgurun wrote: > nit: why is this log an error while the one above is a warning? error is > probably appropriate since functionality will not work in the case. The errors here were logged as warning in general. Mostly they are temporary errors. Maybe the user just needs to log in to the Admin app, or something like that. I set them as errors for the permission related ones because I figured that's more severe issues that would make some apps completely unusable. Also, returning ERR_MISCONFIGURED_AUTH_ENVIRONMENT will disable negotiate for a while, rather than just saying that the request failed. Not sure if making this distinction in the logs is uselful.
lgtm https://codereview.chromium.org/1508833003/diff/40001/net/android/java/src/or... File net/android/java/src/org/chromium/net/HttpNegotiateAuthenticator.java (right): https://codereview.chromium.org/1508833003/diff/40001/net/android/java/src/or... net/android/java/src/org/chromium/net/HttpNegotiateAuthenticator.java:101: Log.e(TAG, "USE_CREDENTIALS permission not granted. Aborting authentication."); On 2015/12/10 10:23:15, dgn wrote: > On 2015/12/09 21:35:40, sgurun wrote: > > nit: why is this log an error while the one above is a warning? error is > > probably appropriate since functionality will not work in the case. > > The errors here were logged as warning in general. Mostly they are temporary > errors. Maybe the user just needs to log in to the Admin app, or something like > that. I set them as errors for the permission related ones because I figured > that's more severe issues that would make some apps completely unusable. Also, > returning ERR_MISCONFIGURED_AUTH_ENVIRONMENT will disable negotiate for a while, > rather than just saying that the request failed. > Not sure if making this distinction in the logs is uselful. ok, probably asanka is a better person to judge the log levels for network stack anyway.
thanks. asanka@: PTAL. The permission handling seem okay, but can you please confirm that the returned codes are appropriate? They didn't change from the previous versions of the code though.
Sorry about the delay, and please ping me if you don't get a response in a timely manner. Things do fall off the plate occasionally. My comment about surfacing the account misconfiguration probably doesn't apply to this CL. I was curious what the user visible behavior is when something like that happens. https://codereview.chromium.org/1508833003/diff/40001/net/android/java/src/or... File net/android/java/src/org/chromium/net/HttpNegotiateAuthenticator.java (right): https://codereview.chromium.org/1508833003/diff/40001/net/android/java/src/or... net/android/java/src/org/chromium/net/HttpNegotiateAuthenticator.java:90: Log.w(TAG, "Unable to obtain a unique eligible account for negotiate auth."); Suggest distinguishing between no accounts vs. more than one account. If this happens, the provider isn't notified, correct? How would the user notice? https://codereview.chromium.org/1508833003/diff/40001/net/android/java/src/or... net/android/java/src/org/chromium/net/HttpNegotiateAuthenticator.java:101: Log.e(TAG, "USE_CREDENTIALS permission not granted. Aborting authentication."); On 2015/12/10 18:43:35, sgurun wrote: > On 2015/12/10 10:23:15, dgn wrote: > > On 2015/12/09 21:35:40, sgurun wrote: > > > nit: why is this log an error while the one above is a warning? error is > > > probably appropriate since functionality will not work in the case. > > > > The errors here were logged as warning in general. Mostly they are temporary > > errors. Maybe the user just needs to log in to the Admin app, or something > like > > that. I set them as errors for the permission related ones because I figured > > that's more severe issues that would make some apps completely unusable. Also, > > returning ERR_MISCONFIGURED_AUTH_ENVIRONMENT will disable negotiate for a > while, > > rather than just saying that the request failed. > > Not sure if making this distinction in the logs is uselful. > > ok, probably asanka is a better person to judge the log levels for network stack > anyway. I'm okay with treating a misconfigured accounts as a warning and treating app permission issues as errors. But what really matters in the end is the error code that's sent back to Chrome which determines how Chrome handles the error.
https://codereview.chromium.org/1508833003/diff/40001/net/android/java/src/or... File net/android/java/src/org/chromium/net/HttpNegotiateAuthenticator.java (right): https://codereview.chromium.org/1508833003/diff/40001/net/android/java/src/or... net/android/java/src/org/chromium/net/HttpNegotiateAuthenticator.java:90: Log.w(TAG, "Unable to obtain a unique eligible account for negotiate auth."); On 2015/12/19 05:05:27, asanka wrote: > Suggest distinguishing between no accounts vs. more than one account. > > If this happens, the provider isn't notified, correct? How would the user > notice? Currently the only thing user sees are error pages with an error code. We don't have plans to show special user friendly error pages. We assume that this would be used in the context of a company with their internal websites. Then the support could be provided internally. Is it enough if I explain the error codes in the documentation (https://goo.gl/46hmKx), have more explicit logs and also include the error code in them for easy grepping? https://codereview.chromium.org/1508833003/diff/40001/net/android/java/src/or... net/android/java/src/org/chromium/net/HttpNegotiateAuthenticator.java:101: Log.e(TAG, "USE_CREDENTIALS permission not granted. Aborting authentication."); On 2015/12/19 05:05:27, asanka wrote: > On 2015/12/10 18:43:35, sgurun wrote: > > On 2015/12/10 10:23:15, dgn wrote: > > > On 2015/12/09 21:35:40, sgurun wrote: > > > > nit: why is this log an error while the one above is a warning? error is > > > > probably appropriate since functionality will not work in the case. > > > > > > The errors here were logged as warning in general. Mostly they are temporary > > > errors. Maybe the user just needs to log in to the Admin app, or something > > like > > > that. I set them as errors for the permission related ones because I figured > > > that's more severe issues that would make some apps completely unusable. > Also, > > > returning ERR_MISCONFIGURED_AUTH_ENVIRONMENT will disable negotiate for a > > while, > > > rather than just saying that the request failed. > > > Not sure if making this distinction in the logs is uselful. > > > > ok, probably asanka is a better person to judge the log levels for network > stack > > anyway. > > I'm okay with treating a misconfigured accounts as a warning and treating app > permission issues as errors. But what really matters in the end is the error > code that's sent back to Chrome which determines how Chrome handles the error. AFAICT, ERR_MISCONFIGURED_AUTH_ENVIRONMENT would disable negotiate until chrome is restarted and the other codes that I use just make the request fail. I'm not very familiar with the //net code though. The error code is also displayed on the failure page, so I wanted them to give some meaningful info to admins trying to debug their setup, rather than just ERR_UNEXPECTED. Should I just use logs for that?
https://codereview.chromium.org/1508833003/diff/40001/net/android/java/src/or... File net/android/java/src/org/chromium/net/HttpNegotiateAuthenticator.java (right): https://codereview.chromium.org/1508833003/diff/40001/net/android/java/src/or... net/android/java/src/org/chromium/net/HttpNegotiateAuthenticator.java:90: Log.w(TAG, "Unable to obtain a unique eligible account for negotiate auth."); On 2015/12/21 17:54:51, dgn wrote: > On 2015/12/19 05:05:27, asanka wrote: > > Suggest distinguishing between no accounts vs. more than one account. > > > > If this happens, the provider isn't notified, correct? How would the user > > notice? > > Currently the only thing user sees are error pages with an error code. We don't > have plans to show special user friendly error pages. We assume that this would > be used in the context of a company with their internal websites. Then the > support could be provided internally. > > Is it enough if I explain the error codes in the documentation > (https://goo.gl/46hmKx), have more explicit logs and also include the error code > in them for easy grepping? Yeah. This one is tricky since the browser is the only entity that knows what really happened. The server will see a request for which it will respond with a 401 page. The auth provider will see nothing. Since this is for an enterprise feature, then it's likely that the server operator will be able to tailor the 401 page to suggest how to resolve this. But it's a good idea to specifically call this out in the documentation.
lgtm
Thanks. Would you mind taking a last look? I changed a return code but aside from that functionality should be the same. The rest is refactoring and documentation. https://codereview.chromium.org/1508833003/diff/40001/net/android/java/src/or... File net/android/java/src/org/chromium/net/HttpNegotiateAuthenticator.java (right): https://codereview.chromium.org/1508833003/diff/40001/net/android/java/src/or... net/android/java/src/org/chromium/net/HttpNegotiateAuthenticator.java:314: int permissionResult = On 2015/12/09 21:35:40, sgurun wrote: > it seems that anytime hasPermission fails, you abort the authentication. put a > Log.e here indicating failure, and get rid of the others above. Left the individual logs to be able to give more detailed messages https://codereview.chromium.org/1508833003/diff/60001/net/android/java/src/or... File net/android/java/src/org/chromium/net/HttpNegotiateAuthenticator.java (right): https://codereview.chromium.org/1508833003/diff/60001/net/android/java/src/or... net/android/java/src/org/chromium/net/HttpNegotiateAuthenticator.java:113: NetError.ERR_MISSING_AUTH_CREDENTIALS, null); Changed from INVALID to MISSING since we don't have anything to use. https://codereview.chromium.org/1508833003/diff/60001/net/android/java/src/or... net/android/java/src/org/chromium/net/HttpNegotiateAuthenticator.java:123: NetError.ERR_MISSING_AUTH_CREDENTIALS, null); Changed from INVALID to MISSING since the credentials have not been denied, we just don't know what to use. https://codereview.chromium.org/1508833003/diff/80001/net/android/java/src/or... File net/android/java/src/org/chromium/net/HttpNegotiateAuthenticator.java (right): https://codereview.chromium.org/1508833003/diff/80001/net/android/java/src/or... net/android/java/src/org/chromium/net/HttpNegotiateAuthenticator.java:113: NetError.ERR_MISSING_AUTH_CREDENTIALS, null); Changed from INVALID to MISSING since we don't have anything to use. https://codereview.chromium.org/1508833003/diff/80001/net/android/java/src/or... net/android/java/src/org/chromium/net/HttpNegotiateAuthenticator.java:123: NetError.ERR_MISSING_AUTH_CREDENTIALS, null); Changed from INVALID to MISSING since the credentials have not been denied, we just don't know what to use.
Friendly ping. I know it already got lgtm'ed but I moved quite a lot of things around, so you might want to have a look. Otherwise I'll just land it.
lgtm
The CQ bit was checked by dgn@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sgurun@chromium.org, nyquist@chromium.org Link to the patchset: https://codereview.chromium.org/1508833003/#ps80001 (title: "Removed the dependent patchset")
On 2016/01/08 21:30:58, asanka wrote: > lgtm thanks!
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1508833003/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1508833003/80001
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Fix permission checking for negotiate auth on Android Removed unnecessary permission checks that were requiring a permission even on M where they are not needed Updated the WebViewShell manifest to properly support negotiate auth BUG=533513 ========== to ========== Fix permission checking for negotiate auth on Android Removed unnecessary permission checks that were requiring a permission even on M where they are not needed Updated the WebViewShell manifest to properly support negotiate auth BUG=533513 Committed: https://crrev.com/0b9b4aefc8954c096ac7f5178aa75fbf5b62ce52 Cr-Commit-Position: refs/heads/master@{#368455} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/0b9b4aefc8954c096ac7f5178aa75fbf5b62ce52 Cr-Commit-Position: refs/heads/master@{#368455} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
