Use Directory.Read.All intead of Directory.AccessAsUser.All (Azure#18901)

Rujun Chen · web-flow · commit 420323ddd1c4 · 2021-02-03T07:46:51.000+08:00
* To avoid admin consent, use "Directory.Read.All" instead of "Directory.AccessAsUser.All".

* Add check: When azure.activedirectory.teannt-id=common, azure.activedirectory.user-group.allowed-groups should be empty.
diff --git a/sdk/spring/azure-spring-boot-samples/azure-spring-boot-sample-active-directory-webapp/src/main/resources/application.yml b/sdk/spring/azure-spring-boot-samples/azure-spring-boot-sample-active-directory-webapp/src/main/resources/application.yml
@@ -7,7 +7,7 @@ azure:
       graph:
         scopes:
             - https://graph.microsoft.com/User.Read
-            - https://graph.microsoft.com/Directory.AccessAsUser.All
+            - https://graph.microsoft.com/Directory.Read.All
       office:
         scopes:
             - https://manage.office.com/ActivityFeed.Read
diff --git a/sdk/spring/azure-spring-boot-test-aad/src/test/java/com/azure/test/aad/selenium/access/token/scopes/AADAccessTokenScopesIT.java b/sdk/spring/azure-spring-boot-test-aad/src/test/java/com/azure/test/aad/selenium/access/token/scopes/AADAccessTokenScopesIT.java
@@ -33,17 +33,17 @@ public void testAccessTokenScopes() {
                 + "https://manage.office.com/ServiceHealth.Read");
         properties.put(
             "azure.activedirectory.authorization-clients.graph.scopes",
-            "https://graph.microsoft.com/User.Read, https://graph.microsoft.com/Directory.AccessAsUser.All");
+            "https://graph.microsoft.com/User.Read, https://graph.microsoft.com/Directory.Read.All");
         aadSeleniumITHelper = new AADSeleniumITHelper(DumbApp.class, properties);
         aadSeleniumITHelper.logIn();
         String httpResponse = aadSeleniumITHelper.httpGet("accessTokenScopes/azure");
         Assert.assertTrue(httpResponse.contains("profile"));
-        Assert.assertTrue(httpResponse.contains("https://graph.microsoft.com/Directory.AccessAsUser.All"));
+        Assert.assertTrue(httpResponse.contains("https://graph.microsoft.com/Directory.Read.All"));
         Assert.assertTrue(httpResponse.contains("https://graph.microsoft.com/User.Read"));
 
         httpResponse = aadSeleniumITHelper.httpGet("accessTokenScopes/graph");
         Assert.assertTrue(httpResponse.contains("profile"));
-        Assert.assertTrue(httpResponse.contains("https://graph.microsoft.com/Directory.AccessAsUser.All"));
+        Assert.assertTrue(httpResponse.contains("https://graph.microsoft.com/Directory.Read.All"));
         Assert.assertTrue(httpResponse.contains("https://graph.microsoft.com/User.Read"));
 
         httpResponse = aadSeleniumITHelper.httpGet("accessTokenScopes/office");
diff --git a/sdk/spring/azure-spring-boot/src/main/java/com/azure/spring/aad/webapp/AADWebAppConfiguration.java b/sdk/spring/azure-spring-boot/src/main/java/com/azure/spring/aad/webapp/AADWebAppConfiguration.java
@@ -116,7 +116,7 @@ private Set<String> accessTokenScopes() {
         if (properties.allowedGroupsConfigured()) {
             // The 2 scopes are need to get group name from graph.
             result.add(properties.getGraphBaseUri() + "User.Read");
-            result.add(properties.getGraphBaseUri() + "Directory.AccessAsUser.All");
+            result.add(properties.getGraphBaseUri() + "Directory.Read.All");
         }
         return result;
     }
diff --git a/sdk/spring/azure-spring-boot/src/main/java/com/azure/spring/autoconfigure/aad/AADAuthenticationProperties.java b/sdk/spring/azure-spring-boot/src/main/java/com/azure/spring/autoconfigure/aad/AADAuthenticationProperties.java
@@ -84,7 +84,7 @@ public class AADAuthenticationProperties implements InitializingBean {
     /**
      * Azure Tenant ID.
      */
-    private String tenantId = "common";
+    private String tenantId;
 
     private String postLogoutRedirectUri;
 
@@ -337,6 +337,16 @@ public void afterPropertiesSet() throws Exception {
                 + "azure.activedirectory.graph-base-uri = " + graphBaseUri + ", "
                 + "azure.activedirectory.graph-membership-uri = " + graphMembershipUri + ".");
         }
+
+        if (!StringUtils.hasText(tenantId)) {
+            tenantId = "common";
+        }
+        if ("common".equals(tenantId) && !userGroup.getAllowedGroups().isEmpty()) {
+            throw new IllegalStateException("When azure.activedirectory.teannt-id=common, "
+                + "azure.activedirectory.user-group.allowed-groups should be empty. "
+                + "But actually azure.activedirectory.user-group.allowed-groups="
+                + userGroup.getAllowedGroups());
+        }
     }
 
     private String addSlash(String uri) {
diff --git a/sdk/spring/azure-spring-boot/src/test/java/com/azure/spring/aad/webapi/AADResourceServerConfigurationTest.java b/sdk/spring/azure-spring-boot/src/test/java/com/azure/spring/aad/webapi/AADResourceServerConfigurationTest.java
@@ -18,17 +18,14 @@
 
 public class AADResourceServerConfigurationTest {
 
-    private final WebApplicationContextRunner contextRunner = new WebApplicationContextRunner()
-        .withPropertyValues("azure.activedirectory.user-group.allowed-groups=User");
+    private final WebApplicationContextRunner contextRunner = new WebApplicationContextRunner();
 
     @Test
     public void testNotExistBearerTokenAuthenticationToken() {
         this.contextRunner
             .withUserConfiguration(AADResourceServerConfiguration.class)
             .withClassLoader(new FilteredClassLoader(BearerTokenAuthenticationToken.class))
-            .run(context -> {
-                assertThat(context).doesNotHaveBean("jwtDecoderByJwkKeySetUri");
-            });
+            .run(context -> assertThat(context).doesNotHaveBean("jwtDecoderByJwkKeySetUri"));
     }
 
     @Test
diff --git a/sdk/spring/azure-spring-boot/src/test/java/com/azure/spring/aad/webapp/AADWebAppConfigurationTest.java b/sdk/spring/azure-spring-boot/src/test/java/com/azure/spring/aad/webapp/AADWebAppConfigurationTest.java
@@ -301,7 +301,7 @@ public void groupConfiguration() {
                 assertDefaultScopes(
                     clientRepo.getAzureClient(),
                     "openid", "profile", "https://graph.microsoft.com/User.Read",
-                    "https://graph.microsoft.com/Directory.AccessAsUser.All"
+                    "https://graph.microsoft.com/Directory.Read.All"
                 );
             });
     }
@@ -349,7 +349,7 @@ public void resourceServerCountTest() {
         assertEquals(resourceServerCount(scopes), 0);
         scopes.add("https://graph.microsoft.com/User.Read");
         assertEquals(resourceServerCount(scopes), 1);
-        scopes.add("https://graph.microsoft.com/Directory.AccessAsUser.All");
+        scopes.add("https://graph.microsoft.com/Directory.Read.All");
         assertEquals(resourceServerCount(scopes), 1);
         scopes.add("https://manage.office.com/ActivityFeed.Read");
         assertEquals(resourceServerCount(scopes), 2);
diff --git a/sdk/spring/azure-spring-boot/src/test/java/com/azure/spring/autoconfigure/aad/AADAuthenticationFilterPropertiesTest.java b/sdk/spring/azure-spring-boot/src/test/java/com/azure/spring/autoconfigure/aad/AADAuthenticationFilterPropertiesTest.java
@@ -23,10 +23,11 @@
 public class AADAuthenticationFilterPropertiesTest {
     @After
     public void clearAllProperties() {
-        System.clearProperty(TestConstants.SERVICE_ENVIRONMENT_PROPERTY);
-        System.clearProperty(TestConstants.CLIENT_ID_PROPERTY);
-        System.clearProperty(TestConstants.CLIENT_SECRET_PROPERTY);
-        System.clearProperty(TestConstants.TARGETED_GROUPS_PROPERTY);
+        System.clearProperty("azure.activedirectory.environment");
+        System.clearProperty("azure.activedirectory.tenant-id");
+        System.clearProperty("azure.activedirectory.client-id");
+        System.clearProperty("azure.activedirectory.client-secret");
+        System.clearProperty("azure.activedirectory.user-group.allowed-groups");
     }
 
     @Test
@@ -47,18 +48,19 @@ public void canSetProperties() {
     }
 
     private void configureAllRequiredProperties() {
-        System.setProperty(TestConstants.CLIENT_ID_PROPERTY, TestConstants.CLIENT_ID);
-        System.setProperty(TestConstants.CLIENT_SECRET_PROPERTY, TestConstants.CLIENT_SECRET);
-        System.setProperty(TestConstants.TARGETED_GROUPS_PROPERTY,
-                TestConstants.TARGETED_GROUPS.toString().replace("[", "").replace("]", ""));
-        System.setProperty(TestConstants.ALLOW_TELEMETRY_PROPERTY, "false");
+        System.setProperty("azure.activedirectory.tenant-id", "demo-tenant-id");
+        System.setProperty("azure.activedirectory.client-id", TestConstants.CLIENT_ID);
+        System.setProperty("azure.activedirectory.client-secret", TestConstants.CLIENT_SECRET);
+        System.setProperty("azure.activedirectory.user-group.allowed-groups",
+            TestConstants.TARGETED_GROUPS.toString().replace("[", "").replace("]", ""));
+        System.setProperty("azure.activedirectory.allow-telemetry", "false");
     }
 
     @Test
     @Ignore // TODO (wepa) clientId and clientSecret can also be configured in oauth2 config, test to be refactored
     public void emptySettingsNotAllowed() {
-        System.setProperty(TestConstants.CLIENT_ID_PROPERTY, "");
-        System.setProperty(TestConstants.CLIENT_SECRET_PROPERTY, "");
+        System.setProperty("azure.activedirectory.client-id", "");
+        System.setProperty("azure.activedirectory.client-secret", "");
 
         try (AnnotationConfigApplicationContext context = new AnnotationConfigApplicationContext()) {
             Exception exception = null;
diff --git a/sdk/spring/azure-spring-boot/src/test/java/com/azure/spring/autoconfigure/aad/AADAuthenticationFilterTest.java b/sdk/spring/azure-spring-boot/src/test/java/com/azure/spring/autoconfigure/aad/AADAuthenticationFilterTest.java
@@ -55,9 +55,9 @@ public AADAuthenticationFilterTest() {
     @Test
     @Ignore
     public void doFilterInternal() {
-        this.contextRunner.withPropertyValues(TestConstants.CLIENT_ID_PROPERTY, TestConstants.CLIENT_ID)
-                .withPropertyValues(TestConstants.CLIENT_SECRET_PROPERTY, TestConstants.CLIENT_SECRET)
-                .withPropertyValues(TestConstants.TARGETED_GROUPS_PROPERTY,
+        this.contextRunner.withPropertyValues("azure.activedirectory.client-id", TestConstants.CLIENT_ID)
+                .withPropertyValues("azure.activedirectory.client-secret", TestConstants.CLIENT_SECRET)
+                .withPropertyValues("azure.activedirectory.client-secret",
                         TestConstants.TARGETED_GROUPS.toString()
                                                      .replace("[", "").replace("]", ""));
 
diff --git a/sdk/spring/azure-spring-boot/src/test/java/com/azure/spring/autoconfigure/aad/ResourceRetrieverTest.java b/sdk/spring/azure-spring-boot/src/test/java/com/azure/spring/autoconfigure/aad/ResourceRetrieverTest.java
@@ -16,12 +16,12 @@
 
 public class ResourceRetrieverTest {
     private final WebApplicationContextRunner contextRunner = new WebApplicationContextRunner()
-            .withConfiguration(AutoConfigurations.of(AADAuthenticationFilterAutoConfiguration.class))
-            .withClassLoader(new FilteredClassLoader(BearerTokenAuthenticationToken.class))
-            .withPropertyValues("azure.activedirectory.client-id=fake-client-id",
-                    "azure.activedirectory.client-secret=fake-client-secret",
-                    "azure.activedirectory.user-group.allowed-groups=fake-group",
-                    "azure.service.endpoints.global.aadKeyDiscoveryUri=http://fake.aad.discovery.uri");
+        .withConfiguration(AutoConfigurations.of(AADAuthenticationFilterAutoConfiguration.class))
+        .withClassLoader(new FilteredClassLoader(BearerTokenAuthenticationToken.class))
+        .withPropertyValues(
+            "azure.activedirectory.client-id=fake-client-id",
+            "azure.activedirectory.client-secret=fake-client-secret",
+            "azure.service.endpoints.global.aadKeyDiscoveryUri=http://fake.aad.discovery.uri");
 
     @Test
     public void resourceRetrieverDefaultConfig() {
@@ -39,19 +39,21 @@ public void resourceRetrieverDefaultConfig() {
 
     @Test
     public void resourceRetriverIsConfigurable() {
-        this.contextRunner.withPropertyValues("azure.activedirectory.jwt-connect-timeout=1234",
+        this.contextRunner
+            .withPropertyValues(
+                "azure.activedirectory.jwt-connect-timeout=1234",
                 "azure.activedirectory.jwt-read-timeout=1234",
                 "azure.activedirectory.jwt-size-limit=123400",
                 "azure.service.endpoints.global.aadKeyDiscoveryUri=http://fake.aad.discovery.uri")
-                .run(context -> {
-                    assertThat(context).hasSingleBean(ResourceRetriever.class);
-                    final ResourceRetriever retriever = context.getBean(ResourceRetriever.class);
-                    assertThat(retriever).isInstanceOf(DefaultResourceRetriever.class);
-
-                    final DefaultResourceRetriever defaultRetriever = (DefaultResourceRetriever) retriever;
-                    assertThat(defaultRetriever.getConnectTimeout()).isEqualTo(1234);
-                    assertThat(defaultRetriever.getReadTimeout()).isEqualTo(1234);
-                    assertThat(defaultRetriever.getSizeLimit()).isEqualTo(123400);
-                });
+            .run(context -> {
+                assertThat(context).hasSingleBean(ResourceRetriever.class);
+                final ResourceRetriever retriever = context.getBean(ResourceRetriever.class);
+                assertThat(retriever).isInstanceOf(DefaultResourceRetriever.class);
+
+                final DefaultResourceRetriever defaultRetriever = (DefaultResourceRetriever) retriever;
+                assertThat(defaultRetriever.getConnectTimeout()).isEqualTo(1234);
+                assertThat(defaultRetriever.getReadTimeout()).isEqualTo(1234);
+                assertThat(defaultRetriever.getSizeLimit()).isEqualTo(123400);
+            });
     }
 }
diff --git a/sdk/spring/azure-spring-boot/src/test/java/com/azure/spring/autoconfigure/aad/TestConstants.java b/sdk/spring/azure-spring-boot/src/test/java/com/azure/spring/autoconfigure/aad/TestConstants.java
@@ -7,11 +7,6 @@
 import java.util.List;
 
 public class TestConstants {
-    public static final String SERVICE_ENVIRONMENT_PROPERTY = "azure.activedirectory.environment";
-    public static final String CLIENT_ID_PROPERTY = "azure.activedirectory.client-id";
-    public static final String CLIENT_SECRET_PROPERTY = "azure.activedirectory.client-secret";
-    public static final String TARGETED_GROUPS_PROPERTY = "azure.activedirectory.user-group.allowed-groups";
-    public static final String ALLOW_TELEMETRY_PROPERTY = "azure.activedirectory.allow-telemetry";
 
     public static final String CLIENT_ID = "real_client_id";
     public static final String CLIENT_SECRET = "real_client_secret";

Original file line number	Diff line number	Diff line change
`@@ -116,7 +116,7 @@ private Set<String> accessTokenScopes() {`
`116`	`116`	`if (properties.allowedGroupsConfigured()) {`
`117`	`117`	`// The 2 scopes are need to get group name from graph.`
`118`	`118`	`result.add(properties.getGraphBaseUri() + "User.Read");`
`119`		`- result.add(properties.getGraphBaseUri() + "Directory.AccessAsUser.All");`
	`119`	`+ result.add(properties.getGraphBaseUri() + "Directory.Read.All");`
`120`	`120`	`}`
`121`	`121`	`return result;`
`122`	`122`	`}`