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