Implement fallback if class java.security.AccessController does not exist

[Marcono1234]

Problem solved by the feature

JEP 411 marked the SecurityManager and related classes such as java.security.AccessController as 'deprecated for removal' since JDK 17.

Currently the Gson code uses AccessController:

gson/gson/src/main/java/com/google/gson/internal/bind/TypeAdapters.java

Line 969 in 91d22b3

AccessController.doPrivileged(

This could become a problem when a future JDK version actually removes the SecurityManager and related classes, and Gson users cannot upgrade then.

Side note: Gson's unit test ReflectionAccessTest.testRestrictiveSecurityManager() also uses the deprecated SecurityManager class, but maybe we should keep this for now to make sure Gson's behavior for JDK versions with SecurityManager is correct.
Once we want to build with JDK versions without SecurityManager we can investigate solutions for this, e.g. moving the test into a separate test class which is excluded when building with newer JDK versions.

Feature description

We should already consider solutions for the case that AccessController does not exist, to avoid any issues if users want to upgrade to future JDK versions.

One solution might be to wrap the usage of AccessController inside a try-catch which catches NoClassDefFoundError and in that case falls back to executing the code to access the fields outside the scope of AccessController#doPrivileged. (To be verified that this actually works as desired.)

Another alternative might be to refactor the code to first check with Class#forName or similar if AccessController exists (and remember the result).

It might also be good to add a unit test which verifies that the AccessController#doPrivileged usage actually has the desired effect. It seems that is currently not covered by any test (?).
Then we could be more confident that whatever change we make does not break the code for users who have a restrictive SecurityManager on an older JDK.

[eamonnmcmanus]

It's going to be difficult to be sure about workarounds for a missing AccessController until we have a JDK that has a missing AccessController.

I'm a bit puzzled about the code in question. Aren't enum constants public? So then we should be able just to use Class.getFields() to read them? Technically that can still throw SecurityException:

If a security manager, s, is present and the caller's class loader is not the same as or an ancestor of the class loader for the current class and invocation of s.checkPackageAccess() denies access to the package of this class.

I am inclined to think that that is going to affect hardly anybody. I'm also not sure we need to be able to call Field.get, since the name of the constant is the name of the field.

[Marcono1234]

It's going to be difficult to be sure about workarounds for a missing AccessController until we have a JDK that has a missing AccessController.

I understood JEP 411 as if the idea was to remove SecurityManager and related classes without adding any replacement. But you are right, it is not clear what exactly will happen; it seems there is no follow-up JEP yet.

---

I'm a bit puzzled about the code in question.

Coincidentally you were the one who suggested using AccessController#doPrivileged in #1495 (comment) 😅, but for the setAccessible call.
I then later expanded it to only have one AccessController#doPrivileged call for all fields instead of one per field.

---

I'm also not sure we need to be able to call Field.get, since the name of the constant is the name of the field.

Unfortunately not when ProGuard or R8 are involved. If I remember correctly, name() still returns the expected value, but the field names might be obfuscated. And the enum values() method to obtain all constants (which is used by Class.getEnumConstants()) might have been removed by the code shrinker (at least #1147 mentions that).

---

Another option would also be to simply remove the AccessController#doPrivileged call. There are only a few issues and pull requests related to SecurityManager (#566, #1361, #1712), and maybe nowadays most users don't use a SecurityManager.
But maybe we can wait with that until it becomes clearer which approach the JDK maintainers will take regarding removal of SecurityManager.

[eamonnmcmanus]

I didn't express my point about the JDK very well. What I meant was that if we add a workaround that is supposed to function when there is no AccessController class, we can't really test it until we have a JDK that really does have no AccessController class.

Thanks for reminding me of the previous discussion, where indeed I noted that enum constants are public but enums themselves aren't necessarily. Still, the whole thing feels very complicated and I'm not sure it is worthwhile now. I would be somewhat inclined to rip it all out and see if anybody complains. If they do, we can put it back again in another release and perhaps also look at handling a missing AccessController.

[Marcono1234]

It seems usage of AccessController actually makes a difference. Without it the following test testJdkEnum_SecurityManager fails with AccessControlException: access denied ("java.lang.RuntimePermission" "accessDeclaredMembers").

(Though I am not completely sure if this is a proper SecurityManager / Policy test setup.)

@Test
public void testJdkEnum() {
  assertThat(gson.toJson(Thread.State.NEW)).isEqualTo("\"NEW\"");
  assertThat(gson.fromJson("\"NEW\"", Thread.State.class)).isEqualTo(Thread.State.NEW);
}

@SuppressWarnings("removal") // java.lang.SecurityManager deprecation in Java 17
@Test
public void testJdkEnum_SecurityManager() {
  // Skip for newer Java versions where `System.setSecurityManager` is unsupported
  assumeTrue(Runtime.version().feature() <= 17);

  var gsonProtectionDomain = Gson.class.getProtectionDomain();
  var testProtectionDomain = getClass().getProtectionDomain();
  assertWithMessage(
          "Gson code and test code should have different ProtectionDomain; otherwise test might"
              + " not work properly")
      .that(gsonProtectionDomain)
      .isNotEqualTo(testProtectionDomain);

  Policy originalPolicy = Policy.getPolicy();
  SecurityManager originalSecurityManager = System.getSecurityManager();
  try {
    Policy.setPolicy(
        new Policy() {
          @Override
          public boolean implies(ProtectionDomain domain, Permission permission) {
            // Allow removing the SecurityManager again
            if (permission instanceof RuntimePermission
                && permission.getName().equals("setSecurityManager")) {
              return true;
            }
            if (domain.equals(gsonProtectionDomain)) {
              return true;
            }
            return originalPolicy.implies(domain, permission);
          }
        });
    System.setSecurityManager(new SecurityManager());

    assertThat(gson.toJson(Thread.State.NEW)).isEqualTo("\"NEW\"");
    assertThat(gson.fromJson("\"NEW\"", Thread.State.class)).isEqualTo(Thread.State.NEW);
  } finally {
    System.setSecurityManager(originalSecurityManager);
    Policy.setPolicy(originalPolicy);
  }
}

However, it seems there are a few other places which fail when a custom SecurityManager is used:

• com.google.gson.reflect.TypeToken.isCapturingTypeVariablesForbidden()
  Due to it's usage of System#getProperty
• Reflection-based adapter
  Probably not that surprising, and should probably fail to prevent accessing implementation details of third-party classes. But it even fails for Record classes.
• Getting no-args constructor for Collection and Map adapter
• com.google.gson.internal.$Gson$Types.equals(Type, Type)
  This calls TypeVariable.getGenericDeclaration() and apparently the implementation sun.reflect.generics.reflectiveObjects.TypeVariableImpl performs undocumented access checks. (Or my test setup is wrong?)

So unless my test setup is wrong or flawed this suggests that usage with a SecurityManager was already cumbersome, or was not working in some cases. So maybe removing the AccessController usage might be fine since most users don't rely on it?

[eamonnmcmanus]

Yes, the more I think about it, the more I agree with my earlier comment about just ripping out the AccessController usage. :-)
It may be that there are people who are depending on it and who are keeping up to date with Gson versions but not JDK versions. If so we can learn about their use cases when they complain after the next release, and then we can decide what to do. And if there is in fact nobody depending on the AccessController then the code will be that much simpler without it.