On Fri, 25 Oct 2024 19:44:54 GMT, Weijun Wang <wei...@openjdk.org> wrote:
> Comments on `java.security` classes.
>
> Also, I'd like to see some clarifications on what "the installed policy" or
> "the current policy" is. The `ProtectionDomain` mentions this when talking
> about dynamic permissions. On the other hand, the `Policy` class suggests
> there is no such a thing. If we do not have this concept no more, some
> modifications might be needed in `ProtectionDomain`.
Yes, good point, let me review the class again and see if we can remove some
more text or make some adjustments.
> src/java.base/share/classes/java/security/AccessControlContext.java line 32:
>
>> 30:
>> 31: /**
>> 32: * AccessControlContext was used with a SecurityManager for access
>> control decisions
>
> I'm not sure how you use this name elsewhere. To me, one either uses
> "Security Manager" as the name for the technique or `SecurityManager` (inside
> `{@code}`) as the name for the class.
Right, it should be "the Security Manager", but we can also link to the
`SecurityManager` class as plain text. I'll make some wording changes to this
class and `AccessController`.
> src/java.base/share/classes/java/security/AccessControlContext.java line 141:
>
>> 139: throws AccessControlException
>> 140: {
>> 141: throw new AccessControlException("");
>
> No message for this exception?
I'm not sure what would be a useful message. All the `SecurityManager` check
methods throw a `SecurityException` with no message. We had to specify
something here because `AccessControlException` doesn't have a no-args ctor.
> src/java.base/share/classes/java/security/AccessControlException.java line 29:
>
>> 27:
>> 28: /**
>> 29: *
>
> Add a sentence like "This was..."?
You mean move the first sentence of the deprecated text to here?
> src/java.base/share/classes/java/security/Policy.java line 90:
>
>> 88: * and subject to removal in a future release. Consequently, this
>> class
>> 89: * is also deprecated and subject to removal. There is no
>> replacement for
>> 90: * the Security Manager or this class.
>
> Don't you need at least one sentence as the body of the class spec here?
> Something like `Policy was...` which is similar to `AccessController`.
Yes, we should be consistent. The deprecated text always comes first, and
sometimes we want to say immediately why this class is not useful anymore in
that text instead of later, further down. I don't know which is better. I guess
I'll go with your suggestion just so it looks more like a normal class.
> src/java.base/share/classes/java/security/Policy.java line 374:
>
>> 372: *
>> 373: * @param codesource the CodeSource to which the returned
>> 374: * PermissionCollection has been granted
>
> Can we say this parameter is ignored? I see some other methods said so.
>
> Same with the other `getPermissions` and `implies`.
Yes, good suggestion.
> The class spec still mentions "permissions which are retrieved by the system
> policy by default". Shall we remove it?
Yes I think we can remove that text.
> Also, getPermissions always returns an empty Permissions object, do we need
> to add an @apiNote for it?
You mean a warning like we have in the `Permission` subclasses?
`URLClassLoader` and other subclasses still populate these permissions, but the
plan is to revisit that code and potentially remove it later. I will remove
"granted to" in the `@return` text.
> src/java.base/share/classes/java/security/Security.java line 489:
>
>> 487:
>> 488: /**
>> 489: * Adds a provider to the next position available..
>
> Two periods at the end.
Will fix.
-------------
PR Comment: https://git.openjdk.org/jdk/pull/21498#issuecomment-2438893608
PR Review Comment: https://git.openjdk.org/jdk/pull/21498#discussion_r1817327704
PR Review Comment: https://git.openjdk.org/jdk/pull/21498#discussion_r1817335403
PR Review Comment: https://git.openjdk.org/jdk/pull/21498#discussion_r1817344307
PR Review Comment: https://git.openjdk.org/jdk/pull/21498#discussion_r1817362628
PR Review Comment: https://git.openjdk.org/jdk/pull/21498#discussion_r1817348142
PR Review Comment: https://git.openjdk.org/jdk/pull/21498#discussion_r1817377926
PR Review Comment: https://git.openjdk.org/jdk/pull/21498#discussion_r1817314475
