On Wed, 6 Aug 2025 15:37:41 GMT, Artur Barashev <abaras...@openjdk.org> wrote:
>> Valerie Peng has updated the pull request incrementally with two additional
>> commits since the last revision:
>>
>> - Add a public static final constant for the property name.
>> - Address more review comments from Sean and Artur.
>
> src/java.base/share/classes/sun/security/util/CryptoAlgorithmConstraints.java
> line 62:
>
>> 60: }
>> 61:
>> 62: public static boolean permits(String service, String algo) {
>
> I think in other places of our code we don't separate the service and the
> algo in 2 strings, those are being used as a single string. So this method's
> signature should be `public static boolean permits(String algo)` for
> consistency.
I don't want the caller classes to have to do the `service` +"." + `algo`
String concatenation. It's cleaner to provide 2 arguments. Given this
`permits(...)` method is already very different from the other `permits(...)`
methods in the super interface, I don't think it really matters. Or, I can
rename the method to something like `isAllowed` if you prefer a different
method name.
> src/java.base/share/classes/sun/security/util/CryptoAlgorithmConstraints.java
> line 98:
>
>> 96: service.equalsIgnoreCase("KeyStore") ||
>> 97: service.equalsIgnoreCase("MessageDigest") ||
>> 98: service.equalsIgnoreCase("Signature")) {
>
> It will be a cleaner solution if we define a set of allowed services as a
> static class variable (all in upper/lower case) and then do
> `SET.contains(service.toUpper/toLower)`. We may also need such set of allowed
> services for other operations (like `*` wildcard substitution).
Sure, I didn't bother since there are only 4 services. But I can do that now
also.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/26377#discussion_r2257916632
PR Review Comment: https://git.openjdk.org/jdk/pull/26377#discussion_r2257921194
