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

Fixed MSAL-Node HttpClient AAD-Server-Error-500 Bug #5342

Merged
merged 13 commits into from
Nov 10, 2022

Conversation

Robbie-Microsoft
Copy link
Collaborator

@Robbie-Microsoft Robbie-Microsoft commented Nov 1, 2022

Previous refactor of MSAL-Node's HttpClient was not tested with an actual 500 error from an AAD server. After using Fiddler Everywhere to further test HttpClient and simulate a 500 error from an AAD server, it was discovered that a 500 error returned from an AAD server would break the application. This PR covers additional functionality in HttpClient to address a 500 error returned from an AAD server.

@github-actions github-actions bot added the msal-node Related to msal-node package label Nov 1, 2022
@Robbie-Microsoft Robbie-Microsoft changed the title Fixed MSAL-Node Http client Server Error 500 Bug Fixed MSAL-Node HttpClient AAD Server-Error-500 Bug Nov 1, 2022
@Robbie-Microsoft Robbie-Microsoft changed the title Fixed MSAL-Node HttpClient AAD Server-Error-500 Bug Fixed MSAL-Node HttpClient AAD-Server-Error-500 Bug Nov 1, 2022
@codecov-commenter
Copy link

codecov-commenter commented Nov 1, 2022

Codecov Report

Merging #5342 (c17d95a) into dev (1aac88b) will increase coverage by 0.02%.
The diff coverage is 92.50%.

Flag Coverage Δ *Carryforward flag
msal-angular 96.50% <ø> (ø) Carriedforward from 1aac88b
msal-browser 86.47% <ø> (ø) Carriedforward from 1aac88b
msal-common 85.45% <ø> (ø) Carriedforward from 1aac88b
msal-core 82.84% <ø> (ø) Carriedforward from 1aac88b
msal-node 83.37% <92.50%> (+0.46%) ⬆️
msal-node-extensions 76.03% <ø> (ø) Carriedforward from 1aac88b
msal-react 94.50% <ø> (ø) Carriedforward from 1aac88b
node-token-validation 88.88% <ø> (ø) Carriedforward from 1aac88b

*This pull request uses carry forward flags. Click here to find out more.

Impacted Files Coverage Δ
lib/msal-node/src/network/HttpClient.ts 92.85% <89.28%> (+0.33%) ⬆️
lib/msal-node/src/utils/Constants.ts 100.00% <100.00%> (ø)
lib/msal-node/src/utils/NetworkUtils.ts 100.00% <100.00%> (ø)

Copy link
Member

@sameerag sameerag left a comment

Choose a reason for hiding this comment

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

Added a few comments, mostly around code structure and readability.

@@ -12,8 +12,19 @@ export enum HttpMethod {
}

export enum HttpStatus {
OK = 200,
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This was not being used anywhere.

Copy link
Member

@sameerag sameerag left a comment

Choose a reason for hiding this comment

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

Added one final comment to complete the status checks and error assignments. Approving though.

@Robbie-Microsoft Robbie-Microsoft merged commit 42e1baf into dev Nov 10, 2022
@Robbie-Microsoft Robbie-Microsoft deleted the httpClient_500_error branch November 10, 2022 20:08
@ghost
Copy link

ghost commented Nov 22, 2022

🎉@azure/msal-node@v1.14.4 has been released which incorporates this pull request.:tada:

Handy links:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
msal-node Related to msal-node package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants