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

[Sean Mullan]

On Thu, 5 Aug 2021 20:10:44 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.

Max, I am thinking now that we don't need to make the new `Subject.callAs` method call `Subject.doAs` if an SM is not disallowed, and instead just use the TL solution. However, we still want `Subject.current` to support both the SM and TL case as that would be typically called by library code. My reasoning is that applications that are calling `doAs` and depending on the SM will need to find another solution anyway, or stay on the older releases. We probably don't want them to transition to the new `callAs` method (which is not deprecated) and then suddenly it stops working when the SM is removed.

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

> 332: 
> 333:     private static enum SubjectStorage {
> 334:         ACC, THREAD_LOCAL, SCOPE_LOCAL

I would leave out SCOPE_LOCAL for now.

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

> 350:             };
> 351: 
> 352:     /**

Some initial comments below. I think this will take a few iterations to refine the text.

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."

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"?

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`?

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.

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?

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.

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 ...".

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

> 480:      * @since 18
> 481:      */
> 482:     public static <T> T getAs(final Subject subject,

Would prefer if we just have one `callAs` method. It might be less of an issue now if we don't need to support the SM case.

src/java.base/share/classes/jdk/internal/access/JavaLangAccess.java line 406:

> 404: 
> 405:     /**
> 406:      * When a security manager is allowed. This is true if the system

Suggest rewording as: "Returns true if a security manager is allowed. This method returns true ..."

src/java.base/share/classes/jdk/internal/access/JavaLangAccess.java line 407:

> 405:     /**
> 406:      * When a security manager is allowed. This is true if the system
> 407:      * property java.security.manager is set to any value other than "disabled".

s/disabled/disallow/

src/java.base/share/classes/jdk/internal/access/JavaLangAccess.java line 408:

> 406:      * When a security manager is allowed. This is true if the system
> 407:      * property java.security.manager is set to any value other than "disabled".
> 408:      * @return

Probably don't need `@return` as it is an internal method.

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

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