New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 705699 link

Starred by 2 users

Issue metadata

Status: Verified
Owner:
Closed: Apr 2017
Cc:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 1
Type: Bug



Sign in to add a comment

Chrome OS SAML: test if it works.

Project Member Reported by alemate@chromium.org, Mar 27 2017

Issue description

I suspect that https://codereview.chromium.org/2779463002/ actually broke SAML (in 58). We need to test that.
 
Cc: xiy...@chromium.org achuith@chromium.org

Comment 2 by xiy...@chromium.org, 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
Cc: sergeyu@chromium.org raymes@chromium.org
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.

Comment 4 by xiy...@chromium.org, 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. :( 
Owner: emaxx@chromium.org
Status: Assigned (was: Untriaged)
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.

Comment 6 by emaxx@chromium.org, 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.
Status: Verified (was: Assigned)
Thanks emaxx. That is all I wanted to figure out (because none of us has the test credentials).
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