RFR: 8267108: Alternate Subject.getSubject and doAs APIs that do not depend on Security Manager APIs [v2]

[Sean Mullan]

On Sat, 23 Oct 2021 00:40:39 GMT, Weijun Wang <weijun at openjdk.org> wrote:

>> New `Subject` APIs `current()` and `callAs()` are created to be replacements of `getSubject()` and `doAs()` since the latter two methods are now deprecated for removal.
>> 
>> In this implementation, by default, `current()` returns the same value as `getSubject(AccessController.getCurrent())` and `callAs()` is implemented based on `doAs()`. This behavior is subject to change in the future once `SecurityManager` is removed.
>> 
>> User can experiment a possible future mechanism by setting the system property `jdk.security.auth.subject.useTL` to `true`, where the `callAs()` method stores the subject into a `ThreadLocal` object and the `current()` method returns it (Note: this mechanism does not work with principal-based permissions).
>> 
>> Inside JDK, we’ve switched from `getSubject()` to `current()` in JGSS and user can start switching to `callAs()` in their applications. Users can also switch to `current()` but please note that if you used to call `getSubject(acc)` in a `doPrivileged` call you might need to try calling `current()` in a `doPrivilegedWithCombiner` call to see if the `AccessControlContext` inside the call inherits the subject from the outer one.
>
> Weijun Wang has updated the pull request incrementally with one additional commit since the last revision:
> 
>   renames

src/java.base/share/classes/javax/security/auth/Subject.java line 296:

> 294:      *       which is equivalent to
> 295:      *       {@code Subject.getSubject(AccessController.getContext())}
> 296:      *       by default in this implementation.

I don't think you need the words "by default".

src/java.base/share/classes/javax/security/auth/Subject.java line 324:

> 322:     }
> 323: 
> 324:     // Store the current subject to a ThreadLocal when a system property is set.

nit: "... in a ThreadLocal ..."

src/java.base/share/classes/javax/security/auth/Subject.java line 357:

> 355:      * is set to {@code true}, it will be retrieved from an inheritable
> 356:      * {@code ThreadLocal} object. This behavior is subject to change in a
> 357:      * future version.

Suggest rewording this to: "By default, this method returns the same value as {@code Subject.getSubject(AccessController.getContext())}. This preserves compatibility with code that may still be calling {@code doAs} which installs the subject in an {@code AccessControlContext}. However, if the system property {@systemProperty jdk.security.auth.subject.useTL} is set to {@code true}, the subject is retrieved from an inheritable {@code ThreadLocal} object. This implementation behavior is subject to change in a future version."

src/java.base/share/classes/javax/security/auth/Subject.java line 360:

> 358:      * <p>
> 359:      * No matter what storage is chosen, the current subject will
> 360:      * always be installed by the {@link #callAs} method.

I'm not really sure if this sentence is necessary. It seems like it doesn't need to be in the specification to me. The first sentence is clear enough to me: "The current subject is installed by the {@link #callAs} method."

src/java.base/share/classes/javax/security/auth/Subject.java line 362:

> 360:      * always be installed by the {@link #callAs} method.
> 361:      *
> 362:      * @return the current subject. The return value can be

Suggest to combine this into one sentence: the current subject, or {@code null} if a current subject is not installed or the current subject is set to {@code null}."

src/java.base/share/classes/javax/security/auth/Subject.java line 381:

> 379:      * @implNote
> 380:      * By default, this method calls {@link #doAs(Subject, PrivilegedExceptionAction)
> 381:      * Subject.doAs(subject, altAction)} to store the subject into

s/to store/which stores/
s/into/in/

src/java.base/share/classes/javax/security/auth/Subject.java line 386:

> 384:      * modified to match the specification of this method.
> 385:      * If the system property {@code jdk.security.auth.subject.useTL}
> 386:      * is set to {@code true}, it will be stored in an inheritable

s/it/the subject/

src/java.base/share/classes/javax/security/auth/Subject.java line 387:

> 385:      * If the system property {@code jdk.security.auth.subject.useTL}
> 386:      * is set to {@code true}, it will be stored in an inheritable
> 387:      * {@code ThreadLocal} object. The behavior is subject to change in a

s/The/This/

src/java.base/share/classes/javax/security/auth/Subject.java line 391:

> 389:      * <p>
> 390:      * No matter what storage is chosen, the current subject will
> 391:      * always be retrievable by the {@link #current} method.

Same comment as above.

src/java.base/share/classes/javax/security/auth/Subject.java line 394:

> 392:      *
> 393:      * @param subject the intended current subject for {@code action}.
> 394:      *                Can be {@code null}.

Is it necessary to allow `null` in the new `callAs` method? Is there a use case where this is useful? I know the `doAs` methods allowed it, but it looks like they simply call `AccessController.doPrivileged` in that case w/o a subject. Seems it would be simpler to not allow it and throw an NPE.

src/java.base/share/classes/javax/security/auth/Subject.java line 400:

> 398:      *            of {@code action}
> 399:      * @return the value returned by the {@code call} method of {@code action}
> 400:      * @throws NullPointerException if the {@code Callable} is {@code null}.

s/if the {@code Callable}/if {@code action}/
remove '.'

src/java.base/share/classes/javax/security/auth/Subject.java line 403:

> 401:      * @throws CompletionException if {@code action.call()} throws an
> 402:      *          exception, which will be the cause of this
> 403:      *          {@code CompletionException}.

Suggest to break this into two sentences: "if {@code action.call()} throws an exception. The cause of the           {@code CompletionException} is set to the exception thrown by {@code action.call()}."

test/jdk/javax/security/auth/Subject/CurrentSubject.java line 63:

> 61: 
> 62:     /**
> 63:      * Ensure the current subject is the expected Subject objet.

typo: objet -> object

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

PR: https://git.openjdk.java.net/jdk/pull/5024