On Wed, 17 Jan 2024 23:41:53 GMT, Weijun Wang <wei...@openjdk.org> wrote:
> This code change adds an alternative implementation of user-based
> authorization `Subject` APIs that doesn't depend on Security Manager APIs.
> Depending on if the Security Manager is allowed, the methods store the
> current subject differently. See the spec change in the `Subject.java` file
> for details. When the Security Manager APIs are finally removed in a future
> release, this new implementation will be only implementation for these
> methods.
>
> One major change in the new implementation is that `Subject.getSubject`
> always throws an `UnsupportedOperationException` since it has an
> `AccessControlContext` argument but the current subject is no longer
> associated with an `AccessControlContext` object.
>
> Now it's the time to migrate from the `getSubject` and `doAs` methods to
> `current` and `callAs`. If the user application is simply calling
> `getSubject(AccessController.getContext())`, then switching to `current()`
> would work. If the `AccessControlContext` argument is retrieved from an
> earlier `getContext()` call and the associated subject might be different
> from that of the current `AccessControlContext`, then instead of storing the
> previous `AccessControlContext` object and passing it into `getSubject` to
> get the "previous" subject, the application should store the `current()`
> return value directly.
src/java.base/share/classes/javax/security/auth/Subject.java line 112:
> 110: *
> 111: * <p><b><a id="sm-not-allowed">These methods behave differently
> depending on
> 112: * whether a security manager is allowed or disallowed</a></b>:
"is allowed or disallowed" but the detail presents them in the reverse order. I
think it would be easier to read if the allowed case went first, this goes for
all the methods. I understand that disallow is the default but I think a bit
easier to present in that order.
src/java.base/share/classes/javax/security/auth/Subject.java line 298:
> 296: * {@code AccessControlContext}. When a security manager is
> 297: * <a href=#sm-not-allowed>not allowed</a>, this method is not
> supported
> 298: * and throws an {@code UnsupportedOperationException}.
I think it might be better to say "This method is intended to be used with a
security manager. It throws UOE if a security manager is not allowed".
src/java.base/share/classes/javax/security/auth/Subject.java line 379:
> 377: * current subject binds to the period of the execution of the
> current
> 378: * thread. Otherwise, it is associated with the current
> 379: * {@code AccessControlContext}.
What would you think of "If a security manager is allowed, this method is
equivalent to calling getSubject with the current ACC. If a security manager is
not allowed, this returns the Subject bound to the current Thread."
src/java.base/share/classes/javax/security/auth/Subject.java line 411:
> 409: * Finally, this method invokes {@code
> AccessController.doPrivileged},
> 410: * passing it the provided {@code PrivilegedAction},
> 411: * as well as the newly constructed {@code AccessControlContext}.
I think this method would be easier to present if the allowed and not-allowed
cases were in separate paragraphs. The reason is that the SM allowed case has a
lot more test. For the not allowed case then it would be clearer to say that
the calls the value returning function with the current Thread bound to the
given Subject.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/17472#discussion_r1467824941
PR Review Comment: https://git.openjdk.org/jdk/pull/17472#discussion_r1467826752
PR Review Comment: https://git.openjdk.org/jdk/pull/17472#discussion_r1467829318
PR Review Comment: https://git.openjdk.org/jdk/pull/17472#discussion_r1467831325
