RFR: 8296244: Alternate implementation of user-based authorization Subject APIs that doesn’t depend on Security Manager APIs

[Alan Bateman]

On Wed, 17 Jan 2024 23:41:53 GMT, Weijun Wang <weijun at 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