On Thu, 25 Aug 2022 15:25:57 GMT, Weijun Wang <wei...@openjdk.org> wrote:

>> Valerie Peng has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   update test to use SHA256 and DSA throughout.
>
> src/java.base/share/classes/java/security/Security.java line 530:
> 
>> 528:             // <crypto_service>.<algo_or_type> <attr_name>:<attr_value>
>> 529:             key = filter.substring(0, index);
>> 530:             value = filter.substring(index + 1);
> 
> Do you need to ensure `value` is non-empty here? I am worried that somewhere 
> later a `string.indexOf("")` is called and it's always true.

Makes sense, I will add a check. Thanks!

> src/java.base/share/classes/java/security/Security.java line 931:
> 
>> 929:                     // check individual component for match and bail if 
>> no match
>> 930:                     if (prop.indexOf(st.nextToken()) == -1) {
>> 931:                         return false;
> 
> So if `value` has several sub-values, all of them must appear in the `prop` 
> value. Do we need to make this clear in the spec?
> 
> Also, you use `indexOf` instead of an exact match to a sub-value in `prop`. 
> Is this always correct? I am wondering if a value can be substring of a 
> different value. I see you support simple class name in the test. Is it worth 
> we doing this? I would rather be strict at the beginning.

Yes, I'd expect if multiple sub-values are specified, it means all of them 
should appear in `prop` in order to be matched. It does raise an interesting 
question as to how to do the filtering based on an "OR" relationship. Perhaps 
an union of 2 separate filter result? I'd suspect that "AND" relationship would 
be more useful. One alternative is to disallow multiple sub-values, and treat 
the value as one sub-value. Thoughts?

As for strict vs loose, I am on the fence, thought that it'd be nice to not 
having to enter the entire value. Could switch direction to be strict for now 
and loosen it up if requested.

-------------

PR: https://git.openjdk.org/jdk/pull/10008

Reply via email to