diff --git a/java/google/registry/tools/DefaultRequestFactoryModule.java b/java/google/registry/tools/DefaultRequestFactoryModule.java index 9856202e0..dfad274d3 100644 --- a/java/google/registry/tools/DefaultRequestFactoryModule.java +++ b/java/google/registry/tools/DefaultRequestFactoryModule.java @@ -29,15 +29,20 @@ import com.google.api.client.json.JsonFactory; import com.google.api.client.json.jackson2.JacksonFactory; import com.google.api.client.util.store.AbstractDataStoreFactory; import com.google.api.client.util.store.FileDataStoreFactory; +import com.google.common.annotations.VisibleForTesting; +import com.google.common.base.Joiner; +import com.google.common.collect.ImmutableSet; +import com.google.common.collect.Ordering; import dagger.Binds; import dagger.Module; import dagger.Provides; +import google.registry.config.RegistryConfig.Config; import java.io.File; import java.io.IOException; import java.io.InputStream; import java.io.InputStreamReader; import java.lang.annotation.Documented; -import java.util.Collections; +import java.util.Collection; import javax.inject.Named; import javax.inject.Provider; import javax.inject.Qualifier; @@ -60,10 +65,6 @@ import javax.inject.Singleton; @Module class DefaultRequestFactoryModule { - // TODO(mmuller): Use @Config("requiredOauthScopes") - private static final String DEFAULT_SCOPE = - "https://www.googleapis.com/auth/userinfo.email"; - private static final File DATA_STORE_DIR = new File(System.getProperty("user.home"), ".config/nomulus/credentials"); @@ -124,8 +125,8 @@ class DefaultRequestFactoryModule { * Module for providing HttpRequestFactory. * *

Localhost connections go to the App Engine dev server. The dev server differs from most HTTP - * connections in that the types whose annotations affect the use of annotaty don't require - * OAuth2 credentials, but instead require a special cookie. + * connections in that it doesn't require OAuth2 credentials, but instead requires a special + * cookie. */ @Module abstract static class RequestFactoryModule { @@ -155,25 +156,23 @@ class DefaultRequestFactoryModule { static class AuthorizerModule { @Provides public Authorizer provideAuthorizer( - final JsonFactory jsonFactory, final AbstractDataStoreFactory dataStoreFactory) { + final JsonFactory jsonFactory, + final AbstractDataStoreFactory dataStoreFactory, + @Config("requiredOauthScopes") final ImmutableSet requiredOauthScopes) { return new Authorizer() { @Override public Credential authorize(GoogleClientSecrets clientSecrets) { try { // Run a new auth flow. GoogleAuthorizationCodeFlow flow = new GoogleAuthorizationCodeFlow.Builder( - new NetHttpTransport(), jsonFactory, clientSecrets, - Collections.singleton(DEFAULT_SCOPE)) + new NetHttpTransport(), jsonFactory, clientSecrets, requiredOauthScopes) .setDataStoreFactory(dataStoreFactory) .build(); - // TODO(mmuller): "credentials" directory needs to be qualified with the scopes and - // client id. - // We pass client id to the authorize method so we can safely persist credentials for - // multiple client ids. return new AuthorizationCodeInstalledApp(flow, new LocalServerReceiver()) - .authorize(clientSecrets.getDetails().getClientId()); + .authorize(createClientScopeQualifier( + clientSecrets.getDetails().getClientId(), requiredOauthScopes)); } catch (Exception ex) { throw new RuntimeException(ex); } @@ -182,6 +181,15 @@ class DefaultRequestFactoryModule { } } + /** + * Create a unique identifier for a given client id and collection of scopes, to be used as an + * identifier for a credential. + */ + @VisibleForTesting + static String createClientScopeQualifier(String clientId, Collection scopes) { + return clientId + " " + Joiner.on(" ").join(Ordering.natural().sortedCopy(scopes)); + } + /** * Interface that encapsulates the authorization logic to produce a credential for the user, * allowing us to override the behavior for unit tests. diff --git a/javatests/google/registry/tools/DefaultRequestFactoryModuleTest.java b/javatests/google/registry/tools/DefaultRequestFactoryModuleTest.java index 8ab9c6f0e..7382b96e8 100644 --- a/javatests/google/registry/tools/DefaultRequestFactoryModuleTest.java +++ b/javatests/google/registry/tools/DefaultRequestFactoryModuleTest.java @@ -15,6 +15,7 @@ package google.registry.tools; import static com.google.common.truth.Truth.assertThat; +import static google.registry.tools.DefaultRequestFactoryModule.createClientScopeQualifier; import static org.mockito.Matchers.any; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.verify; @@ -27,6 +28,7 @@ import com.google.api.client.http.HttpRequest; import com.google.api.client.http.HttpRequestFactory; import com.google.api.client.http.HttpRequestInitializer; import com.google.api.client.util.store.AbstractDataStoreFactory; +import com.google.common.collect.ImmutableList; import com.google.common.net.HostAndPort; import google.registry.testing.Providers; import java.io.IOException; @@ -106,4 +108,41 @@ public class DefaultRequestFactoryModuleTest { credentialProvider); assertThat(factory.getInitializer()).isSameAs(FAKE_CREDENTIAL); } + + @Test + public void test_createClientScopeQualifier() { + String simpleQualifier = + createClientScopeQualifier("client-id", ImmutableList.of("foo", "bar")); + + // If we change the way we encode client id and scopes, this assertion will break. That's + // probably ok and you can just change the text. The things you have to be aware of are: + // - Names in the new encoding should have a low risk of collision with the old encoding. + // - Changing the encoding will force all OAuth users of the nomulus tool to do a new login + // (existing credentials will not be used). + assertThat(simpleQualifier).isEqualTo("client-id bar foo"); + + // Verify order independence. + assertThat(simpleQualifier).isEqualTo( + createClientScopeQualifier("client-id", ImmutableList.of("bar", "foo"))); + + // Verify changing client id produces a different value. + assertThat(simpleQualifier).isNotEqualTo( + createClientScopeQualifier("new-client", ImmutableList.of("bar", "foo"))); + + // Verify that adding/deleting/modifying scopes produces a different value. + assertThat(simpleQualifier).isNotEqualTo( + createClientScopeQualifier("client id", ImmutableList.of("bar", "foo", "baz"))); + assertThat(simpleQualifier).isNotEqualTo( + createClientScopeQualifier("client id", ImmutableList.of("barx", "foo"))); + assertThat(simpleQualifier).isNotEqualTo( + createClientScopeQualifier("client id", ImmutableList.of("bar", "foox"))); + assertThat(simpleQualifier).isNotEqualTo( + createClientScopeQualifier("client id", ImmutableList.of("bar"))); + + // Verify that delimiting works. + assertThat(simpleQualifier).isNotEqualTo( + createClientScopeQualifier("client-id", ImmutableList.of("barf", "oo"))); + assertThat(simpleQualifier).isNotEqualTo( + createClientScopeQualifier("client-idb", ImmutableList.of("ar", "foo"))); + } }