Chrome OS SAML: test if it works. |
||||
Issue descriptionI suspect that https://codereview.chromium.org/2779463002/ actually broke SAML (in 58). We need to test that.
,
Mar 27 2017
That is, we need to verify the SAML flow that uses webcam still works. Other SAML should be fine. The refactor CL (https://codereview.chromium.org/2696703006) might be the one introdueced the regression. It moves the code from WebUILoginView::RequestMediaAccessPermission to MediaPermission::GetPermissionStatus. However, it added WebContent instance check [1]. This could break the media request from the WebView hosted in the login screen. The WebContents for such request carries the guest WebContents instead of the host [2]. [1]: https://cs.chromium.org/chromium/src/chrome/browser/media/webrtc/media_permission.cc?rcl=625dec950fc94b1678646d4d5ef19b76c49c96db&l=66 [2]: https://cs.chromium.org/chromium/src/extensions/browser/guest_view/web_view/web_view_permission_helper.cc?rcl=625dec950fc94b1678646d4d5ef19b76c49c96db&l=253
,
Mar 27 2017
I apologize for the sloppy reviewing on my part. I ought to have ascertained that this didn't actually break SAML. I was relying on browser test coverage which has proved insufficient.
,
Mar 27 2017
It seems that I read the code wrong. :p The WebContents instance check is fine because code in link [2] calls with the embedder, which is the login screen WebContents. So we should be good on this. It would still be good to test the code path though. As said, we don't have test with fake the media devices to cover the code path. :(
,
Apr 3 2017
Assigning to emaxx to triage accordingly. We should have more tests for this feature. I ought to mention that prior to landing that CL I knew that coverage of this code wasn't great and asked emaxx to manually test it, which he kindly did (thanks again). I think the problem here - and it's reflected by the comment in code - is that no one knew that this code was used outside the SAML webview flow. That case was never reflected in tests nor in documentation and when I was asking around about what the code was used for, it never came up.
,
Apr 4 2017
Didn't manage to perform the testing on M59, as all builds with the fresh enough version of Chrome seem to be broken, as displayed by GoldenEye. But I just did the manual testing on M58 build which should include the CL that was mentioned in comment 0: Chrome: 58.0.3029.51 Chrome OS: 9334.32.0 board: link And Chrome OS SAML login using webcam (using the test credentials for clever.com) worked successfully for me. So is there anything else that needs to be tested manually? Otherwise we should probably file a separate bug for tracking the addition of the browser test for SAML webcam scenario.
,
Apr 4 2017
Thanks emaxx. That is all I wanted to figure out (because none of us has the test credentials).
,
Apr 4 2017
Thanks emaxx! By the way, I'm adding an end-to-end test for webcam usage in the ChromeOS login page here: https://codereview.chromium.org/2746873004/ That doesn't fully cover the SAML case which comes from a webview inside the login page, but coupled with the SAML unittests I added it gets us reasonable coverage. |
||||
►
Sign in to add a comment |
||||
Comment 1 by alemate@chromium.org
, Mar 27 2017