Skip to content

Conversation

@Avery-Dunn
Copy link
Contributor

@Avery-Dunn Avery-Dunn commented Apr 1, 2025

This PR removes usage of several com.nimbusds.oauth2 classes, as per #909

Mainly HTTPRequest, HTTPResponse, and ClientAuthentication were removed, along with some related minor utils. These were essentially just used as a helper for making HTTP requests: managing query parameters, creating assertions for different credentials, etc.

To replace them, this PR does the following:

  • Refactors code in ConfidentialClientApplication related to credentials and assertions, it now mainly deals with assertion/secret Strings, some logic was moved to other classes, and some methods that now had duplicate or unused code
  • Adjusts OAuthHttpRequest and HttpResponse to remove Nimbus classes and add the very minor features we were using from those classes
  • Refactors code in TokenRequestExecutor related to credentials and assertions, simplifying and clarifying the behavior around query parameters and assertions/secrets that are needed in each scenario

@Avery-Dunn Avery-Dunn requested a review from a team as a code owner April 1, 2025 17:13
@Avery-Dunn Avery-Dunn changed the title Remove HTTPRequest, ClientAuthentication, and related classes Remove usage of com.nimbusds.oauth2's HTTP classes Apr 1, 2025
private String applicationName;
private String applicationVersion;
private AadInstanceDiscoveryResponse aadAadInstanceDiscoveryResponse;
protected abstract ClientAuthentication clientAuthentication();
Copy link

Choose a reason for hiding this comment

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

We don't need a replacement for this?

Copy link
Contributor Author

@Avery-Dunn Avery-Dunn Apr 4, 2025

Choose a reason for hiding this comment

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

Long story short, this was mainly used as a parameter or return type of various nimbus methods. It's utility behavior was fully delegated to other classes:

  • It represented a client assertion/secret/cert, which some existing MSAL classes did as well
  • It was used to add query parameters to an HTTP request, but TokenRequestExecutor.addJWTBearerAssertionParams() now handles that
  • It was passed into some nimbus APIs that are no longer used

if (!StringHelper.isBlank(location)) {
try {
response.setLocation(new URI(location));
response.addHeader("Location", new URI(location).toString());
Copy link

Choose a reason for hiding this comment

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

Maybe worth it to have an enum/constants for well known headers.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was planning to do this in a follow up PR that removes various nimbus utility methods, since we'll then have the full picture of what constants and enums would be useful

Avery-Dunn and others added 2 commits April 4, 2025 10:46
Remove usage of com.nimbusds.oauth2's Token classes
@Avery-Dunn Avery-Dunn merged commit 70d4312 into avdunn/nimbus-removal Apr 4, 2025
1 of 2 checks passed
@Avery-Dunn Avery-Dunn deleted the avdunn/nimbus-http branch June 3, 2025 17:05
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.

4 participants