Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

compatability for jpackaged modular applications #571

Closed
wants to merge 4 commits into from

Conversation

RJPG-TK
Copy link

@RJPG-TK RJPG-TK commented Nov 24, 2022

In order to use IWA in a jpackaged modular application that runs on a current JVM (e.g. 17), I propose these changes to be merged.

Problem

MSAL4J uses

private static String getProductVersion() {
   if (HttpHeaders.class.getPackage().getImplementationVersion() == null) {
      return "1.0";
   }
   return HttpHeaders.class.getPackage().getImplementationVersion();
}

in com.microsoft.aad.msal4j.HttpHeaders to set the header value for x-client-VER.

However, if it's set to "1.0" IWA fails with
Execution of class com.microsoft.aad.msal4j.AcquireTokenByAuthorizationGrantSupplier failed. com.microsoft.aad.msal4j.MsalClientException: Password is required for managed user
See also: AzureAD/microsoft-authentication-library-for-dotnet#1860

This is an issue because Package#getImplementationVersion returns null in a jpackaged modular application.

Solution

From Java 9 onwards, ModuleDescriptor.Version can be used to do pretty much the same. Therefore, I have set up a multi release jar build, with JVM 9+ using ModuleDescriptor.
For this to work, a few plugins needed to be updated (see pom).

Restrictions

Due to bnd not yet properly supporting multi release jars, I used fixupmessages as suggested in the related issue bndtools/bnd#2227.
Errors like this one: projectlombok/lombok#2681 made me build the project on jdk-11.0.16.1+1.
Furthermore, I skipped the javadoc task - due to this bug https://issues.apache.org/jira/browse/MJAVADOC-586 / https://bugs.openjdk.org/browse/JDK-8220702.

All in all, a mvn clean package -Dmaven.javadoc.skip=true should run on jdk-11.0.16.1+1.

Workaround for the meantime

We can exploit that extra headers passed to the builder will be passed through to HttpHeaders and eventually override already set headers. This is not a solution, but a temporary workaround, as it relies on an implementation detail that might change some day!
In code:

IntegratedWindowsAuthenticationParameters.builder(...)
   .tenant(...)
   .extraHttpHeaders(getExtraHeaders())
   .build();

with something like

private Map<String, String> getExtraHeaders() {
	Optional<ModuleDescriptor.Version> version = IAuthenticationResult.class.getModule().getDescriptor().version();
	Map<String, String> extraHeaders =
		Map.of(
			"x-client-VER",
			version.map(ModuleDescriptor.Version::toString).orElse(TESTED_MSAL4J_X_CLIENT_VER_STRING)
		);
	return extraHeaders;
}

@RJPG-TK
Copy link
Author

RJPG-TK commented Nov 24, 2022

@RJPG-TK please read the following Contributor License Agreement(CLA). If you agree with the CLA, please reply with the following information.

@microsoft-github-policy-service agree [company="{your company}"]

Options:

  • (default - no company specified) I have sole ownership of intellectual property rights to my Submissions and I am not making Submissions in the course of work for my employer.
  • (when company given) I am making Submissions in the course of work for my employer (or my employer has intellectual property rights in my Submissions by contract or applicable law). I have permission from my employer to make Submissions and enter into this Agreement on behalf of my employer. By signing below, the defined term “You” includes me and my employer.

Contributor License Agreement

@microsoft-github-policy-service agree company="Techniker Krankenkasse"

@RJPG-TK
Copy link
Author

RJPG-TK commented Nov 24, 2022

@microsoft-github-policy-service agree company="Techniker Krankenkasse"

@RJPG-TK
Copy link
Author

RJPG-TK commented Feb 2, 2023

@Avery-Dunn Hi, could you kindly have a look into this PR if you find the time?

@@ -0,0 +1,126 @@
// Copyright (c) Microsoft Corporation. All rights reserved.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@siddhijain @Avery-Dunn - please try to respond to community contributions. This looks ok to me, although it's missing tests. What is the context here? I don't see any feedback on this PR.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, Avery and I talked about this PR, but we never responded.

Context - We use a method getProductVersion() to get the msal4j version, and it returns 1 by default (i.e., when it cannot find the implementation version, for eg when running tests or in a jpackaged modular application). According to the person who raised PR, it is throwing exceptions while using IWA flow.
The PR adds another version of the HttpHeaders class that uses Java 9 features to handle this error. However, we avoid maintaining different versions of a class for different Java versions due to reasons like increased code complexity, code duplication, incompatibility issues, upgrade difficulties, and many more. 

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As it seems this PR won't be merged, is there any fix you'd propose?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@RJPG-TK For the time being, please continue using the workaround you provided in your message above, as we find a solution to the issue.

@Avery-Dunn Avery-Dunn closed this Jun 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants