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