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

Friendly hint for a typical oidc_authority error #680

Open
wants to merge 805 commits into
base: dev
Choose a base branch
from

Conversation

rayluo
Copy link
Collaborator

@rayluo rayluo commented Mar 21, 2024

In our own web app testing, we sometimes forgot to append the /v2.0 suffix to a CIAM CUD oidc_authority, and ended up with a cryptic error message, "AADSTS500207: The account type can't be used for the resource you're trying to access". This may become an FAQ and a frequent source of customer support requests.

In this PR, we tentatively add a hint into the error message.

'Did you forget to append "/v2.0" to your oidc_authority? '

so that a full error page in a web app may look like this:

Login Failure

invalid_request
Did you forget to append "/v2.0" to your oidc_authority? AADSTS500207: The account type can't be used for the resource you're trying to access. Trace ID: e4568f2b-f5b3-4e5e-b766-e7689b180000 Correlation ID: 765569d0-7583-45ec-93f1-69d6095164a4 Timestamp: 2024-03-21 03:17:17Z

Note:

  • Currently, this treatment is added into the most flows, including CCA's auth code flow, Client Credential flow, Username Password flow (ROPC), and PCA's Device Code flow.

    • Note that the ROPC flow yields a different error number.
  • PCA's interactive flow has two places to return the error: the API response, and the error rendered in browser. In this PR, we added the treatment for the former because it was easy (just a one-liner). The latter would also be a good candidate to receive this treatment, however it is not implemented in this PR due to its complexity.

  • The so-called "dev samples" inside the MSAL repo are also updated this time. They can become the blueprint when we update the AzureSamples later.

  • This PR also contains a change to skip user realm discovery for oidc authority.

Bubble up refresh exception when we cannot recover
Reduce duplicated magic strings

Add test cases

Writing docs
Define some Cloud Instance constants and the usage pattern of using them
Fine tune http_cache usage pattern
Descriptive error messages for troubleshooting
Actionable suggestion for ID token validation failures
Actionable exception from ADFS username password flow
Merge MSAL Python 1.17.0 back to dev branch
This way, it will probably show up properly in PyPI, too.
rayluo and others added 23 commits January 29, 2024 15:31
Give a hint on where the client_id came from
This is needed because our org has transitioned to a read-only GITHUB_TOKEN for GitHub Action workflows.
This change fixes #653
Using a bare except statement when importing hides other errors, which
then get lost when the next import fails.

Co-authored-by: Ed Singleton <singletoned@Lorne.local>
Ray: I tested it on his Win laptop to successfully acquire normal token and ssh cert from broker
…irect-uri

Update the default broker redirect URI to be a valid URI
* Rebrand from AAD to Microsoft Entra
* Readme rebranding
broker-test.py shall test Azure CLI in MSA-PT mode
if ("AADSTS500207" in response.get("error_description", "") and
self._oidc_authority and not self._oidc_authority.endswith("/v2.0")):
response["error_description"] = (
'Did you forget to append "/v2.0" to your oidc_authority? '
Copy link

Choose a reason for hiding this comment

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

I feel like the best way that this error should be handled/prevented is for the v2.0 endpoint not even existing for CIAM; which is being worked on by the CIAM team. You could choose to keep this handling here, but just know that in the near future developers should never be getting this error anymore as being caused by forgetting to add v2.0 to the URL path.

Copy link
Member

Choose a reason for hiding this comment

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

@SammyO - does the CIAM team consider making this change for CIAM CUD GA? Because once we go GA, it's all over - you'll have to support "v2.0" suffix (maybe through DNS hackery), otherwise you'll break production apps.

Copy link

Choose a reason for hiding this comment

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

@bgavrilMS yes, exactly because of the reason you've outlined there.

Copy link
Member

Choose a reason for hiding this comment

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

@rayluo - pls open a similar issue to MSAL.NET

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

the v2.0 endpoint not even existing for CIAM; which is being worked on by the CIAM team

Does that mean the OIDC authority for CUD after its GA will be simply https://contoso.com/tenant i.e. without the /v2.0 suffix? That would essentially mean the V2 URL converges into the V1 URL. We had this proposal before, but it was rejected because it is believed that V1 and V2 URLs need to coexist. Is that no longer the case? @SammyO

Copy link

Choose a reason for hiding this comment

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

@rayluo what I think will be happening is a v1.0 version of an authority endpoint returning something like a 404. I don't think there will be a converging (as this would hide logic from developers and make things less explicit, which IMO is not desirable). But let me confirm.

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.