Skip to content
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

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

Open
ddorwin opened this issue Oct 19, 2016 · 13 comments

Comments

Projects
None yet
4 participants
@ddorwin
Copy link
Contributor

commented Oct 19, 2016

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

This comment has been minimized.

Copy link
Contributor

commented Oct 19, 2016

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

This comment has been minimized.

Copy link
Contributor Author

commented Oct 19, 2016

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

This comment has been minimized.

Copy link
Contributor

commented Oct 21, 2016

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

This comment has been minimized.

Copy link
Contributor Author

commented Oct 22, 2016

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

This comment has been minimized.

Copy link

commented Oct 25, 2016

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

This comment has been minimized.

Copy link
Contributor

commented Oct 25, 2016

@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

This comment has been minimized.

Copy link

commented Oct 25, 2016

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

This comment has been minimized.

Copy link
Contributor

commented Oct 25, 2016

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

This comment has been minimized.

Copy link
Contributor Author

commented Oct 25, 2016

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

This comment has been minimized.

Copy link
Contributor

commented Oct 25, 2016

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

This comment has been minimized.

Copy link
Contributor

commented Oct 25, 2016

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
#4027 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AHPfiy6GvXJM0twgjROVAwNSZGau9jNMks5q3myVgaJpZM4KbbWb
.

@ddorwin

This comment has been minimized.

Copy link
Contributor Author

commented Oct 27, 2016

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

This comment has been minimized.

Copy link
Contributor

commented Oct 31, 2016

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.

@jdsmith3000 jdsmith3000 added this to the V1 milestone Oct 31, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.