Refactor implementation of AadOAuth2UserService #32595

backwind1233 · 2022-12-14T07:09:52Z

Description

Refer to Design for implementation of OAuth2UserService for more details.

Code implementation by design.
Add unit tests for AadOAuth2UserService.

All SDK Contribution checklist:

The pull request does not introduce [breaking changes]
CHANGELOG is updated for new features, bug fixes or other significant changes.
I have read the contribution guidelines.

General Guidelines and Best Practices

Title of the pull request is clear and informative.
There are a small number of commits, each of which have an informative message. This means that previously merged commits do not appear in the history of the PR. For more information on cleaning up the commits in your PR, see this page.

Testing Guidelines

Pull request includes test coverage for the included changes.

azure-sdk · 2022-12-14T07:15:23Z

API change check

API changes are not detected in this pull request.

saragluna · 2022-12-15T03:13:24Z

...ava/com/azure/spring/cloud/autoconfigure/aad/implementation/webapp/AadOAuth2UserService.java

    }

    /**
     * Creates a new instance of {@link AadOAuth2UserService}.
     *
     * @param properties the AAD authentication properties
     * @param graphClient the graph client
-     * @param restTemplateBuilder the restTemplateBuilder


why deleting this?

saragluna · 2022-12-15T03:14:54Z

...ava/com/azure/spring/cloud/autoconfigure/aad/implementation/webapp/AadOAuth2UserService.java

+    }
+
+    private AadOAuth2UserService(AadAuthenticationProperties properties,
+                                GraphClient graphClient) {


saragluna · 2022-12-15T03:24:06Z

...com/azure/spring/cloud/autoconfigure/aad/implementation/webapp/AadOAuth2UserServiceTest.java

+/**
+ * Tests for {@link AadOAuth2UserService}.
+ */
+public class AadOAuth2UserServiceTest {


...com/azure/spring/cloud/autoconfigure/aad/implementation/webapp/AadOAuth2UserServiceTest.java

saragluna · 2022-12-15T03:27:29Z

...a/com/azure/spring/cloud/autoconfigure/aad/implementation/webapp/TestOAuth2AccessTokens.java

+import java.util.Arrays;
+import java.util.HashSet;
+
+public final class TestOAuth2AccessTokens {


Can this be an internal class?

Yes, will do.

saragluna · 2022-12-15T03:47:48Z

...com/azure/spring/cloud/autoconfigure/aad/implementation/webapp/AadOAuth2UserServiceTest.java

+        public static OAuth2AccessToken noScopes() {
+            return new OAuth2AccessToken(OAuth2AccessToken.TokenType.BEARER, "no-scopes", Instant.now(),
+                Instant.now().plus(Duration.ofDays(1)));
+        }
+
+        public static OAuth2AccessToken scopes(String... scopes) {


no need for public

saragluna · 2022-12-15T03:49:15Z

...com/azure/spring/cloud/autoconfigure/aad/implementation/webapp/AadOAuth2UserServiceTest.java

+class AadOAuth2UserServiceTest {
+    private ClientRegistration.Builder clientRegistrationBuilder;
+    private OidcIdToken idToken;
+    private AadOAuth2UserService aadOAuth2UserService;


seems can be local variable

I think loadUserWithCustomAuthorities() is the special one.
Other test cases could leverage the code in setup().

saragluna · 2022-12-15T03:49:25Z

...com/azure/spring/cloud/autoconfigure/aad/implementation/webapp/AadOAuth2UserServiceTest.java

+    private GraphClient graphClient;
+    private AadAuthenticationProperties properties;


same for these two

I made a little change.

saragluna · 2022-12-15T03:51:25Z

...com/azure/spring/cloud/autoconfigure/aad/implementation/webapp/AadOAuth2UserServiceTest.java

+        idTokenClaims.put(StandardClaimNames.NAME, "user1");
+        idTokenClaims.put(StandardClaimNames.EMAIL, "user1@example.com");
+
+        this.idToken = new OidcIdToken("access-token", Instant.MIN, Instant.MAX, idTokenClaims);


seems like this idToken isn't used by each test case, we can consider making this instantiated in each test method instead.

I think loadUserWithCustomAuthorities() is the special one.
Other test cases could leverage the code in setup().

If a variable is not fit for all cases, we should narrow the scope to method. It's okay to have some duplication in the UT. Which will make each test case easy to read.

saragluna · 2022-12-15T03:52:11Z

...com/azure/spring/cloud/autoconfigure/aad/implementation/webapp/AadOAuth2UserServiceTest.java

+    private AadOAuth2UserService aadOAuth2UserService;
+    private OAuth2AccessToken accessToken;
+    private Map<String, Object> idTokenClaims = new HashMap<>();
+    private GraphClient graphClient;


same for this graph client

moarychan · 2022-12-15T06:22:29Z

...ava/com/azure/spring/cloud/autoconfigure/aad/implementation/webapp/AadOAuth2UserService.java

        if (authentication != null) {
            LOGGER.debug("User {}'s authorities saved from session: {}.", authentication.getName(), authentication.getAuthorities());
            return (DefaultOidcUser) session.getAttribute(DEFAULT_OIDC_USER);
        }


Why not move this checking up to the top?

can you make it more clear?

I mean the checking for authentication != null can be done early, like the below code:

public OidcUser loadUser(OidcUserRequest userRequest) throws OAuth2AuthenticationException { Authentication authentication = SecurityContextHolder.getContext().getAuthentication(); if (authentication != null) { LOGGER.debug("User {}'s authorities saved from session: {}.", authentication.getName(), authentication.getAuthorities()); return (DefaultOidcUser) session.getAttribute(DEFAULT_OIDC_USER); } ServletRequestAttributes attr = (ServletRequestAttributes) RequestContextHolder.currentRequestAttributes(); HttpSession session = attr.getRequest().getSession(true); // ... return defaultOidcUser; }

We should get the session object first.

moarychan · 2022-12-15T06:29:29Z

...ava/com/azure/spring/cloud/autoconfigure/aad/implementation/webapp/AadOAuth2UserService.java

     */
    @Override
    public OidcUser loadUser(OidcUserRequest userRequest) throws OAuth2AuthenticationException {
-        // Delegate to the default implementation for loading a user
-        OidcUser oidcUser = oidcUserService.loadUser(userRequest);


This service is an extension for OidcUserService, if we remove the dependency of oidcUserService, it will not be a full Open ID Connect process that Spring Security implements. Some OAuth2AuthenticationExceptions will be ignored the new implementation will not throw OAuth2AuthenticationException .

This service is an extension for OidcUserService

I think it's an implementation of OAuth2UserService.

if we remove the dependency of oidcUserService, it will not be a full Open ID Connect process that Spring Security implements.

Yes, there are differences between our implementation Spring Security .
What dou mean the Open ID Connect process?

Some OAuth2AuthenticationExceptions will be ignored the new implementation will not throw OAuth2AuthenticationException .

So, is there any problem?

refactor

db71374

ghost added the azure-spring All azure-spring related issues label Dec 14, 2022

backwind1233 changed the title ~~draft~~ Draft--refactor Dec 14, 2022

backwind1233 changed the title ~~Draft--refactor~~ Draft--refactor AadOAuth2UserService# Dec 14, 2022

backwind1233 changed the title ~~Draft--refactor AadOAuth2UserService#~~ Draft--refactor AadOAuth2UserService#loadUser Dec 14, 2022

update java doc

6362ea9

backwind1233 self-assigned this Dec 14, 2022

backwind1233 added 5 commits December 14, 2022 21:22

update test codes

66e9b54

update java doc

baf7c49

add test:load user from session

ccd1af2

refactor with java8

4674b12

add clean up

55d3e20

backwind1233 linked an issue Dec 15, 2022 that may be closed by this pull request

[BUG] AadClientRegistrationRepository#toClientRegistration did not set userInfoUri #31546

Closed

backwind1233 changed the title ~~Draft--refactor AadOAuth2UserService#loadUser~~ Refactor implementation of AadOAuth2UserService Dec 15, 2022

backwind1233 marked this pull request as ready for review December 15, 2022 02:54

backwind1233 requested review from chenrujun, hui1110, Netyyyy, saragluna, stliu, yiliuTo, moarychan and fangjian0423 as code owners December 15, 2022 02:54

backwind1233 added the Client This issue points to a problem in the data-plane of the library. label Dec 15, 2022

backwind1233 added this to the 2023-01 milestone Dec 15, 2022

saragluna requested changes Dec 15, 2022

View reviewed changes

address comments

72afea9

saragluna requested changes Dec 15, 2022

View reviewed changes

address more comments

ecc88e8

moarychan reviewed Dec 15, 2022

View reviewed changes

backwind1233 added 4 commits December 15, 2022 15:25

refactor ut

9d69d7f

make TestOAuth2AccessTokens final

e1dcd10

ut refactor

fdb84f3

format

e76ca6b

saragluna approved these changes Dec 15, 2022

View reviewed changes

backwind1233 merged commit 71029d5 into Azure:main Dec 16, 2022

backwind1233 deleted the aadoauth2userservice_gzh branch December 16, 2022 01:10

backwind1233 mentioned this pull request Dec 16, 2022

Refactor implementation of AadOAuth2UserService #32622

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor implementation of AadOAuth2UserService #32595

Refactor implementation of AadOAuth2UserService #32595

backwind1233 commented Dec 14, 2022 •

edited

Loading

azure-sdk commented Dec 14, 2022

saragluna Dec 15, 2022

saragluna Dec 15, 2022

saragluna Dec 15, 2022

saragluna Dec 15, 2022

backwind1233 Dec 15, 2022

saragluna Dec 15, 2022

saragluna Dec 15, 2022

backwind1233 Dec 15, 2022

saragluna Dec 15, 2022

backwind1233 Dec 15, 2022 •

edited

Loading

saragluna Dec 15, 2022

backwind1233 Dec 15, 2022

saragluna Dec 15, 2022

saragluna Dec 15, 2022

moarychan Dec 15, 2022

backwind1233 Dec 15, 2022

moarychan Dec 15, 2022

backwind1233 Dec 15, 2022

moarychan Dec 15, 2022

backwind1233 Dec 15, 2022

		private GraphClient graphClient;
		private AadAuthenticationProperties properties;

Refactor implementation of AadOAuth2UserService #32595

Refactor implementation of AadOAuth2UserService #32595

Conversation

backwind1233 commented Dec 14, 2022 • edited Loading

Description

All SDK Contribution checklist:

azure-sdk commented Dec 14, 2022

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

backwind1233 Dec 15, 2022 • edited Loading

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

backwind1233 commented Dec 14, 2022 •

edited

Loading

backwind1233 Dec 15, 2022 •

edited

Loading