On Wed, 18 Aug 2021 15:01:12 GMT, Sean Mullan <mul...@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.
>
> src/java.base/share/classes/javax/security/auth/Subject.java line 353:
>
>> 351:
>> 352: /**
>> 353: * Return the current installed subject.
>
> Nit. I know other accessor methods in Subject use the word "Return", but I
> think "Returns" is more grammatically correct and reads better when
> describing what this method does. Also just say "Returns the current subject."
I'll use "returns".
> src/java.base/share/classes/javax/security/auth/Subject.java line 355:
>
>> 353: * Return the current installed subject.
>> 354: * <p>
>> 355: * The current installed subject is installed by the {@link #call}
>
> s/The current installed subject/The current subject/
>
> I am not sure "installed" is the best term to use, as I equate that most
> commonly with installing software on your disk. Maybe "set" or "registered"?
I thought about "associated" but cannot precisely describe what it it is
associated to. The "install" word actually comes from "install a
SecurityManager with the `System::setSecurityManager` method".
I'll use "current object".
> src/java.base/share/classes/javax/security/auth/Subject.java line 357:
>
>> 355: * The current installed subject is installed by the {@link #call}
>> 356: * or {@link #run} method. When either {@code call(subject,
>> action)} or
>> 357: * {@code run(subject, action)} is called, {@code action} is
>> executed
>
> Should this be `callAs` or `doAs`?
Correct, it should be `callAs` and `getAs`.
> src/java.base/share/classes/javax/security/auth/Subject.java line 359:
>
>> 357: * {@code run(subject, action)} is called, {@code action} is
>> executed
>> 358: * with {@code subject} as its current installed subject which can
>> be
>> 359: * retrieved by this method. After {@code action} is finished, the
>> current
>
> Just say "current subject" instead of "current installed subject" throughout.
> Once you explain how the current subject is installed (or registered), there
> is no need to repeat that term.
I see what you mean, it's only "current subject", but we can use a verb to
describe how it's set.
> src/java.base/share/classes/javax/security/auth/Subject.java line 361:
>
>> 359: * retrieved by this method. After {@code action} is finished, the
>> current
>> 360: * installed subject is reset to its previous value. The current
>> installed
>> 361: * subject is {@code null} before the first call of {@code call()}
>
> It is also null if there are no active `call` or `doAs` methods, right?
Yes, is this implied by "reset to its previous value"?
> src/java.base/share/classes/javax/security/auth/Subject.java line 369:
>
>> 367: *
>> 368: * @implNote
>> 369: * In this implementation, if a {@code SecurityManager} is allowed,
>
> We should probably link "allowed" to the section in the `SecurityManager` API
> that describes the behavior. Perhaps we could see if we could add a hyperlink.
Precisely, this "allowed" covers "-Djava.security.manager" set to "allow", "",
and a class name. I should probably be more precise.
> src/java.base/share/classes/javax/security/auth/Subject.java line 373:
>
>> 371: * {@code getSubject(AccessController.getContext())}. Otherwise,
>> the current
>> 372: * installed subject is stored in an inheritable {@code
>> ThreadLocal}.
>> 373: * Also, each of the various {@code doAs(subject, action)} and
>
> It's not clear if the "Also ..." sentence is only for the non-SM case. I
> think you could just use one sentence here, replace "Also, each" with "and
> each ...".
Oops, I think I made a mistake here. No matter if there's SM, `current` and
`getSubject` returns the same thing. I should have said "the current subject"
is stored inside "current AccessControlContext" if SM is on or a `ThreadLocal`
is not. I'll rephrase.
-------------
PR: https://git.openjdk.java.net/jdk/pull/5024
