On Tue, 11 Jun 2024 16:51:11 GMT, Weijun Wang <wei...@openjdk.org> wrote:
>> Kevin Walls has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Sean comments
>
> src/java.management.rmi/share/classes/javax/management/remote/rmi/RMIConnectionImpl.java
> line 1436:
>
>> 1434: } else {
>> 1435: // ACC is present, we have a Subject and SM is
>> permitted:
>> 1436: return
>> AccessController.doPrivileged((PrivilegedExceptionAction<Object>) () ->
>> Subject.doAs(subject, op), acc);
>
> Why is it necessary to call both `doAs` and `doPrivileged`?
Can you just call `AccessController.doPrivileged(op, acc)` as in the original
code? `Subject.doAs` requires a permission, which is why I assume you are
wrapping it in a `doPrivileged` but you are asserting all of the permissions of
the `java.management` module, which means you probably want to use limited
doPriv and only assert the `getSubject` permission but that is getting
complicated. I'd go back to the original call as it should work in the SM allow
case.
> src/java.management/share/classes/com/sun/jmx/remote/internal/ServerNotifForwarder.java
> line 350:
>
>> 348: @SuppressWarnings("removal")
>> 349: private Subject getSubject() {
>> 350: Subject subject = null;
>
> Just call `Subject.current()`. When SM is allowed, it is equivalent to
> `getSubject(AccessController.getContext())`.
For the same reason you should be able to just call `Subject.current` in the
tests. I don't think you need the `SimpleStandard.useGetSubjectACC` property to
toggle the testing of either old or replacement APIs as long as the test has
runs in both SM allow and disallow (i.e., no SM prop set) modes. Would you
agree Weijun?
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/19624#discussion_r1635475145
PR Review Comment: https://git.openjdk.org/jdk/pull/19624#discussion_r1635449088
