web-platform-tests / wpt

Test suites for Web platform specs — including WHATWG, W3C, and others
https://web-platform-tests.org/
Other
4.91k stars 3.06k forks source link

[eme] drm-temporary-license-type.html does not request correct license type #4027

Open ddorwin opened 7 years ago

ddorwin commented 7 years ago

https://www.w3c-test.org/encrypted-media/drm-temporary-license-type.html is a negative test that is supposed to provide a persistent license for a "temporary" session. For Widevine, the license that is obtained is not really persistent, so update() succeeds rather than failing as expected. It appears that the storeLicense property in the request (drm-messagehandler.js) is not resulting in the expected license.

As an aside, this test and the JS should be updated to make it clearer what this test does.

mwatson2 commented 7 years ago

License generated with storeLicense set to true are accepted by persistent-license sessions on ChromeOS, though, and are persisted at least long enough to be retrieved and used in a different window. So, in what sense are they not really persistent ?

ddorwin commented 7 years ago

I don't know what the DrmToday server is doing, but I would guess that there is additional logic that is evaluating information in the license request that is different on Chrome OS. storeLicense probably needs to override that logic too.

mwatson2 commented 7 years ago

The issue with this test seems to be that for some DRMs (at least Widevine), the type of the returned license is constrained by the server to match the type of the session (based on something in the license challenge). For these DRMs the server will never generate a persistent license for us to supply to a temporary session and there is no reason to make it do so just to test that it fails.

An alternative version of the test would be to request a persistent license and then test that either update() fails, or if it succeeds, the resulting keys are not persisted (which we can check by closing the session, opening a new one and trying either to play or to load the old session).

For the moment we should not include this test in the test results.

ddorwin commented 7 years ago

This seems like a reasonable approach to covering the intent of the related spec text.

In the meantime, should we comment out inclusion of the testharness sources to prevent this test from appearing in the results?

marado commented 7 years ago

The issue with this test seems to be that for some DRMs (at least Widevine), the type of the returned license is constrained by the server to match the type of the session (based on something in the license challenge). For these DRMs the server will never generate a persistent license for us to supply to a temporary session and there is no reason to make it do so just to test that it fails.

From what I read of this comment, you're saying that the server seems to have a bug (according to the spec), as it will not generate a license. While you say that "there is no reason to make it do so", I have to disagree: the test seems to actually test something that is on the spec, the spec seems to define a behavior as it is wanted, and the way to test browser compliance is to actually test it. If Widevine doesn't comply with EME in this regard is surely out of the scope of this work group, but should not be used as an excuse not to test a valid scenario of the spec.

An alternative version of the test would be to request a persistent license and then test that either update() fails, or if it succeeds, the resulting keys are not persisted (which we can check by closing the session, opening a new one and trying either to play or to load the old session).

Which sounds like another valid test, but not a replacement to this one.

For the moment we should not include this test in the test results.

If the test is valid and correct, I don't see why should it be removed. The purpose of having tests is exactly to get issues, like this one, solved, right?

mwatson2 commented 7 years ago

@marado In the Widevine case, the type of the session (indicated in the license challenge) overrides the request outside the challenge for a persistent license. The server does generate a license in this case and it is a temporary one, matching the session type.

If any change were to be made to the server, it would be to reject this request since the session types inside and outside the challenge do not match.

If the test is valid and correct, I don't see why should it be removed. The purpose of having tests is exactly to get issues, like this one, solved, right?

The test as written is not valid for Widevine, because the Widevine server does not support providing a license which does not match the session type. It returns one which does match instead. The EME specification does not constrain the server. So there is nothing to test here for Widevine. The failure case described in the specification does not exist for this DRM.

marado commented 7 years ago

In the Widevine case, the type of the session (indicated in the license challenge) overrides the request outside the challenge for a persistent license. The server does generate a license in this case and it is a temporary one, matching the session type.

I'm sorry if my comments are off track, but I would like to understand this.

According to the spec, for generateRequest, a requested type is given (persistent-license, in this case), and, being a correctly formed request, the spec states:

Let message be a license request for the requested license type generated based on the sanitized init data interpreted per initDataType.

Since the spec does not take the session type into account here, it seems to me that it is saying that, independently of whether the session is persistent or temporary, if the request is for a persistent license (or any other particular type), the expected result is a persistent license (or whatever type is stated in the request).

If any change were to be made to the server, it would be to reject this request since the session types inside and outside the challenge do not match.

I don't see how would that be compliant with the expected behavior according to the spec, either. If that's the behavior that /should/ be expected (or the current), then I think that changes would be need, not on the server but on the spec.

The test as written is not valid for Widevine

As I see them, standards tests aren't written to be be valid for a particular implementation, they're written to test the validity of implementations. If the test is using EME in a possible way (as it is), and there is an expected outcome (which, I believe, there is), then the test is correct, and if an implementation fails the test, then the implementation is wrong.

mwatson2 commented 7 years ago

Let message be a license request for the requested license type generated based on the sanitized init data interpreted per initDataType.

There is no separate concept of license type in the specification, so what this means is the license type appropriate for the session type. Put another way, session type is all that is available at the client EME implementation to determine the license type.

if the request is for a persistent license (or any other particular type), the expected result is a persistent license (or whatever type is stated in the request).

and

I don't see how would that be compliant with the expected behavior according to the spec

We are not writing a specification for the server and the server is not the subject of our tests.

As I see them, standards tests aren't written to be be valid for a particular implementation, they're written to test the validity of implementations. If the test is using EME in a possible way (as it is), and there is an expected outcome (which, I believe, there is)

As written, the test is reliant on a particular server behavior. That behavior is neither defined in our specification nor implemented in Widevine. And it would not make sense to implement it either (failing this ambiguous request would be preferable).

So, there is not an expected outcome according to the specification.

ddorwin commented 7 years ago

The purpose of the step this test covers is for the client to reject responses from the server that do not match the session type. If the server or protocol do not allow such invalid responses, it doesn't make sense to force them to support such responses just so that we can test that the client will reject them.

@mwatson2's proposal would check that either it is not possible to get such a response (at least as far as we can detect from side effects) or the client rejects the response.

jdsmith3000 commented 7 years ago

I agree with @ddorwin's stated purpose of the test (to confirm EME client behavior). The license server behavior is perhaps unexpected, but seems an acceptable response to the conflict between license request and session type.

@mwatson2: Is the suggestion to check the returned license before verifying whether it can be used or not workable?

mwatson2 commented 7 years ago

On Tue, Oct 25, 2016 at 1:54 PM, jdsmith3000 notifications@github.com wrote:

I agree with @ddorwin https://github.com/ddorwin's stated purpose of the test (to confirm EME client behavior). The license server behavior is perhaps unexpected, but seems an acceptable response to the conflict between license request and session type.

@mwatson2 https://github.com/mwatson2: Is the suggestion to check the returned license before verifying whether it can be used or not workable?

​There's no way to check the returned license directly. The idea is to check it by using it. If the server returns a temporary license the client should accept it and then we should not be able to retrieve it with load() in a later session. If the server returns a persistent license, the client should not accept it.

So, the failure case is where the server returns a persistent license, the client accepts it into a temporary session and we find we can retrieve or use it later.

...Mark

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/w3c/web-platform-tests/issues/4027#issuecomment-256172737, or mute the thread https://github.com/notifications/unsubscribe-auth/AHPfiy6GvXJM0twgjROVAwNSZGau9jNMks5q3myVgaJpZM4KbbWb .

ddorwin commented 7 years ago

The selected configuration currently used for this test doesn't allow persistetState, so an implementation would need to violate the spec in that way as well to store the license. (This is something we should cover in a separate test.)

Thus, we should request persistentState on clients that support it, falling back to not requiring it so the test runs (as Mark has proposed) on all clients. I propose the following set of configurations (each in addition to the current single configuration) in order:

  1. persistentState: true, sessionTypes: [ "persistent-license", "temporary" ]
  2. persistentState: true
  3. (nothing additional)

This would cause the client to select a configuration that seems most likely to succeed in requesting a persistent license, which should then be rejected.

jdsmith3000 commented 7 years ago

Do we have folks making test changes still? I'm wondering if this test case should be turned off since it's not supported on most CDMs, and by none in our currently active test population.