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

[Weijun Wang]

On Wed, 18 Aug 2021 15:01:12 GMT, Sean Mullan <mullan 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.
>
> 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