On Sat, 23 Oct 2021 00:40:39 GMT, Weijun Wang <wei...@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
