Skip to content
This repository has been archived by the owner on May 2, 2024. It is now read-only.

Improve error messages for Azure AD error codes #193

Merged
merged 2 commits into from
Apr 25, 2023

Conversation

peat-psuwit
Copy link
Contributor

This PR improve error messages for 2 cases:

  • Provides useful diagnostics for Azure AD code 65001 and 7000218 [1], which can be found when the Azure AD application is mis-configured. Error 65001 could even be resolved by just following the link in the error message.
  • Provides links, similar to [1], for other unknown codes. It links to Microsoft's error code website, which should helps when debugging the issue.

[1]

@peat-psuwit peat-psuwit requested a review from a team as a code owner April 22, 2023 18:07
@github-actions
Copy link

github-actions bot commented Apr 22, 2023

Everyone contributing to this PR have now signed the CLA. Thanks!

@didrocks
Copy link
Member

Hey! Thanks for this enhancements and making aad-auth better. The code itself looks good and will help a lot of users as we saw bug reports around this.

There are two things to consider before merging:

Do not hesitate if this triggers any questions or if you need any help! Thanks again :)

@peat-psuwit
Copy link
Contributor Author

I've actually signed CLA right after I submitted this PR, but I guess it has to be checked by a human before an entitlement is added. Anyway, should be solved now.

The test cases are added.

@codecov
Copy link

codecov bot commented Apr 25, 2023

Codecov Report

Merging #193 (b1f06b7) into main (b401372) will increase coverage by 0.23%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #193      +/-   ##
==========================================
+ Coverage   83.00%   83.24%   +0.23%     
==========================================
  Files          36       36              
  Lines        2913     2948      +35     
  Branches      279      274       -5     
==========================================
+ Hits         2418     2454      +36     
+ Misses        377      376       -1     
  Partials      118      118              
Impacted Files Coverage Δ
internal/aad/aad.go 92.30% <100.00%> (+2.51%) ⬆️
internal/aad/msal_mock.go 93.42% <100.00%> (+2.19%) ⬆️

... and 2 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Member

@denisonbarbosa denisonbarbosa left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for the changes!
A few minor nitpicks to improve readability, but everything else is great.

internal/aad/aad_test.go Outdated Show resolved Hide resolved
internal/aad/aad.go Outdated Show resolved Hide resolved
These errors usually comes from a mistake in setting up Azure AD
application. Shows more useful messages (and sometimes a remedy) in
the log to help in debugging.
This makes the life easier when debugging.
Copy link
Member

@denisonbarbosa denisonbarbosa left a comment

Choose a reason for hiding this comment

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

Looks great now! Thanks for addressing the suggestions.

Copy link
Member

@didrocks didrocks left a comment

Choose a reason for hiding this comment

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

Excellent work! Thanks for your help here, I’m sure it will unlock a lot of users who wants to set that up.

@denisonbarbosa denisonbarbosa merged commit 16d85f3 into ubuntu:main Apr 25, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants