Implement fallback if class `java.security.AccessController` does not exist about gson HOT 5 CLOSED

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.

from gson.

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.

from gson.

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.

from gson.

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.

from gson.

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?

from gson.