Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1431473004:
Add Kerberos support to webview (Closed)
|
Created:
5 years, 1 month ago by dgn Modified:
5 years ago CC:
chromium-reviews, cbentzel+watch_chromium.org, android-webview-reviews_chromium.org, aberent Base URL:
https://chromium.googlesource.com/chromium/src.git@ActivitylessNegoAuth Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd Kerberos support to webview
Add negotiate handler to WebView's http auth handlers and
adds support for the policies (AuthAndroidNegotiateAccountType and
AuthServerWhitelist) required to configure Kerberos in WebView
BUG=533513
Committed: https://crrev.com/4779a19456ede067cb623eb117570e9bf5780239
Cr-Commit-Position: refs/heads/master@{#361878}
Patch Set 1 #
Total comments: 1
Patch Set 2 : Added permission checks #Patch Set 3 : Rebase #
Total comments: 6
Patch Set 4 : Address review comments #
Total comments: 28
Patch Set 5 : rebase, address some review comments #Patch Set 6 : Fix HttpAuthHandlerNegotiate factory creation #Patch Set 7 : Fix some formatting and imports #Patch Set 8 : Document the new supported policies #Depends on Patchset: Messages
Total messages: 28 (8 generated)
dgn@chromium.org changed reviewers: + bauerb@chromium.org
PTAL. A couple of CLs still need to land before this one, but it should be in its (pre-review) final form. https://codereview.chromium.org/1431473004/diff/1/android_webview/test/shell/... File android_webview/test/shell/src/org/chromium/android_webview/shell/AwShellActivity.java (right): https://codereview.chromium.org/1431473004/diff/1/android_webview/test/shell/... android_webview/test/shell/src/org/chromium/android_webview/shell/AwShellActivity.java:122: AwBrowserProcess.start(this, getApplication()); This change will be removed as the dependency on https://codereview.chromium.org/1378653002#ps40001 is replaced by the new way to get ApplicationContext
dgn@chromium.org changed reviewers: + sgurun@chromium.org
PTAL
LGTM w/ nits: https://codereview.chromium.org/1431473004/diff/40001/android_webview/browser... File android_webview/browser/aw_browser_policy_connector.cc (right): https://codereview.chromium.org/1431473004/diff/40001/android_webview/browser... android_webview/browser/aw_browser_policy_connector.cc:45: // Negotiate authenticaton Nit: "authentication". Also, maybe "HTTP Negotiate authentication", so that it's clear this is about network? https://codereview.chromium.org/1431473004/diff/40001/android_webview/browser... File android_webview/browser/net/aw_url_request_context_getter.cc (right): https://codereview.chromium.org/1431473004/diff/40001/android_webview/browser... android_webview/browser/net/aw_url_request_context_getter.cc:63: net::HostResolver::CreateDefaultResolver(NULL))); Use nullptr rather than NULL. https://codereview.chromium.org/1431473004/diff/40001/android_webview/common/... File android_webview/common/pref_names.h (right): https://codereview.chromium.org/1431473004/diff/40001/android_webview/common/... android_webview/common/pref_names.h:17: } // namespace android_webview This is the wrong way around 😃
https://codereview.chromium.org/1431473004/diff/40001/android_webview/browser... File android_webview/browser/aw_browser_policy_connector.cc (right): https://codereview.chromium.org/1431473004/diff/40001/android_webview/browser... android_webview/browser/aw_browser_policy_connector.cc:45: // Negotiate authenticaton On 2015/11/10 17:51:28, Bernhard Bauer wrote: > Nit: "authentication". Also, maybe "HTTP Negotiate authentication", so that it's > clear this is about network? Done. https://codereview.chromium.org/1431473004/diff/40001/android_webview/browser... File android_webview/browser/net/aw_url_request_context_getter.cc (right): https://codereview.chromium.org/1431473004/diff/40001/android_webview/browser... android_webview/browser/net/aw_url_request_context_getter.cc:63: net::HostResolver::CreateDefaultResolver(NULL))); On 2015/11/10 17:51:29, Bernhard Bauer wrote: > Use nullptr rather than NULL. Done. https://codereview.chromium.org/1431473004/diff/40001/android_webview/common/... File android_webview/common/pref_names.h (right): https://codereview.chromium.org/1431473004/diff/40001/android_webview/common/... android_webview/common/pref_names.h:17: } // namespace android_webview On 2015/11/10 17:51:29, Bernhard Bauer wrote: > This is the wrong way around 😃 😳 Thanks, done.
On 2015/11/11 12:26:36, dgn wrote: > https://codereview.chromium.org/1431473004/diff/40001/android_webview/browser... > File android_webview/browser/aw_browser_policy_connector.cc (right): > > https://codereview.chromium.org/1431473004/diff/40001/android_webview/browser... > android_webview/browser/aw_browser_policy_connector.cc:45: // Negotiate > authenticaton > On 2015/11/10 17:51:28, Bernhard Bauer wrote: > > Nit: "authentication". Also, maybe "HTTP Negotiate authentication", so that > it's > > clear this is about network? > > Done. > > https://codereview.chromium.org/1431473004/diff/40001/android_webview/browser... > File android_webview/browser/net/aw_url_request_context_getter.cc (right): > > https://codereview.chromium.org/1431473004/diff/40001/android_webview/browser... > android_webview/browser/net/aw_url_request_context_getter.cc:63: > net::HostResolver::CreateDefaultResolver(NULL))); > On 2015/11/10 17:51:29, Bernhard Bauer wrote: > > Use nullptr rather than NULL. > > Done. > > https://codereview.chromium.org/1431473004/diff/40001/android_webview/common/... > File android_webview/common/pref_names.h (right): > > https://codereview.chromium.org/1431473004/diff/40001/android_webview/common/... > android_webview/common/pref_names.h:17: } // namespace android_webview > On 2015/11/10 17:51:29, Bernhard Bauer wrote: > > This is the wrong way around 😃 > > 😳 Thanks, done. will look at this tomorrow.
Description was changed from ========== Add Kerberos support to webview Add negotiate handler to WebView's http auth handlers and adds support for the policies (AuthAndroidNegotiateAccountType and AuthServerWhitelist) required to configure Kerberos in WebView BUG=533513 ========== to ========== Add Kerberos support to webview Add negotiate handler to WebView's http auth handlers and adds support for the policies (AuthAndroidNegotiateAccountType and AuthServerWhitelist) required to configure Kerberos in WebView BUG=533513 ==========
After discussing with aberent@ about his cl that enables dynamically updating auth policies (https://codereview.chromium.org/1414313002/), it looks like there are a few things not properly supported by the //net APIs. For example, net::URLRequestContextBuilder::add_http_auth_handler_factory() implies creating factories that are currently declared EXPORT_PRIVATE, and I didn't see any other satisfying way to set up the factories. His CL will fix some duplication with the policy declaration in //chrome, but all the Builder related code does not seem very friendly for android to register auth factories.
https://codereview.chromium.org/1431473004/diff/60001/android_webview/browser... File android_webview/browser/aw_browser_context.cc (right): https://codereview.chromium.org/1431473004/diff/60001/android_webview/browser... android_webview/browser/aw_browser_context.cc:186: browser_policy_connector_.reset(new AwBrowserPolicyConnector()); I see why InitUserPrefService had to move up but is there a reason to move browser_policy_connector_ creation? https://codereview.chromium.org/1431473004/diff/60001/android_webview/browser... android_webview/browser/aw_browser_context.cc:304: // Create user pref service nit: s/Create/Initialize/ https://codereview.chromium.org/1431473004/diff/60001/android_webview/browser... File android_webview/browser/net/aw_url_request_context_getter.cc (right): https://codereview.chromium.org/1431473004/diff/60001/android_webview/browser... android_webview/browser/net/aw_url_request_context_getter.cc:212: CreateCmdLineConfiguredHostResolver()); move creation below, right before where it is used. https://codereview.chromium.org/1431473004/diff/60001/android_webview/browser... android_webview/browser/net/aw_url_request_context_getter.cc:235: builder.add_http_auth_handler_factory( the documentation for add_http_auth_handler_factory says "Build must be called after this method"? probably an outdated comment but please re-confirm and if outdated please remove the comment since it is misleading. I did not see any other user of add_http_auth_handler_factory. https://codereview.chromium.org/1431473004/diff/60001/android_webview/browser... android_webview/browser/net/aw_url_request_context_getter.cc:314: DCHECK(resolver); looks like this check is in a weird place. Either move to function prologue or before where it is first used. https://codereview.chromium.org/1431473004/diff/60001/android_webview/browser... File android_webview/browser/net/aw_url_request_context_getter.h (right): https://codereview.chromium.org/1431473004/diff/60001/android_webview/browser... android_webview/browser/net/aw_url_request_context_getter.h:74: scoped_ptr<net::HttpAuthHandlerNegotiate::Factory> yeah, how will this work, it is only NET_EXPORT_PRIVATE. https://codereview.chromium.org/1431473004/diff/60001/android_webview/common/... File android_webview/common/pref_names.cc (right): https://codereview.chromium.org/1431473004/diff/60001/android_webview/common/... android_webview/common/pref_names.cc:14: "AuthAndroidNegotiateAccountType"; is there a reason to choose a different string than what chrome is using for pref value? https://codereview.chromium.org/1431473004/diff/60001/android_webview/common/... File android_webview/common/pref_names.h (right): https://codereview.chromium.org/1431473004/diff/60001/android_webview/common/... android_webview/common/pref_names.h:10: // Mirrors select preferences from chrome/common/pref_names.h if this file is just a mirror from preferences in chrome, maybe the chrome preferences should move down one layer so it can be used by both webview and chrome. Why do we replicate it? Also discounting the mirroring, the prefs are only used at the browser side, so probably this file should not be in common. https://codereview.chromium.org/1431473004/diff/60001/android_webview/common/... android_webview/common/pref_names.h:14: extern const char kAuthAndroidNegotiateAccountType[]; creating this file only for two const char * seems unnecessary. Are you expecting to add more pref names in future? https://codereview.chromium.org/1431473004/diff/60001/android_webview/java/sr... File android_webview/java/src/org/chromium/android_webview/AwBrowserProcess.java (right): https://codereview.chromium.org/1431473004/diff/60001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/AwBrowserProcess.java:70: CombinedPolicyProvider.get().registerProvider(new AwPolicyProvider(context)); please remind me where the user manager reads the application restrictions from. did you check if it does add any measurable latency?
https://codereview.chromium.org/1431473004/diff/60001/android_webview/browser... File android_webview/browser/aw_browser_context.cc (right): https://codereview.chromium.org/1431473004/diff/60001/android_webview/browser... android_webview/browser/aw_browser_context.cc:186: browser_policy_connector_.reset(new AwBrowserPolicyConnector()); On 2015/11/13 08:43:51, sgurun wrote: > I see why InitUserPrefService had to move up but is there a reason to move > browser_policy_connector_ creation? It's used in InitUserPrefService(). See lines 328/329 https://codereview.chromium.org/1431473004/diff/60001/android_webview/browser... android_webview/browser/aw_browser_context.cc:304: // Create user pref service On 2015/11/13 08:43:51, sgurun wrote: > nit: s/Create/Initialize/ It's a null pointer before that. Isn't Create appropriate then? https://codereview.chromium.org/1431473004/diff/60001/android_webview/browser... File android_webview/browser/net/aw_url_request_context_getter.cc (right): https://codereview.chromium.org/1431473004/diff/60001/android_webview/browser... android_webview/browser/net/aw_url_request_context_getter.cc:235: builder.add_http_auth_handler_factory( On 2015/11/13 08:43:51, sgurun wrote: > the documentation for add_http_auth_handler_factory says "Build must be called > after this method"? probably an outdated comment but please re-confirm and if > outdated please remove the comment since it is misleading. > I did not see any other user of add_http_auth_handler_factory. We do call Build(), line 240. The comment is right, it add the auth handler to the factory so that this handler is added to the default ones (basic and digest) when the url request context is created. https://codereview.chromium.org/1431473004/diff/60001/android_webview/browser... File android_webview/browser/net/aw_url_request_context_getter.h (right): https://codereview.chromium.org/1431473004/diff/60001/android_webview/browser... android_webview/browser/net/aw_url_request_context_getter.h:74: scoped_ptr<net::HttpAuthHandlerNegotiate::Factory> On 2015/11/13 08:43:51, sgurun wrote: > yeah, how will this work, it is only NET_EXPORT_PRIVATE. Didn't notice that when I wrote the patch, and I got told, NET_EXPORT_PRIVATE and NET_EXPORT are the same for now so I didn't notice. AFAIK, the only other option we have now is to get the default AuthHandlerFactory created by the builder, then throw it away and replace it with a new one that supports negotiate. I'll go with that and once https://codereview.chromium.org/1414313002/ lands, it should be possible to initialize negotiate on android with the URLRequestContext Builder https://codereview.chromium.org/1431473004/diff/60001/android_webview/common/... File android_webview/common/pref_names.h (right): https://codereview.chromium.org/1431473004/diff/60001/android_webview/common/... android_webview/common/pref_names.h:10: // Mirrors select preferences from chrome/common/pref_names.h On 2015/11/13 08:43:51, sgurun wrote: > if this file is just a mirror from preferences in chrome, maybe the chrome > preferences should move down one layer so it can be used by both webview and > chrome. Why do we replicate it? > > Also discounting the mirroring, the prefs are only used at the browser side, so > probably this file should not be in common. They should get moved in https://codereview.chromium.org/1414313002/. I will remove this file when we update webview to use the dynamic policies. Will move to browser for now https://codereview.chromium.org/1431473004/diff/60001/android_webview/common/... android_webview/common/pref_names.h:14: extern const char kAuthAndroidNegotiateAccountType[]; On 2015/11/13 08:43:51, sgurun wrote: > creating this file only for two const char * seems unnecessary. Are you > expecting to add more pref names in future? Depends on whether we need to support more policies for which preferences are defined in the //chrome layer. I personally don't have any plans about that for now.
https://codereview.chromium.org/1431473004/diff/60001/android_webview/browser... File android_webview/browser/aw_browser_context.cc (right): https://codereview.chromium.org/1431473004/diff/60001/android_webview/browser... android_webview/browser/aw_browser_context.cc:186: browser_policy_connector_.reset(new AwBrowserPolicyConnector()); On 2015/11/13 12:37:07, dgn wrote: > On 2015/11/13 08:43:51, sgurun wrote: > > I see why InitUserPrefService had to move up but is there a reason to move > > browser_policy_connector_ creation? > > It's used in InitUserPrefService(). See lines 328/329 k https://codereview.chromium.org/1431473004/diff/60001/android_webview/browser... android_webview/browser/aw_browser_context.cc:304: // Create user pref service On 2015/11/13 12:37:07, dgn wrote: > On 2015/11/13 08:43:51, sgurun wrote: > > nit: s/Create/Initialize/ > > It's a null pointer before that. Isn't Create appropriate then? no strong opinion. I guess actually you are right, create sounds better. https://codereview.chromium.org/1431473004/diff/60001/android_webview/browser... File android_webview/browser/net/aw_url_request_context_getter.cc (right): https://codereview.chromium.org/1431473004/diff/60001/android_webview/browser... android_webview/browser/net/aw_url_request_context_getter.cc:235: builder.add_http_auth_handler_factory( On 2015/11/13 12:37:07, dgn wrote: > On 2015/11/13 08:43:51, sgurun wrote: > > the documentation for add_http_auth_handler_factory says "Build must be called > > after this method"? probably an outdated comment but please re-confirm and if > > outdated please remove the comment since it is misleading. > > I did not see any other user of add_http_auth_handler_factory. > > We do call Build(), line 240. The comment is right, it add the auth handler to > the factory so that this handler is added to the default ones (basic and digest) > when the url request context is created. sure, builder.build() needs to be called for any useful output from builder I think, that is what I was trying to point out. it did not make me sense to state the normal as a comment. actually thinking again, that is really outside the scope. https://codereview.chromium.org/1431473004/diff/60001/android_webview/browser... File android_webview/browser/net/aw_url_request_context_getter.h (right): https://codereview.chromium.org/1431473004/diff/60001/android_webview/browser... android_webview/browser/net/aw_url_request_context_getter.h:74: scoped_ptr<net::HttpAuthHandlerNegotiate::Factory> On 2015/11/13 12:37:07, dgn wrote: > On 2015/11/13 08:43:51, sgurun wrote: > > yeah, how will this work, it is only NET_EXPORT_PRIVATE. > > Didn't notice that when I wrote the patch, and I got told, NET_EXPORT_PRIVATE > and NET_EXPORT are the same for now so I didn't notice. AFAIK, the only other > option we have now is to get the default AuthHandlerFactory created by the > builder, then throw it away and replace it with a new one that supports > negotiate. I'll go with that and once > https://codereview.chromium.org/1414313002/ lands, it should be possible to > initialize negotiate on android with the URLRequestContext Builder the other cl already accumulated a lot of lgtm's so seems close to land? https://codereview.chromium.org/1431473004/diff/60001/android_webview/common/... File android_webview/common/pref_names.h (right): https://codereview.chromium.org/1431473004/diff/60001/android_webview/common/... android_webview/common/pref_names.h:10: // Mirrors select preferences from chrome/common/pref_names.h On 2015/11/13 12:37:07, dgn wrote: > On 2015/11/13 08:43:51, sgurun wrote: > > if this file is just a mirror from preferences in chrome, maybe the chrome > > preferences should move down one layer so it can be used by both webview and > > chrome. Why do we replicate it? > > > > Also discounting the mirroring, the prefs are only used at the browser side, > so > > probably this file should not be in common. > > They should get moved in https://codereview.chromium.org/1414313002/. I will > remove this file when we update webview to use the dynamic policies. Will move > to browser for now ok
On 2015/11/18 02:23:28, sgurun wrote: > https://codereview.chromium.org/1431473004/diff/60001/android_webview/browser... > File android_webview/browser/aw_browser_context.cc (right): > > https://codereview.chromium.org/1431473004/diff/60001/android_webview/browser... > android_webview/browser/aw_browser_context.cc:186: > browser_policy_connector_.reset(new AwBrowserPolicyConnector()); > On 2015/11/13 12:37:07, dgn wrote: > > On 2015/11/13 08:43:51, sgurun wrote: > > > I see why InitUserPrefService had to move up but is there a reason to move > > > browser_policy_connector_ creation? > > > > It's used in InitUserPrefService(). See lines 328/329 > k > > https://codereview.chromium.org/1431473004/diff/60001/android_webview/browser... > android_webview/browser/aw_browser_context.cc:304: // Create user pref service > On 2015/11/13 12:37:07, dgn wrote: > > On 2015/11/13 08:43:51, sgurun wrote: > > > nit: s/Create/Initialize/ > > > > It's a null pointer before that. Isn't Create appropriate then? > > no strong opinion. I guess actually you are right, create sounds better. > > https://codereview.chromium.org/1431473004/diff/60001/android_webview/browser... > File android_webview/browser/net/aw_url_request_context_getter.cc (right): > > https://codereview.chromium.org/1431473004/diff/60001/android_webview/browser... > android_webview/browser/net/aw_url_request_context_getter.cc:235: > builder.add_http_auth_handler_factory( > On 2015/11/13 12:37:07, dgn wrote: > > On 2015/11/13 08:43:51, sgurun wrote: > > > the documentation for add_http_auth_handler_factory says "Build must be > called > > > after this method"? probably an outdated comment but please re-confirm and > if > > > outdated please remove the comment since it is misleading. > > > I did not see any other user of add_http_auth_handler_factory. > > > > We do call Build(), line 240. The comment is right, it add the auth handler to > > the factory so that this handler is added to the default ones (basic and > digest) > > when the url request context is created. > > sure, builder.build() needs to be called for any useful output from builder I > think, that is what I was trying to point out. it did not make me sense to state > the normal as a comment. actually thinking again, that is really outside the > scope. > > https://codereview.chromium.org/1431473004/diff/60001/android_webview/browser... > File android_webview/browser/net/aw_url_request_context_getter.h (right): > > https://codereview.chromium.org/1431473004/diff/60001/android_webview/browser... > android_webview/browser/net/aw_url_request_context_getter.h:74: > scoped_ptr<net::HttpAuthHandlerNegotiate::Factory> > On 2015/11/13 12:37:07, dgn wrote: > > On 2015/11/13 08:43:51, sgurun wrote: > > > yeah, how will this work, it is only NET_EXPORT_PRIVATE. > > > > Didn't notice that when I wrote the patch, and I got told, NET_EXPORT_PRIVATE > > and NET_EXPORT are the same for now so I didn't notice. AFAIK, the only other > > option we have now is to get the default AuthHandlerFactory created by the > > builder, then throw it away and replace it with a new one that supports > > negotiate. I'll go with that and once > > https://codereview.chromium.org/1414313002/ lands, it should be possible to > > initialize negotiate on android with the URLRequestContext Builder > > > the other cl already accumulated a lot of lgtm's so seems close to land? > > https://codereview.chromium.org/1431473004/diff/60001/android_webview/common/... > File android_webview/common/pref_names.h (right): > > https://codereview.chromium.org/1431473004/diff/60001/android_webview/common/... > android_webview/common/pref_names.h:10: // Mirrors select preferences from > chrome/common/pref_names.h > On 2015/11/13 12:37:07, dgn wrote: > > On 2015/11/13 08:43:51, sgurun wrote: > > > if this file is just a mirror from preferences in chrome, maybe the chrome > > > preferences should move down one layer so it can be used by both webview and > > > chrome. Why do we replicate it? > > > > > > Also discounting the mirroring, the prefs are only used at the browser side, > > so > > > probably this file should not be in common. > > > > They should get moved in https://codereview.chromium.org/1414313002/. I will > > remove this file when we update webview to use the dynamic policies. Will move > > to browser for now > > ok ah sorry, I did not really want to mean lgtm (I just used it stupidly in the comments :). please address the other comments too and wait for real lgtm before landing.
On 2015/11/18 17:37:53, sgurun wrote: > On 2015/11/18 02:23:28, sgurun wrote: > > > https://codereview.chromium.org/1431473004/diff/60001/android_webview/browser... > > File android_webview/browser/aw_browser_context.cc (right): > > > > > https://codereview.chromium.org/1431473004/diff/60001/android_webview/browser... > > android_webview/browser/aw_browser_context.cc:186: > > browser_policy_connector_.reset(new AwBrowserPolicyConnector()); > > On 2015/11/13 12:37:07, dgn wrote: > > > On 2015/11/13 08:43:51, sgurun wrote: > > > > I see why InitUserPrefService had to move up but is there a reason to move > > > > browser_policy_connector_ creation? > > > > > > It's used in InitUserPrefService(). See lines 328/329 > > k > > > > > https://codereview.chromium.org/1431473004/diff/60001/android_webview/browser... > > android_webview/browser/aw_browser_context.cc:304: // Create user pref service > > On 2015/11/13 12:37:07, dgn wrote: > > > On 2015/11/13 08:43:51, sgurun wrote: > > > > nit: s/Create/Initialize/ > > > > > > It's a null pointer before that. Isn't Create appropriate then? > > > > no strong opinion. I guess actually you are right, create sounds better. > > > > > https://codereview.chromium.org/1431473004/diff/60001/android_webview/browser... > > File android_webview/browser/net/aw_url_request_context_getter.cc (right): > > > > > https://codereview.chromium.org/1431473004/diff/60001/android_webview/browser... > > android_webview/browser/net/aw_url_request_context_getter.cc:235: > > builder.add_http_auth_handler_factory( > > On 2015/11/13 12:37:07, dgn wrote: > > > On 2015/11/13 08:43:51, sgurun wrote: > > > > the documentation for add_http_auth_handler_factory says "Build must be > > called > > > > after this method"? probably an outdated comment but please re-confirm and > > if > > > > outdated please remove the comment since it is misleading. > > > > I did not see any other user of add_http_auth_handler_factory. > > > > > > We do call Build(), line 240. The comment is right, it add the auth handler > to > > > the factory so that this handler is added to the default ones (basic and > > digest) > > > when the url request context is created. > > > > sure, builder.build() needs to be called for any useful output from builder I > > think, that is what I was trying to point out. it did not make me sense to > state > > the normal as a comment. actually thinking again, that is really outside the > > scope. > > > > > https://codereview.chromium.org/1431473004/diff/60001/android_webview/browser... > > File android_webview/browser/net/aw_url_request_context_getter.h (right): > > > > > https://codereview.chromium.org/1431473004/diff/60001/android_webview/browser... > > android_webview/browser/net/aw_url_request_context_getter.h:74: > > scoped_ptr<net::HttpAuthHandlerNegotiate::Factory> > > On 2015/11/13 12:37:07, dgn wrote: > > > On 2015/11/13 08:43:51, sgurun wrote: > > > > yeah, how will this work, it is only NET_EXPORT_PRIVATE. > > > > > > Didn't notice that when I wrote the patch, and I got told, > NET_EXPORT_PRIVATE > > > and NET_EXPORT are the same for now so I didn't notice. AFAIK, the only > other > > > option we have now is to get the default AuthHandlerFactory created by the > > > builder, then throw it away and replace it with a new one that supports > > > negotiate. I'll go with that and once > > > https://codereview.chromium.org/1414313002/ lands, it should be possible to > > > initialize negotiate on android with the URLRequestContext Builder > > > > > > the other cl already accumulated a lot of lgtm's so seems close to land? > > > > > https://codereview.chromium.org/1431473004/diff/60001/android_webview/common/... > > File android_webview/common/pref_names.h (right): > > > > > https://codereview.chromium.org/1431473004/diff/60001/android_webview/common/... > > android_webview/common/pref_names.h:10: // Mirrors select preferences from > > chrome/common/pref_names.h > > On 2015/11/13 12:37:07, dgn wrote: > > > On 2015/11/13 08:43:51, sgurun wrote: > > > > if this file is just a mirror from preferences in chrome, maybe the chrome > > > > preferences should move down one layer so it can be used by both webview > and > > > > chrome. Why do we replicate it? > > > > > > > > Also discounting the mirroring, the prefs are only used at the browser > side, > > > so > > > > probably this file should not be in common. > > > > > > They should get moved in https://codereview.chromium.org/1414313002/. I will > > > remove this file when we update webview to use the dynamic policies. Will > move > > > to browser for now > > > > ok > > ah sorry, I did not really want to mean lgtm (I just used it stupidly in the > comments :). please address the other comments too and wait for real lgtm before > landing. (You can write "n o t l g t m" to undo a previous l g t m.
https://codereview.chromium.org/1431473004/diff/60001/android_webview/browser... File android_webview/browser/net/aw_url_request_context_getter.cc (right): https://codereview.chromium.org/1431473004/diff/60001/android_webview/browser... android_webview/browser/net/aw_url_request_context_getter.cc:212: CreateCmdLineConfiguredHostResolver()); On 2015/11/13 08:43:51, sgurun wrote: > move creation below, right before where it is used. Done. https://codereview.chromium.org/1431473004/diff/60001/android_webview/browser... android_webview/browser/net/aw_url_request_context_getter.cc:314: DCHECK(resolver); On 2015/11/13 08:43:51, sgurun wrote: > looks like this check is in a weird place. Either move to function prologue or > before where it is first used. Done. https://codereview.chromium.org/1431473004/diff/60001/android_webview/browser... File android_webview/browser/net/aw_url_request_context_getter.h (right): https://codereview.chromium.org/1431473004/diff/60001/android_webview/browser... android_webview/browser/net/aw_url_request_context_getter.h:74: scoped_ptr<net::HttpAuthHandlerNegotiate::Factory> On 2015/11/18 02:23:28, sgurun wrote: > On 2015/11/13 12:37:07, dgn wrote: > > On 2015/11/13 08:43:51, sgurun wrote: > > > yeah, how will this work, it is only NET_EXPORT_PRIVATE. > > > > Didn't notice that when I wrote the patch, and I got told, NET_EXPORT_PRIVATE > > and NET_EXPORT are the same for now so I didn't notice. AFAIK, the only other > > option we have now is to get the default AuthHandlerFactory created by the > > builder, then throw it away and replace it with a new one that supports > > negotiate. I'll go with that and once > > https://codereview.chromium.org/1414313002/ lands, it should be possible to > > initialize negotiate on android with the URLRequestContext Builder > > > the other cl already accumulated a lot of lgtm's so seems close to land? What I mentioned above doesn't seem to work anymore, as the factory seems to be copied to a http session object during the initialization via the builder. I'll see if exporting the classes to configure the builder is doable instead. https://codereview.chromium.org/1431473004/diff/60001/android_webview/common/... File android_webview/common/pref_names.cc (right): https://codereview.chromium.org/1431473004/diff/60001/android_webview/common/... android_webview/common/pref_names.cc:14: "AuthAndroidNegotiateAccountType"; On 2015/11/13 08:43:51, sgurun wrote: > is there a reason to choose a different string than what chrome is using for > pref value? No, I had mixed up pref and policy names. fixed in browser_context https://codereview.chromium.org/1431473004/diff/60001/android_webview/common/... File android_webview/common/pref_names.h (right): https://codereview.chromium.org/1431473004/diff/60001/android_webview/common/... android_webview/common/pref_names.h:10: // Mirrors select preferences from chrome/common/pref_names.h On 2015/11/18 02:23:28, sgurun wrote: > On 2015/11/13 12:37:07, dgn wrote: > > On 2015/11/13 08:43:51, sgurun wrote: > > > if this file is just a mirror from preferences in chrome, maybe the chrome > > > preferences should move down one layer so it can be used by both webview and > > > chrome. Why do we replicate it? > > > > > > Also discounting the mirroring, the prefs are only used at the browser side, > > so > > > probably this file should not be in common. > > > > They should get moved in https://codereview.chromium.org/1414313002/. I will > > remove this file when we update webview to use the dynamic policies. Will move > > to browser for now > > ok In the end this won't move to //net. Moved them in browser_context.h https://codereview.chromium.org/1431473004/diff/60001/android_webview/java/sr... File android_webview/java/src/org/chromium/android_webview/AwBrowserProcess.java (right): https://codereview.chromium.org/1431473004/diff/60001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/AwBrowserProcess.java:70: CombinedPolicyProvider.get().registerProvider(new AwPolicyProvider(context)); On 2015/11/13 08:43:51, sgurun wrote: > please remind me where the user manager reads the application restrictions from. > did you check if it does add any measurable latency? We read them through the app restriction system, where the administrator would set the restrictions on the embedding app. Because this is done asynchronously, we have a cache (in android shared preferences) that we read the policies synchronously from. We then update them once we receive the policies from the app restrictions system. But currently, since those policies can't be updated at runtime, it's just as if we were only reading them from the cache, which should not have much impact. I don't think we have any latency comparison, neither on chrome nor on webview.
PTAL https://codereview.chromium.org/1431473004/diff/60001/android_webview/browser... File android_webview/browser/net/aw_url_request_context_getter.h (right): https://codereview.chromium.org/1431473004/diff/60001/android_webview/browser... android_webview/browser/net/aw_url_request_context_getter.h:74: scoped_ptr<net::HttpAuthHandlerNegotiate::Factory> On 2015/11/25 00:27:15, dgn wrote: > On 2015/11/18 02:23:28, sgurun wrote: > > On 2015/11/13 12:37:07, dgn wrote: > > > On 2015/11/13 08:43:51, sgurun wrote: > > > > yeah, how will this work, it is only NET_EXPORT_PRIVATE. > > > > > > Didn't notice that when I wrote the patch, and I got told, > NET_EXPORT_PRIVATE > > > and NET_EXPORT are the same for now so I didn't notice. AFAIK, the only > other > > > option we have now is to get the default AuthHandlerFactory created by the > > > builder, then throw it away and replace it with a new one that supports > > > negotiate. I'll go with that and once > > > https://codereview.chromium.org/1414313002/ lands, it should be possible to > > > initialize negotiate on android with the URLRequestContext Builder > > > > > > the other cl already accumulated a lot of lgtm's so seems close to land? > > What I mentioned above doesn't seem to work anymore, as the factory seems to be > copied to a http session object during the initialization via the builder. I'll > see if exporting the classes to configure the builder is doable instead. Fixed. I'm not creating a factory registry configured only for negotiate and adding it as extra factory to the builder.
On 2015/11/25 19:01:08, dgn wrote: > PTAL > > https://codereview.chromium.org/1431473004/diff/60001/android_webview/browser... > File android_webview/browser/net/aw_url_request_context_getter.h (right): > > https://codereview.chromium.org/1431473004/diff/60001/android_webview/browser... > android_webview/browser/net/aw_url_request_context_getter.h:74: > scoped_ptr<net::HttpAuthHandlerNegotiate::Factory> > On 2015/11/25 00:27:15, dgn wrote: > > On 2015/11/18 02:23:28, sgurun wrote: > > > On 2015/11/13 12:37:07, dgn wrote: > > > > On 2015/11/13 08:43:51, sgurun wrote: > > > > > yeah, how will this work, it is only NET_EXPORT_PRIVATE. > > > > > > > > Didn't notice that when I wrote the patch, and I got told, > > NET_EXPORT_PRIVATE > > > > and NET_EXPORT are the same for now so I didn't notice. AFAIK, the only > > other > > > > option we have now is to get the default AuthHandlerFactory created by the > > > > builder, then throw it away and replace it with a new one that supports > > > > negotiate. I'll go with that and once > > > > https://codereview.chromium.org/1414313002/ lands, it should be possible > to > > > > initialize negotiate on android with the URLRequestContext Builder > > > > > > > > > the other cl already accumulated a lot of lgtm's so seems close to land? > > > > What I mentioned above doesn't seem to work anymore, as the factory seems to > be > > copied to a http session object during the initialization via the builder. > I'll > > see if exporting the classes to configure the builder is doable instead. > > Fixed. I'm not creating a factory registry configured only for negotiate and > adding it as extra factory to the builder. lgtm what is the testing strategy? you can ping me offline later or we can discuss in the bug.
dgn@chromium.org changed reviewers: + atwilson@chromium.org
atwilson@chromium.org: Please review changes in components/policy/resources/policy_templates.json Testing policy: mostly manual. There are existing tests for the various components used (in //net and //policy), but for the webview integration, it relies for now on manual testing using the tools at https://chromium.googlesource.com/chromium/src/+/master/tools/android/kerberos with this patch: https://codereview.chromium.org/1416443003/ Instrumentation tests are blocked on http://crbug.com/540789
policy_templates.json lgtm
The CQ bit was checked by dgn@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org, sgurun@chromium.org Link to the patchset: https://codereview.chromium.org/1431473004/#ps140001 (title: "Document the new supported policies")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1431473004/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1431473004/140001
Thanks!
Message was sent while issue was closed.
Description was changed from ========== Add Kerberos support to webview Add negotiate handler to WebView's http auth handlers and adds support for the policies (AuthAndroidNegotiateAccountType and AuthServerWhitelist) required to configure Kerberos in WebView BUG=533513 ========== to ========== Add Kerberos support to webview Add negotiate handler to WebView's http auth handlers and adds support for the policies (AuthAndroidNegotiateAccountType and AuthServerWhitelist) required to configure Kerberos in WebView BUG=533513 ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== Add Kerberos support to webview Add negotiate handler to WebView's http auth handlers and adds support for the policies (AuthAndroidNegotiateAccountType and AuthServerWhitelist) required to configure Kerberos in WebView BUG=533513 ========== to ========== Add Kerberos support to webview Add negotiate handler to WebView's http auth handlers and adds support for the policies (AuthAndroidNegotiateAccountType and AuthServerWhitelist) required to configure Kerberos in WebView BUG=533513 Committed: https://crrev.com/4779a19456ede067cb623eb117570e9bf5780239 Cr-Commit-Position: refs/heads/master@{#361878} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/4779a19456ede067cb623eb117570e9bf5780239 Cr-Commit-Position: refs/heads/master@{#361878} |
