New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

XWALK-2697 #3126

Merged
merged 1 commit into from Jul 29, 2015

Conversation

Projects
None yet
7 participants
@ZhengXinCN
Copy link
Contributor

ZhengXinCN commented Jul 7, 2015

Support for client-side SSL certificate for Android

Bug=XWALK-2697

@crosswalk-trybot

This comment has been minimized.

Copy link

crosswalk-trybot commented Jul 7, 2015

Testing patch series with ZhengXinCN/crosswalk@ced66da as its head.

Bot Status
Crosswalk Android-X86 [FAILED 💔](https://build.crosswalk-project.org/try/builders/Crosswalk Android-X86/builds/3437)
Crosswalk Tizen 3 Common [FAILED 💔](https://build.crosswalk-project.org/try/builders/Crosswalk Tizen 3 Common/builds/3025)
Crosswalk Android x86-64 [FAILED 💔](https://build.crosswalk-project.org/try/builders/Crosswalk Android x86-64/builds/484)
Crosswalk Linux [FAILED 💔](https://build.crosswalk-project.org/try/builders/Crosswalk Linux/builds/3436)
@crosswalk-trybot

This comment has been minimized.

Copy link

crosswalk-trybot commented Jul 7, 2015

Testing patch series with ZhengXinCN/crosswalk@eff0d80 as its head.

Bot Status
Crosswalk Android-X86 [FAILED 💔](https://build.crosswalk-project.org/try/builders/Crosswalk Android-X86/builds/3438)
Crosswalk Tizen 3 Common [FAILED 💔](https://build.crosswalk-project.org/try/builders/Crosswalk Tizen 3 Common/builds/3026)
Crosswalk Android x86-64 [FAILED 💔](https://build.crosswalk-project.org/try/builders/Crosswalk Android x86-64/builds/485)
Crosswalk Linux [FAILED 💔](https://build.crosswalk-project.org/try/builders/Crosswalk Linux/builds/3437)
@crosswalk-trybot

This comment has been minimized.

Copy link

crosswalk-trybot commented Jul 7, 2015

Testing patch series with ZhengXinCN/crosswalk@aaf9564 as its head.

Bot Status
Crosswalk Android-X86 [SUCCESS 💚](https://build.crosswalk-project.org/try/builders/Crosswalk Android-X86/builds/3439)
Crosswalk Tizen 3 Common [FAILED 💔](https://build.crosswalk-project.org/try/builders/Crosswalk Tizen 3 Common/builds/3027)
Crosswalk Android x86-64 [SUCCESS 💚](https://build.crosswalk-project.org/try/builders/Crosswalk Android x86-64/builds/486)
Crosswalk Linux [FAILED 💔](https://build.crosswalk-project.org/try/builders/Crosswalk Linux/builds/3438)
@crosswalk-trybot

This comment has been minimized.

Copy link

crosswalk-trybot commented Jul 7, 2015

Testing patch series with ZhengXinCN/crosswalk@66e8841 as its head.

Bot Status
Crosswalk Tizen 3 Common [SUCCESS 💚](https://build.crosswalk-project.org/try/builders/Crosswalk Tizen 3 Common/builds/3028)
Crosswalk Linux [SUCCESS 💚](https://build.crosswalk-project.org/try/builders/Crosswalk Linux/builds/3439)
Crosswalk Android-X86 [SUCCESS 💚](https://build.crosswalk-project.org/try/builders/Crosswalk Android-X86/builds/3440)
Crosswalk Android x86-64 [SUCCESS 💚](https://build.crosswalk-project.org/try/builders/Crosswalk Android x86-64/builds/487)
@darktears

This comment has been minimized.

Copy link
Contributor

darktears commented Jul 7, 2015

Please squash all into one commit.

@ZhengXinCN ZhengXinCN force-pushed the ZhengXinCN:master branch from 66e8841 to 9f95c8c Jul 8, 2015

@crosswalk-trybot

This comment has been minimized.

Copy link

crosswalk-trybot commented Jul 8, 2015

Testing patch series with ZhengXinCN/crosswalk@9f95c8c as its head.

Bot Status
Crosswalk Android-X86 [SUCCESS 💚](https://build.crosswalk-project.org/try/builders/Crosswalk Android-X86/builds/3441)
Crosswalk Tizen 3 Common [SUCCESS 💚](https://build.crosswalk-project.org/try/builders/Crosswalk Tizen 3 Common/builds/3029)
Crosswalk Android x86-64 [SUCCESS 💚](https://build.crosswalk-project.org/try/builders/Crosswalk Android x86-64/builds/488)
Crosswalk Linux [SUCCESS 💚](https://build.crosswalk-project.org/try/builders/Crosswalk Linux/builds/3440)
@wuhengzhi

This comment has been minimized.

Copy link
Contributor

wuhengzhi commented Jul 8, 2015

Test case is also needed for this feature. Thanks a lot for you patch. @ZhengXinCN

@ZhengXinCN

This comment has been minimized.

Copy link
Contributor

ZhengXinCN commented Jul 9, 2015

Does anyone have any idea to do a unit test without a server requests Client Cert?

  1. TestWebServer seems don't support request a client cert.

@ZhengXinCN ZhengXinCN force-pushed the ZhengXinCN:master branch from 9f95c8c to 2fcf21b Jul 10, 2015

@crosswalk-trybot

This comment has been minimized.

Copy link

crosswalk-trybot commented Jul 10, 2015

Testing patch series with ZhengXinCN/crosswalk@2fcf21b as its head.

Bot Status
Crosswalk Android-X86 [FAILED 💔](https://build.crosswalk-project.org/try/builders/Crosswalk Android-X86/builds/3443)
Crosswalk Tizen 3 Common [SUCCESS 💚](https://build.crosswalk-project.org/try/builders/Crosswalk Tizen 3 Common/builds/3031)
Crosswalk Android x86-64 [FAILED 💔](https://build.crosswalk-project.org/try/builders/Crosswalk Android x86-64/builds/490)
Crosswalk Linux [SUCCESS 💚](https://build.crosswalk-project.org/try/builders/Crosswalk Linux/builds/3442)

@ZhengXinCN ZhengXinCN force-pushed the ZhengXinCN:master branch from 2fcf21b to e1bf871 Jul 10, 2015

@crosswalk-trybot

This comment has been minimized.

Copy link

crosswalk-trybot commented Jul 10, 2015

Testing patch series with ZhengXinCN/crosswalk@e1bf871 as its head.

Bot Status
Crosswalk Android-X86 [SUCCESS 💚](https://build.crosswalk-project.org/try/builders/Crosswalk Android-X86/builds/3444)
Crosswalk Linux [SUCCESS 💚](https://build.crosswalk-project.org/try/builders/Crosswalk Linux/builds/3443)
Crosswalk Android x86-64 [SUCCESS 💚](https://build.crosswalk-project.org/try/builders/Crosswalk Android x86-64/builds/491)
Crosswalk Tizen 3 Common [SUCCESS 💚](https://build.crosswalk-project.org/try/builders/Crosswalk Tizen 3 Common/builds/3032)
@ZhengXinCN

This comment has been minimized.

Copy link
Contributor

ZhengXinCN commented Jul 10, 2015

@wuhengzhi I added a simple test case that connects to this URL https://egov.privasphere.com/. I actually don't own this server, so not sure if this is a good idea. However, we need proper SSL certificates on this test server where we attempt to connect to and this one looks fine.

I am uncertain if the test was executed during "Testing patch series" and if it passed?

@wuhengzhi

This comment has been minimized.

Copy link
Contributor

wuhengzhi commented Jul 12, 2015

  1. Please move the test case from XWalkCoreInternalTest to XWalkCoreTest, since the related function was overridden from XWalkResourceClient instead of XWalkResourceClientInternal.
  2. Please enrich the commit log, e.g some code was ported from upstream; Add title for commit log: [Android] Support for client side SSL certificate.
  3. For test, please install XWalkCoreShell.apk, then execute ./build/android/test_runner.py instrumentation --release --test-apk=XWalkCoreTest
@wuhengzhi

This comment has been minimized.

Copy link
Contributor

wuhengzhi commented Jul 13, 2015

@ZhengXinCN From http://developer.android.com/reference/android/webkit/WebViewClient.html#onReceivedClientCertRequest(android.webkit.WebView, android.webkit.ClientCertRequest), we can see onReceivedClientCertRequest has only two paramters.
I think we should supply the similar API to user. e.g. For XWalkResourceClientInternal, onReceivedClientCertRequest(XWalkViewInternal view, ClientCertRequestHandlerInternal handler).
For XWalkResourceClient, onReceivedClientCertRequest(XWalkView view, ClientCertRequestHandler handler).
This PR will also resolve XWALK-3667.

@ZhengXinCN ZhengXinCN force-pushed the ZhengXinCN:master branch from e1bf871 to 526e125 Jul 15, 2015

@crosswalk-trybot

This comment has been minimized.

Copy link

crosswalk-trybot commented Jul 15, 2015

Testing patch series with ZhengXinCN/crosswalk@526e125 as its head.

Bot Status
Crosswalk Android x86-64 [SUCCESS 💚](https://build.crosswalk-project.org/try/builders/Crosswalk Android x86-64/builds/501)
Crosswalk Linux [SUCCESS 💚](https://build.crosswalk-project.org/try/builders/Crosswalk Linux/builds/3453)

@ZhengXinCN ZhengXinCN force-pushed the ZhengXinCN:master branch from 526e125 to 52373a9 Jul 16, 2015

@crosswalk-trybot

This comment has been minimized.

Copy link

crosswalk-trybot commented Jul 16, 2015

Testing patch series with ZhengXinCN/crosswalk@52373a9 as its head.

Bot Status
Crosswalk Tizen 3 Common [SUCCESS 💚](https://build.crosswalk-project.org/try/builders/Crosswalk Tizen 3 Common/builds/3046)
Crosswalk Android-X86 [SUCCESS 💚](https://build.crosswalk-project.org/try/builders/Crosswalk Android-X86/builds/3458)
Crosswalk Android x86-64 [SUCCESS 💚](https://build.crosswalk-project.org/try/builders/Crosswalk Android x86-64/builds/505)
Crosswalk Linux [SUCCESS 💚](https://build.crosswalk-project.org/try/builders/Crosswalk Linux/builds/3457)
@ZhengXinCN

This comment has been minimized.

Copy link
Contributor

ZhengXinCN commented Jul 16, 2015

@wuhengzhi OK, adapted as requested, can you please review once more?

@wuhengzhi

This comment has been minimized.

Copy link
Contributor

wuhengzhi commented Jul 16, 2015

I will review it today, thanks.

@wuhengzhi

This comment has been minimized.

Copy link
Contributor

wuhengzhi commented Jul 17, 2015

Please add BUG=XWALK-2697, XWALK-3667 in comment.

@ZhengXinCN

This comment has been minimized.

Copy link
Contributor

ZhengXinCN commented Jul 17, 2015

@wuhengzhi Thanks, I will fix them later on, and another question, do you have any code style formatter setting file for Eclipse of this?

@wuhengzhi

This comment has been minimized.

Copy link
Contributor

wuhengzhi commented Jul 17, 2015

Sorry, I have no format file.

@ZhengXinCN ZhengXinCN force-pushed the ZhengXinCN:master branch from 52373a9 to 7c9a77d Jul 20, 2015

@crosswalk-trybot

This comment has been minimized.

Copy link

crosswalk-trybot commented Jul 20, 2015

Testing patch series with ZhengXinCN/crosswalk@7c9a77d as its head.

Bot Status
Crosswalk Tizen 3 Common [SUCCESS 💚](https://build.crosswalk-project.org/try/builders/Crosswalk Tizen 3 Common/builds/3054)
Crosswalk Android-X86 [SUCCESS 💚](https://build.crosswalk-project.org/try/builders/Crosswalk Android-X86/builds/3466)
Crosswalk Android x86-64 [SUCCESS 💚](https://build.crosswalk-project.org/try/builders/Crosswalk Android x86-64/builds/513)
Crosswalk Linux [SUCCESS 💚](https://build.crosswalk-project.org/try/builders/Crosswalk Linux/builds/3465)
@ZhengXinCN

This comment has been minimized.

Copy link
Contributor

ZhengXinCN commented Jul 20, 2015

@wuhengzhi can you please review once more? when you have time. thanks

@wuhengzhi

This comment has been minimized.

Copy link
Contributor

wuhengzhi commented Jul 20, 2015

In the comment, wrer is were?

@ahoward

This comment has been minimized.

Copy link

ahoward commented Jul 20, 2015

+1000

@ZhengXinCN ZhengXinCN force-pushed the ZhengXinCN:master branch from 7c9a77d to ab5dfe2 Jul 21, 2015

@crosswalk-trybot

This comment has been minimized.

Copy link

crosswalk-trybot commented Jul 21, 2015

Testing patch series with ZhengXinCN/crosswalk@ab5dfe2 as its head.

Bot Status
Crosswalk Tizen 3 Common [SUCCESS 💚](https://build.crosswalk-project.org/try/builders/Crosswalk Tizen 3 Common/builds/3058)
Crosswalk Android x86-64 [SUCCESS 💚](https://build.crosswalk-project.org/try/builders/Crosswalk Android x86-64/builds/517)
Crosswalk Android-X86 [SUCCESS 💚](https://build.crosswalk-project.org/try/builders/Crosswalk Android-X86/builds/3470)
Crosswalk Linux [SUCCESS 💚](https://build.crosswalk-project.org/try/builders/Crosswalk Linux/builds/3469)
@wuhengzhi

This comment has been minimized.

Copy link
Contributor

wuhengzhi commented Jul 21, 2015

Thanks @ZhengXinCN, did you run test case by ./build/android/test_runner.py instrumentation --release --test-apk=XWalkCoreTest.
@axinging @lincsoon Please help to review, thanks.

@ZhengXinCN

This comment has been minimized.

Copy link
Contributor

ZhengXinCN commented Jul 21, 2015

@wuhengzhi yes, I did run the test after I changed API, and my test case went pass. and I just test again seems no problem:

W 0.077s Main org.xwalk.core.xwview.test.OnCreateWindowRequestedTest#testOnCreateWindowRequested has no annotations. Assuming "SmallTest".
W 0.212s Main Creating 1 test runners.
W 0.213s bfe8 Creating shard 0 for device 4790f03ef70fbfe8.
W 2.115s bfe8 Unable to enable java asserts for 4790f03ef70fbfe8, non rooted device
W 4.880s Main Running tests with 1 test runners.
W 194.828s bfe8 Will retry test org.xwalk.core.xwview.test.ShouldOverrideUrlLoadingTest#testCalledOnJavaScriptLocationDelayedReplaceRedirect, try #1.
W 469.244s bfe8 Test size not found in annotations for test 'org.xwalk.core.xwview.test.OnCreateWindowRequestedTest#testOnCreateWindowRequested', using 1 minute for timeout.
C 477.678s Main ********************************************************************************
C 477.678s Main Summary
C 477.678s Main ********************************************************************************
C 477.679s Main [==========] 124 tests ran.
C 477.679s Main [ PASSED ] 124 tests.
C 477.679s Main ********************************************************************************

* @param handler An instance of a ClientCertRequestHandlerInternal
*/
@XWalkAPI
public void onReceivedClientCertRequest(XWalkViewInternal view,

This comment has been minimized.

@axinging

axinging Jul 21, 2015

Contributor

@lincsoon , this is API 5.0 or 6.0?

@axinging

This comment has been minimized.

Copy link
Contributor

axinging commented Jul 22, 2015

I will give LGTM. Thanks for Zhengxin's contribution.
Please help to add one line in the new API XWalkResourceClientInternal.onReceivedClientCertRequest's comments:

@ZhengXinCN ZhengXinCN force-pushed the ZhengXinCN:master branch from ab5dfe2 to 6b93d73 Jul 22, 2015

@crosswalk-trybot

This comment has been minimized.

Copy link

crosswalk-trybot commented Jul 22, 2015

Testing patch series with ZhengXinCN/crosswalk@6b93d73 as its head.

Bot Status
Crosswalk Tizen 3 Common [SUCCESS 💚](https://build.crosswalk-project.org/try/builders/Crosswalk Tizen 3 Common/builds/3062)
Crosswalk Android-X86 [SUCCESS 💚](https://build.crosswalk-project.org/try/builders/Crosswalk Android-X86/builds/3474)
Crosswalk Android x86-64 [SUCCESS 💚](https://build.crosswalk-project.org/try/builders/Crosswalk Android x86-64/builds/521)
Crosswalk Linux [SUCCESS 💚](https://build.crosswalk-project.org/try/builders/Crosswalk Linux/builds/3473)
@sunlin-link

This comment has been minimized.

Copy link
Contributor

sunlin-link commented Jul 23, 2015

Why expose ClientCertRequestHandlerInternal to external? I couldn't see the need to extend ClientCertRequestHandler or any interface to handle a ClientCertRequestHandler object.

@sunlin-link

This comment has been minimized.

Copy link
Contributor

sunlin-link commented Jul 23, 2015

LGTM except the matter I mentioned above. Thanks for @ZhengXinCN 's contribution too!

@wuhengzhi

This comment has been minimized.

Copy link
Contributor

wuhengzhi commented Jul 23, 2015

public void onReceivedClientCertRequest(XWalkView view, ClientCertRequest handler) will be overriden by user, so ClientCertRequestHandlerInternal need to expose to external. Please look at http://developer.android.com/reference/android/webkit/WebViewClient.html#onReceivedClientCertRequest(android.webkit.WebView, android.webkit.ClientCertRequest).

@sunlin-link

This comment has been minimized.

Copy link
Contributor

sunlin-link commented Jul 23, 2015

ClientCertRequest or ClientCertRequestHandler?

@wuhengzhi

This comment has been minimized.

Copy link
Contributor

wuhengzhi commented Jul 24, 2015

Oh, ClientCertRequestHandlerInternal is the internal implementation, need not to expose to external.

@ZhengXinCN

This comment has been minimized.

Copy link
Contributor

ZhengXinCN commented Jul 24, 2015

seems I should expose android.webkit.ClientCertRequest.

@sunlin-link

This comment has been minimized.

Copy link
Contributor

sunlin-link commented Jul 24, 2015

I think it's okay to add the interface org.xwalk.core.ClientCertRequest. Just remove ClientCertRequestHandler from reflection generator if you don't want the developers to use it directly.

@ZhengXinCN

This comment has been minimized.

Copy link
Contributor

ZhengXinCN commented Jul 24, 2015

@lincsoon
if I removed "ClientCertRequestHandlerInternal" from generator, the generated class "XWalkResourceClientBridge.java" will failed to build (it wants [classname]Bridge.java).

User needs to override "onReceivedClientCertRequest" from "XWalkResourceClient" to handle client SSL request. Any idea to have a handler pass out without reflection generator?

because what I need actually exactly the same as "public boolean onJavascriptModalDialog" in "XWalkUIClientInternal"

@sunlin-link

This comment has been minimized.

Copy link
Contributor

sunlin-link commented Jul 28, 2015

Sorry, I didn't notice that ClientCertRequest is an interface. Then you can keep ClientCertRequestHandler in the reflection generator.

But one more thing, you need put ClientCertRequest in the java docs so that the developer can know it. Please check the target xwalk_core_library_documentation in xwalk_core_library_android.gypi and add ClientCertRequest to the variable api_files. Then file the description of ClientCertRequestInternal just like XWalkJavascriptResultInternal, except the version is since 6.0.

@wuhengzhi

This comment has been minimized.

Copy link
Contributor

wuhengzhi commented Jul 28, 2015

ClientCertRequestInternal could be declared as abstract class, please take wuhengzhi@fc8e7f2 for a reference.

@sunlin-link

This comment has been minimized.

Copy link
Contributor

sunlin-link commented Jul 28, 2015

Yes, you can change ClientCertRequestInternal to abstract class like XWalkDownloadListener, or keep current implementation like XWalkJavascriptResult. Because we don't have special provisions here, both styles are okay.

[Android] Support for client side SSL certificate.
- some classes and code were ported from upstream.
- test case uses a public server from Internet.

BUG=XWALK-2697, XWALK-3667

@ZhengXinCN ZhengXinCN force-pushed the ZhengXinCN:master branch from 6b93d73 to 203a117 Jul 28, 2015

@crosswalk-trybot

This comment has been minimized.

Copy link

crosswalk-trybot commented Jul 28, 2015

Testing patch series with ZhengXinCN/crosswalk@203a117 as its head.

Bot Status
Crosswalk Tizen 3 Common [SUCCESS 💚](https://build.crosswalk-project.org/try/builders/Crosswalk Tizen 3 Common/builds/3080)
Crosswalk Android-X86 [SUCCESS 💚](https://build.crosswalk-project.org/try/builders/Crosswalk Android-X86/builds/3492)
Crosswalk Android x86-64 [SUCCESS 💚](https://build.crosswalk-project.org/try/builders/Crosswalk Android x86-64/builds/539)
Crosswalk Linux [SUCCESS 💚](https://build.crosswalk-project.org/try/builders/Crosswalk Linux/builds/3491)
@ZhengXinCN

This comment has been minimized.

Copy link
Contributor

ZhengXinCN commented Jul 28, 2015

@lincsoon I added it.
@wuhengzhi I prefer to use interface.

sunlin-link added a commit that referenced this pull request Jul 29, 2015

@sunlin-link sunlin-link merged commit 0e30056 into crosswalk-project:master Jul 29, 2015

1 check passed

default All bots are green
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment