diff --git a/AGENTS.md b/AGENTS.md index 86afc988..c4cfef9d 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -36,6 +36,16 @@ gh -R minekube/connect-java release view --json tagName,targetCommitis curl -I -L --fail https://github.com/minekube/connect-java/releases/download//connect-velocity.jar ``` +## Injector Scoping (config availability) + +- The parent injector binds `ConfigHolder`; `ConnectPlatform.init()` populates it + before `ConfigLoadedModule` binds `ConnectConfig` in the child injector. +- Anything reachable while constructing the parent/platform injector must avoid + injecting `ConnectConfig` directly. Depend on the parent-bound `ConfigHolder` + and read `configHolder.get()` lazily after initialization instead. +- The Bedrock identity graph follows this pattern; see + `core/.../module/BedrockParentInjectorStartupTest` for the regression guard. + ## Velocity Join Bugs - For Velocity proxy issues, test both `CONFIGURATION` and `PLAY` state packet @@ -57,3 +67,10 @@ Run targeted module tests for packet/session fixes, then the broader build: Do not call production fixed from a Connect Java release alone. Confirm the released jar is in the hub image, the hub pod logs the expected plugin version, and Moxy accepts a tunnel with the same `connectorVersion`. + +## Maintaining this file + +Keep this file for knowledge useful to almost every future agent session in this project. +Do not repeat what the codebase already shows; point to the authoritative file or command instead. +Prefer rewriting or pruning existing entries over appending new ones. +When updating this file, preserve this bar for all agents and keep entries concise. diff --git a/core/src/main/java/com/minekube/connect/bedrock/BedrockIdentityEnforcer.java b/core/src/main/java/com/minekube/connect/bedrock/BedrockIdentityEnforcer.java index 35e92934..354b71b8 100644 --- a/core/src/main/java/com/minekube/connect/bedrock/BedrockIdentityEnforcer.java +++ b/core/src/main/java/com/minekube/connect/bedrock/BedrockIdentityEnforcer.java @@ -10,6 +10,7 @@ import com.minekube.connect.api.player.bedrock.BedrockIdentityReplayCache; import com.minekube.connect.api.player.bedrock.BedrockIdentityVerificationException; import com.minekube.connect.api.player.bedrock.BedrockIdentityVerifier; +import com.minekube.connect.config.ConfigHolder; import com.minekube.connect.config.ConnectConfig; import com.minekube.connect.config.ConnectConfig.BedrockIdentityConfig; import com.minekube.connect.network.netty.LocalSession; @@ -28,14 +29,28 @@ public final class BedrockIdentityEnforcer { private static final String PROTOCOL_BEDROCK = "bedrock"; private static final String REJECT_MESSAGE = "Bedrock identity verification failed"; - private final ConnectConfig config; + private final Supplier config; private final ConnectLogger logger; private final Supplier now; private final BedrockIdentityKeyProvider keyProvider; private final BedrockAdmissionCoordinator admissionCoordinator; private final BedrockIdentityReplayCache replayCache = new BedrockIdentityReplayCache(); + /** + * Injection seam used by the plugin. The config is resolved lazily from {@link ConfigHolder} so + * this enforcer can be constructed by the (config-agnostic) parent injector while the platform + * injector is built, before {@code ConnectPlatform.init()} loads the configuration. + */ @Inject + public BedrockIdentityEnforcer( + ConfigHolder configHolder, + ConnectLogger logger, + BedrockIdentityKeyProvider keyProvider, + BedrockAdmissionCoordinator admissionCoordinator) { + this(Objects.requireNonNull(configHolder, "configHolder")::get, logger, Instant::now, + keyProvider, admissionCoordinator); + } + public BedrockIdentityEnforcer( ConnectConfig config, ConnectLogger logger, @@ -89,6 +104,15 @@ public BedrockIdentityEnforcer( Supplier now, BedrockIdentityKeyProvider keyProvider, BedrockAdmissionCoordinator admissionCoordinator) { + this(constant(config), logger, now, keyProvider, admissionCoordinator); + } + + private BedrockIdentityEnforcer( + Supplier config, + ConnectLogger logger, + Supplier now, + BedrockIdentityKeyProvider keyProvider, + BedrockAdmissionCoordinator admissionCoordinator) { this.config = Objects.requireNonNull(config, "config"); this.logger = Objects.requireNonNull(logger, "logger"); this.now = Objects.requireNonNull(now, "now"); @@ -96,6 +120,15 @@ public BedrockIdentityEnforcer( this.admissionCoordinator = admissionCoordinator; } + private static Supplier constant(ConnectConfig config) { + Objects.requireNonNull(config, "config"); + return () -> config; + } + + private ConnectConfig config() { + return Objects.requireNonNull(config.get(), "config"); + } + BedrockIdentityEnforcer( ConnectConfig config, ConnectLogger logger, @@ -180,7 +213,7 @@ Decision verify( SessionProtocol protocol) { Objects.requireNonNull(player, "player"); Objects.requireNonNull(protocol, "protocol"); - BedrockIdentityConfig bedrockIdentity = config.getBedrockIdentity(); + BedrockIdentityConfig bedrockIdentity = config().getBedrockIdentity(); BedrockIdentityConfiguration identityConfiguration = BedrockIdentityConfiguration.from(bedrockIdentity); if (identityConfiguration.isDisabled()) { return Decision.allowed(null); @@ -280,7 +313,7 @@ private BedrockIdentityVerifier verifier( .publicKey(publicKey) .now(now) .endpointId(endpointId) - .endpointName(config.getEndpoint()) + .endpointName(config().getEndpoint()) .orgId(endpointOrgId) .sessionId(player.getSessionId()) .protocol(PROTOCOL_BEDROCK) diff --git a/core/src/main/java/com/minekube/connect/bedrock/BedrockIdentityKeyProvider.java b/core/src/main/java/com/minekube/connect/bedrock/BedrockIdentityKeyProvider.java index 86ff415a..de616a2a 100644 --- a/core/src/main/java/com/minekube/connect/bedrock/BedrockIdentityKeyProvider.java +++ b/core/src/main/java/com/minekube/connect/bedrock/BedrockIdentityKeyProvider.java @@ -2,7 +2,9 @@ import com.google.gson.Gson; import com.google.gson.JsonSyntaxException; +import com.google.inject.name.Named; import com.minekube.connect.api.player.bedrock.BedrockIdentityVerifier; +import com.minekube.connect.config.ConfigHolder; import com.minekube.connect.config.ConnectConfig; import java.io.ByteArrayOutputStream; import java.io.IOException; @@ -16,29 +18,49 @@ import java.util.Objects; import java.util.concurrent.TimeUnit; import java.util.function.Supplier; +import javax.inject.Inject; +import javax.inject.Singleton; import okhttp3.HttpUrl; import okhttp3.OkHttpClient; import okhttp3.Request; import okhttp3.Response; import okhttp3.ResponseBody; +@Singleton public final class BedrockIdentityKeyProvider { private static final Gson GSON = new Gson(); private static final long METADATA_BODY_LIMIT_BYTES = 64 * 1024; private static final long METADATA_CALL_TIMEOUT_SECONDS = 5; private static final long METADATA_FAILURE_BACKOFF_SECONDS = 5; - private final ConnectConfig config; + private final Supplier config; private final OkHttpClient httpClient; private final Supplier now; private CachedKeys cachedKeys; private Instant nextRefreshAt = Instant.MIN; + /** + * Injection seam used by the plugin. The config is resolved lazily from {@link ConfigHolder} so + * this provider can be constructed by the (config-agnostic) parent injector before + * {@code ConnectPlatform.init()} loads the configuration. + */ + @Inject + public BedrockIdentityKeyProvider( + ConfigHolder configHolder, + @Named("defaultHttpClient") OkHttpClient httpClient) { + this(Objects.requireNonNull(configHolder, "configHolder")::get, httpClient, Instant::now); + } + public BedrockIdentityKeyProvider(ConnectConfig config, OkHttpClient httpClient) { this(config, httpClient, Instant::now); } BedrockIdentityKeyProvider(ConnectConfig config, OkHttpClient httpClient, Supplier now) { + this(constant(config), httpClient, now); + } + + private BedrockIdentityKeyProvider( + Supplier config, OkHttpClient httpClient, Supplier now) { this.config = Objects.requireNonNull(config, "config"); this.httpClient = Objects.requireNonNull(httpClient, "httpClient").newBuilder() .callTimeout(METADATA_CALL_TIMEOUT_SECONDS, TimeUnit.SECONDS) @@ -48,9 +70,18 @@ public BedrockIdentityKeyProvider(ConnectConfig config, OkHttpClient httpClient) this.now = Objects.requireNonNull(now, "now"); } + private static Supplier constant(ConnectConfig config) { + Objects.requireNonNull(config, "config"); + return () -> config; + } + + private ConnectConfig config() { + return Objects.requireNonNull(config.get(), "config"); + } + synchronized List keys() { List staticKeys = staticKeys(); - String metadataUrl = config.getBedrockIdentity().getMetadataUrl(); + String metadataUrl = config().getBedrockIdentity().getMetadataUrl(); if (metadataUrl == null || metadataUrl.isEmpty()) { return staticKeys; } @@ -83,7 +114,7 @@ synchronized List keys() { synchronized boolean hasUsableKeys() { List keys = keys(); - String metadataUrl = config.getBedrockIdentity().getMetadataUrl(); + String metadataUrl = config().getBedrockIdentity().getMetadataUrl(); if (metadataUrl == null || metadataUrl.isEmpty()) { return !keys.isEmpty(); } @@ -103,7 +134,7 @@ private RemoteKeys fetchKeys(String metadataUrl) throws IOException { } VerifierMetadata metadata = GSON.fromJson(readBody(body), VerifierMetadata.class); if (metadata == null || !"Ed25519".equals(metadata.algorithm) || - !config.getBedrockIdentity().getExpectedIssuer().equals(metadata.issuer)) { + !config().getBedrockIdentity().getExpectedIssuer().equals(metadata.issuer)) { throw new IllegalArgumentException("metadata scope is invalid"); } List keys = new ArrayList<>(); @@ -178,9 +209,9 @@ private Instant failureBackoffUntil(Instant current) { private List staticKeys() { List keys = new ArrayList<>(); - addKey(keys, config.getBedrockIdentity().getPublicKey()); - if (config.getBedrockIdentity().getPublicKeys() != null) { - for (String key : config.getBedrockIdentity().getPublicKeys()) { + addKey(keys, config().getBedrockIdentity().getPublicKey()); + if (config().getBedrockIdentity().getPublicKeys() != null) { + for (String key : config().getBedrockIdentity().getPublicKeys()) { addKey(keys, key); } } @@ -213,7 +244,7 @@ private static void addMetadataKey(List keys, String encoded) { } private int metadataCacheSeconds(int remoteCacheSeconds) { - int configured = config.getBedrockIdentity().getMetadataCacheSeconds(); + int configured = config().getBedrockIdentity().getMetadataCacheSeconds(); if (configured <= 0) { return remoteCacheSeconds; } @@ -221,7 +252,7 @@ private int metadataCacheSeconds(int remoteCacheSeconds) { } private int metadataMaxStaleSeconds() { - return Math.max(0, config.getBedrockIdentity().getMetadataMaxStaleSeconds()); + return Math.max(0, config().getBedrockIdentity().getMetadataMaxStaleSeconds()); } private static final class CachedKeys { diff --git a/core/src/main/java/com/minekube/connect/module/CommonModule.java b/core/src/main/java/com/minekube/connect/module/CommonModule.java index e7849f55..95d9f689 100644 --- a/core/src/main/java/com/minekube/connect/module/CommonModule.java +++ b/core/src/main/java/com/minekube/connect/module/CommonModule.java @@ -125,20 +125,16 @@ public OkHttpClient defaultOkHttpClient() { return HttpUtils.defaultOkHttpClient(); } - @Provides - @Singleton - public BedrockIdentityKeyProvider bedrockIdentityKeyProvider( - ConnectConfig config, - @Named("defaultHttpClient") OkHttpClient httpClient) { - return new BedrockIdentityKeyProvider(config, httpClient); - } + // BedrockIdentityKeyProvider is bound just-in-time (@Inject @Singleton) so it and the enforcer + // can be resolved by the config-agnostic parent injector without pulling ConnectConfig into it. + // See BedrockParentInjectorStartupTest. @Provides @Singleton public BedrockIdentityReadiness bedrockIdentityReadiness( - ConnectConfig config, + ConfigHolder configHolder, BedrockIdentityKeyProvider keyProvider) { - return new BedrockIdentityReadiness(config, keyProvider); + return new BedrockIdentityReadiness(configHolder.get(), keyProvider); } @Provides diff --git a/core/src/test/java/com/minekube/connect/bedrock/BedrockIdentityEnforcerTest.java b/core/src/test/java/com/minekube/connect/bedrock/BedrockIdentityEnforcerTest.java index ce0ab97b..eeb1fb9e 100644 --- a/core/src/test/java/com/minekube/connect/bedrock/BedrockIdentityEnforcerTest.java +++ b/core/src/test/java/com/minekube/connect/bedrock/BedrockIdentityEnforcerTest.java @@ -78,6 +78,21 @@ void retainsLegacyHttpClientConstructor() throws Exception { new ConnectConfig(), mock(ConnectLogger.class), new OkHttpClient())); } + @Test + void retainsLegacyConfigConstructor() throws Exception { + Constructor constructor = BedrockIdentityEnforcer.class.getConstructor( + ConnectConfig.class, + ConnectLogger.class, + BedrockIdentityKeyProvider.class, + BedrockAdmissionCoordinator.class); + + assertNotNull(constructor.newInstance( + new ConnectConfig(), + mock(ConnectLogger.class), + new BedrockIdentityKeyProvider(new ConnectConfig(), new OkHttpClient()), + null)); + } + @Test void legacyConstructorUsesContextAdmissionRegistry() throws Exception { KeyPair keyPair = ed25519KeyPair(); diff --git a/core/src/test/java/com/minekube/connect/module/BedrockParentInjectorStartupTest.java b/core/src/test/java/com/minekube/connect/module/BedrockParentInjectorStartupTest.java new file mode 100644 index 00000000..7464783e --- /dev/null +++ b/core/src/test/java/com/minekube/connect/module/BedrockParentInjectorStartupTest.java @@ -0,0 +1,61 @@ +package com.minekube.connect.module; + +import static org.junit.jupiter.api.Assertions.assertNull; +import static org.junit.jupiter.api.Assertions.assertSame; +import static org.mockito.Mockito.mock; + +import com.google.inject.AbstractModule; +import com.google.inject.Guice; +import com.google.inject.Injector; +import com.google.inject.Key; +import com.google.inject.name.Names; +import com.minekube.connect.api.logger.ConnectLogger; +import com.minekube.connect.bedrock.BedrockIdentityEnforcer; +import com.minekube.connect.config.ConfigHolder; +import com.minekube.connect.config.ConnectConfig; +import okhttp3.OkHttpClient; +import org.junit.jupiter.api.Test; + +/** + * Regression guard for resolving the Bedrock graph before configuration is loaded. + * + *

The production parent injector builds the platform before {@code ConnectPlatform.init()} + * creates the config child. The graph must resolve config lazily through the parent-bound + * {@link ConfigHolder}, so the loaded {@link ConnectConfig} can be bound in the child afterward. + */ +class BedrockParentInjectorStartupTest { + + /** + * Mirrors the production ordering: a parent injector (with only pre-config bindings available) + * resolves the Bedrock enforcer the way {@code SpigotPlatformModule.platformInjector(...)} does, + * and only afterwards does {@code ConnectPlatform.init()} create the config-owning child injector. + */ + @Test + void resolvingBedrockGraphDoesNotBindConfigOnParentInjector() { + Injector parent = Guice.createInjector(new AbstractModule() { + @Override + protected void configure() { + // ConfigHolder is bound on the parent and populated by init() before config is read. + bind(ConfigHolder.class).toInstance(new ConfigHolder()); + bind(ConnectLogger.class).toInstance(mock(ConnectLogger.class)); + bind(OkHttpClient.class) + .annotatedWith(Names.named("defaultHttpClient")) + .toInstance(new OkHttpClient()); + } + }); + + // The platform injector resolves the Bedrock enforcer before config load. + parent.getInstance(BedrockIdentityEnforcer.class); + + // Regression guard: resolving the Bedrock graph must NOT create a ConnectConfig binding on + // the parent, otherwise the child ConfigLoadedModule below fails with JitBindingAlreadySet. + assertNull( + parent.getExistingBinding(Key.get(ConnectConfig.class)), + "resolving the Bedrock graph must not establish a ConnectConfig binding on the parent injector"); + + // Reproduces ConnectPlatform.init(): the loaded config is bound in a child injector. + ConnectConfig loaded = new ConnectConfig(); + Injector child = parent.createChildInjector(new ConfigLoadedModule(loaded)); + assertSame(loaded, child.getInstance(ConnectConfig.class)); + } +}