Azure AD Integration #97

yaweiw · 2017-08-04T02:13:43Z

autoconfigure for AAD integration jwt token filter

msftclas · 2017-08-04T02:13:47Z

@yaweiw,
Thanks for your contribution as a Microsoft full-time employee or intern. You do not need to sign a CLA.
Thanks,
Microsoft Pull Request Bot

yungez · 2017-08-04T02:24:47Z

activedirectory/azure-ad-integration-spring-boot-autoconfigure/pom.xml

+    </parent>
+    <modelVersion>4.0.0</modelVersion>
+
+    <artifactId>azure-ad-integration-spring-boot-autoconfigure</artifactId>


normally service name + spring boot autoconfiguration, no need integration,

yungez · 2017-08-04T02:25:52Z

activedirectory/azure-ad-integration-spring-boot-autoconfigure/pom.xml

+        <dependency>
+            <groupId>com.nimbusds</groupId>
+            <artifactId>nimbus-jose-jwt</artifactId>
+            <version>4.39.2</version>


add the version in parent pom

add this package to parent's depedency list

yungez · 2017-08-04T02:27:56Z

...rc/main/java/com/microsoft/azure/autoconfigure/adintegration/AzureADJwtFilterProperties.java

+    @NotEmpty
+    private List<String> allowedRolesGroups;
+
+    private final String aadSignInUri = "https://login.microsoftonline.com/";


yungez · 2017-08-04T02:28:32Z

...configure/src/main/java/com/microsoft/azure/autoconfigure/adintegration/AzureADJwtToken.java

+            String bearerToken) throws ParseException, JOSEException, BadJOSEException, MalformedURLException {
+        final ConfigurableJWTProcessor jwtProcessor = new DefaultJWTProcessor();
+        final JWKSource keySource = new RemoteJWKSet(
+                new URL("https://login.microsoftonline.com/common/discovery/keys"));


define as constants?

yungez · 2017-08-04T02:29:03Z

...configure/src/main/java/com/microsoft/azure/autoconfigure/adintegration/AzureADJwtToken.java

+    }
+
+    private JWKSet loadAadPublicKeys() throws IOException, ParseException {
+        final int connectTimeout = 1000;


just cusrious, is there plan to make these configuration items configurable for users?

as discussed, theses settings are used to prevent overloading aad discover keys endpoint.

yungez · 2017-08-04T02:30:51Z

...figure/src/main/java/com/microsoft/azure/autoconfigure/adintegration/AzureADUserProfile.java

+                i++;
+            }
+        } catch (Exception e) {
+            e.printStackTrace();


not sure about business logic, besides print out, is it necessary to rethrow the exception?

and catch specific exception instead of generic exception

url.openConnection() throws Exception. so no specific exception in this case.

yungez · 2017-08-04T02:34:50Z

common/azure-spring-boot-starter-parent/pom.xml

@@ -229,6 +229,8 @@

    <modules>
        <module>../azure-spring-common</module>
+        <module>../../activedirectory/azure-ad-integration-spring-boot-autoconfigure</module>
+        <module>../../activedirectory/azure-ad-integration-spring-boot-autoconfigure-sample</module>


seems there's no xxx-sample folder in this pr..

codecov-io · 2017-08-04T02:46:36Z

Codecov Report

Merging #97 into master will decrease coverage by 6.63%.
The diff coverage is 12.9%.

@@            Coverage Diff             @@
##           master      #97      +/-   ##
==========================================
- Coverage   50.48%   43.84%   -6.64%     
==========================================
  Files          41       49       +8     
  Lines         723      878     +155     
  Branches       72       89      +17     
==========================================
+ Hits          365      385      +20     
- Misses        321      456     +135     
  Partials       37       37

Impacted Files	Coverage Δ
...osoft/azure/autoconfigure/aad/AzureADJwtToken.java	`0% <0%> (ø)`
.../autoconfigure/aad/JacksonObjectMapperFactory.java	`0% <0%> (ø)`
...azure/autoconfigure/aad/AzureADUserMembership.java	`0% <0%> (ø)`
...e/autoconfigure/aad/CustomPermissionEvaluator.java	`0% <0%> (ø)`
...zure/autoconfigure/aad/DirectoryServiceObject.java	`0% <0%> (ø)`
...nfigure/aad/AzureADJwtFilterAutoConfiguration.java	`100% <100%> (ø)`
...azure/autoconfigure/aad/AzureADJwtTokenFilter.java	`8.16% <8.16%> (ø)`
.../autoconfigure/aad/AzureADJwtFilterProperties.java	`83.33% <83.33%> (ø)`
... and 6 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f39f426...0621ed3. Read the comment docs.

xscript · 2017-08-04T02:50:25Z

activedirectory/azure-ad-integration-spring-boot-autoconfigure/pom.xml

+                <groupId>org.apache.maven.plugins</groupId>
+                <artifactId>maven-compiler-plugin</artifactId>
+                <configuration>
+                    <source>1.7</source>


1.8 for other starters?

aovechki

can you please include a readme showing basic usage in the initial PR?

aovechki · 2017-08-04T03:30:16Z

...figure/src/main/java/com/microsoft/azure/autoconfigure/adintegration/AzureADUserProfile.java

+        if (responseCode == 200) {
+            return responseInJson;
+        } else {
+            throw new Exception(responseInJson);


This should be a more specific exception, depending on 1) the actual response status code 2) parsed response body

The idea is for any non-OK responses. Rethrow the exception with response Json. should be self-explanatory for troubleshooting.

aovechki · 2017-08-04T03:30:56Z

...figure/src/main/java/com/microsoft/azure/autoconfigure/adintegration/AzureADUserProfile.java

+        conn.setRequestProperty("Accept", "application/json;odata=minimalmetadata");
+        String responseInJson = getResponseStringFromConn(conn);
+        int responseCode = conn.getResponseCode();
+        if (responseCode == 200) {


is there a const for http 200 already defined?

yes. HTTPResponse.SC_OK. thanks.

aovechki · 2017-08-04T03:32:00Z

...configure/src/main/java/com/microsoft/azure/autoconfigure/adintegration/AzureADJwtToken.java

+    }
+
+    private JWKSet loadAadPublicKeys() throws IOException, ParseException {
+        final int connectTimeout = 1000;


It's better to include time unit in the name as its suffix, ie connectTimeoutInSeconds

agree. the unit here is in milliseconds. the naming was descended from JWKSet load method in nimbusds. But we should make it more intuitive by adding units.
e.g.
public static JWKSet load(final URL url,
final int connectTimeout,
final int readTimeout,
final int sizeLimit)
throws IOException, ParseException {

aovechki · 2017-08-04T03:34:23Z

...ure/src/main/java/com/microsoft/azure/autoconfigure/adintegration/AzureADJwtTokenFilter.java

+        }
+
+        if (result == null) {
+            throw new ServiceUnavailableException("authentication result was null");


Consider a more helpful message here, ie Unable to acquire OBO token for

aovechki · 2017-08-04T03:37:35Z

...src/main/java/com/microsoft/azure/autoconfigure/adintegration/CustomPermissionEvaluator.java

+    public static boolean hasPermission(List<DirectoryServiceObject> customerRolesGroups, List<String> targetRolesGroups) {
+        boolean permitted = false;
+        for (DirectoryServiceObject rg : customerRolesGroups) {
+            if (targetRolesGroups.contains(rg.getDisplayName())) {


is case important here?

i think so. they are string literals.

aovechki · 2017-08-04T03:50:28Z

...ure/src/main/java/com/microsoft/azure/autoconfigure/adintegration/AzureADJwtTokenFilter.java

+                    AuthenticationResult result = acquireTokenForGraphApi(
+                            tokenEncoded,
+                            tid);
+                    userProfile = new AzureADUserProfile(result.getAccessToken());


looks like authz code will always run (which will retrieve user's group memberships). But this is not really necessary unless authZ requirements are explicitly configred on the resources that this filter is protecting

In the current design, users must specify what AAD roles/groups that can access the API. So membership info is retrieved upon authentication. this can be changed.
@notempty
private List allowedRolesGroups;

aovechki · 2017-08-04T03:51:41Z

...ure/src/main/java/com/microsoft/azure/autoconfigure/adintegration/AzureADJwtTokenFilter.java

+                try {
+                    String tid = jwtToken.getClaim("tid").toString();
+
+                    AuthenticationResult result = acquireTokenForGraphApi(


What is the validity period of the graph api token? Could it be cached?

Just like normal JWT token, OBO token has a "exp" field in token payload. It can be cached but may not required in this case. doFilterInternal is executed once per request upon authentication. The user will be mapped to "ROLE_ALLOWED" or "ROLE_DISALLOWED" based on it's membership in aad. The second request will require a new token so will not be reused anyway.

aovechki · 2017-08-04T03:53:59Z

.../java/com/microsoft/azure/autoconfigure/adintegration/AzureADJwtFilterAutoConfiguration.java

+ * Licensed under the MIT License. See LICENSE in the project root for
+ * license information.
+ */
+package com.microsoft.azure.autoconfigure.adintegration;


Consider using AAD or AzureAD for the folder/package instead of adintegration

aovechki · 2017-08-04T03:59:57Z

...figure/src/main/java/com/microsoft/azure/autoconfigure/adintegration/AzureADUserProfile.java

+import java.util.ArrayList;
+import java.util.List;
+
+public class AzureADUserProfile {


naming: this class effectively loads user's groups, it's not really a "userprofile"

aovechki · 2017-08-04T04:02:42Z

...ure/src/main/java/com/microsoft/azure/autoconfigure/adintegration/AzureADJwtTokenFilter.java

+                    AuthenticationResult result = acquireTokenForGraphApi(
+                            tokenEncoded,
+                            tid);
+                    userProfile = new AzureADUserProfile(result.getAccessToken());


Should consider caching user's memberships, it's likely an expensive operation to retrieve them

sure i can get a cache implemented later.

yungez · 2017-08-04T07:17:25Z

activedirectory/azure-ad-integration-spring-boot-autoconfigure/README.md

+## Usage
+
+### Register the application in Azure AD
+* Go to Azure Active Directory - App registrations - New application registration to register the application in Azure Active Directory.  `Application ID` is `clientId` in `application.properties`.   


on Azure Portal

and description of this autoconfigure package

ZhijunZhao · 2017-08-04T09:23:14Z

...onfigure/src/main/java/com/microsoft/azure/autoconfigure/aad/AzureADJwtFilterProperties.java

+
+@Validated
+@ConfigurationProperties("azure.activedirectory")
+public class AzureADJwtFilterProperties {


Please reference this commit 86aa124

to add Java doc for each field. There will be hints when user use the properties.

ZhijunZhao · 2017-08-04T09:27:32Z

...-boot-autoconfigure/src/main/java/com/microsoft/azure/autoconfigure/aad/AzureADJwtToken.java

+            String bearerToken) throws ParseException, JOSEException, BadJOSEException, MalformedURLException {
+        final ConfigurableJWTProcessor jwtProcessor = new DefaultJWTProcessor();
+        final JWKSource keySource = new RemoteJWKSet(
+                new URL("https://login.microsoftonline.com/common/discovery/keys"));


Make this constant a static field?

ZhijunZhao · 2017-08-04T09:59:15Z

...onfigure/src/main/java/com/microsoft/azure/autoconfigure/aad/AzureADJwtFilterProperties.java

+    private List<String> allowedRolesGroups;
+
+    private final String aadSignInUri = "https://login.microsoftonline.com/";
+    private final String aadGraphAPIUri = "https://graph.windows.net/";


ZhijunZhao · 2017-08-04T10:06:13Z

pom.xml

+            <dependency>
+                <groupId>com.microsoft.azure</groupId>
+                <artifactId>adal4j</artifactId>
+                <version>${azure.adal4j.version}</version>


move this dependency up to the azure lib category.

yungez · 2017-08-07T02:44:46Z

activedirectory/azure-ad-integration-spring-boot-autoconfigure/pom.xml

+        </dependency>
+        <dependency>
+            <groupId>org.mockito</groupId>
+            <artifactId>mockito-core</artifactId>


spring-boot-starter-test contains mockito already, are you demanding different version of mockito here?

aovechki · 2017-08-07T02:37:17Z

...autoconfigure/src/main/java/com/microsoft/azure/autoconfigure/aad/AzureADJwtTokenFilter.java

        }

        if (result == null) {
-            throw new ServiceUnavailableException("authentication result was null");
+            throw new ServiceUnavailableException(
+                    "unable to acquire on-behalf-of token for client " + aadJwtFilterProp.getClientId());


unable->Unable

aovechki · 2017-08-07T02:38:59Z

activedirectory/azure-ad-integration-spring-boot-autoconfigure/README.md

@@ -0,0 +1,40 @@
+## Overview
+This package provides a Spring Security filter to validate the Jwt token from Azure AD. The Jwt token is also used to acquire a On-Behalf-Of token for Azure AD Graph API so that authenticated user's membership information is available for authorization of access of API resources. 


Should insert a paragraph that describes the scenario that this enables, ie using Azure AD to implement authN/Z for spring-boot based Rest APIs.

This feature is deprecated in version 3.x. We will not support these features in 4.0.0. I found the first PR of this feature: microsoft/azure-spring-boot#97 It's on 2017-08-07. At that time, there is no spring-security-oauth2-resource-server. spring-security-oauth2-resource-server first released on 2018-09-21. refs: https://repo1.maven.org/maven2/org/springframework/security/spring-security-oauth2-resource-server/

yaweiw added 3 commits August 3, 2017 16:38

AAD Jwt filter

5f3ed9c

remove a todo

90b6db2

fix a minor build issue

bd66c2f

msftclas added the cla-not-required label Aug 4, 2017

yaweiw requested review from xscript, ZhijunZhao, zhzy0077, yungez and aovechki August 4, 2017 02:14

yungez reviewed Aug 4, 2017

View reviewed changes

fix pom in starter parent

a56832d

xscript reviewed Aug 4, 2017

View reviewed changes

aovechki suggested changes Aug 4, 2017

View reviewed changes

yaweiw added 3 commits August 4, 2017 13:46

fix as recommended

d37d24b

code refine

c44444b

add readme for autoconfigure usage

a2dca25

yungez reviewed Aug 4, 2017

View reviewed changes

yaweiw added 2 commits August 4, 2017 15:36

add overview

0dd6672

fix a minor in readme

694371d

ZhijunZhao reviewed Aug 4, 2017

View reviewed changes

yaweiw added 3 commits August 7, 2017 10:22

fix as Zhijun suggested

3902f9a

move adal4j up to lib

e4ad658

resolve conflict

39e8d7a

yungez reviewed Aug 7, 2017

View reviewed changes

aovechki approved these changes Aug 7, 2017

View reviewed changes

remove a unnecessary dependency

0621ed3

ZhijunZhao merged commit 670a965 into master Aug 7, 2017

ZhijunZhao deleted the aadintegration branch August 7, 2017 03:30

chenrujun mentioned this pull request Sep 6, 2021

Delete un-supported AAD features. Azure/azure-sdk-for-java#23922

Closed

		@@ -0,0 +1,40 @@
		## Overview
		This package provides a Spring Security filter to validate the Jwt token from Azure AD. The Jwt token is also used to acquire a On-Behalf-Of token for Azure AD Graph API so that authenticated user's membership information is available for authorization of access of API resources.

Azure AD Integration #97

Azure AD Integration #97

Conversation

yaweiw commented Aug 4, 2017

msftclas commented Aug 4, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Aug 4, 2017 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aovechki left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Aug 4, 2017 •

edited

Loading