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

Fix validating B2C issuer with tfp in issuer URI. #340

Merged
merged 3 commits into from
Aug 5, 2020
Merged

Conversation

pmaytak
Copy link
Contributor

@pmaytak pmaytak commented Jul 20, 2020

Issue #274

Fix to validate the B2C issuer with tfp URI type (ex. {domain}/{tfp}/{tid}/{userFlow}/v2.0/) correctly.

More detail:
B2C tokens compatibility
Configure B2C JWT token compatibility

@pmaytak pmaytak requested review from jmprieur and jennyf19 July 20, 2020 05:54
Copy link
Collaborator

@jmprieur jmprieur left a comment

Choose a reason for hiding this comment

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

Thanks @pmaytak

Let's circle back with the B2C team as my understanding is they want to discourage the use of tfp.
I should probably have reacted a bit earlier, but I'd want to make sure we really want to support this scenario.

@jennyf19
Copy link
Collaborator

I agree, i'll ping b2c and see what they say about this. we don't want to necessarily support legacy stuff if we don't have to. i would like to get more context on that before moving forward.

@pmaytak
Copy link
Contributor Author

pmaytak commented Jul 23, 2020

So, a couple of options:

  1. Keep code as is - B2C tfp issuers with fail with generic invalid issuer exception.
  2. Merge this PR - B2C tfp issuers will validate correctly.
  3. At the end of the validate method, we can parse actual issuer URI, if it has tfp, then throw a more descriptive exception.
    throw new SecurityTokenInvalidIssuerException(
    string.Format(
    CultureInfo.InvariantCulture,
    IDWebErrorMessage.IssuerDoesNotMatchValidIssuers,
    actualIssuer));

@jmprieur
Copy link
Collaborator

@pmaytak. Given the reply from the B2C team and the customer, I think we could do 3.

  • When you write parse, do you mean just verify that there is the /tpf/ string in it?
  • We'd throw an exception explaining that this is not supported with an aka.ms link https://aka.ms/ms-id-web/b2c-issuer

@pmaytak pmaytak force-pushed the pmaytak/b2cIssuer branch 3 times, most recently from 2b544f3 to 41455a5 Compare August 1, 2020 05:14
B2C-Tfp-Issuer.md Outdated Show resolved Hide resolved
@pmaytak
Copy link
Contributor Author

pmaytak commented Aug 1, 2020

@jmprieur @jennyf19
Updated PR:

  • throwing an exception with an aka.ms link.
  • added B2C-Tfp-Issuer.md just to get feedback for the wiki (will not merge it)

Copy link
Collaborator

@jmprieur jmprieur left a comment

Choose a reason for hiding this comment

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

LGTM.
I wonder what the md file is. Is it for the wiki?

B2C-Tfp-Issuer.md Outdated Show resolved Hide resolved
B2C-Tfp-Issuer.md Outdated Show resolved Hide resolved
B2C-Tfp-Issuer.md Outdated Show resolved Hide resolved
@pmaytak pmaytak force-pushed the pmaytak/b2cIssuer branch 2 times, most recently from edfd2f4 to 21fb1cb Compare August 4, 2020 06:17
@jennyf19
Copy link
Collaborator

jennyf19 commented Aug 4, 2020

let's not include the .md file for this. thx.

@pmaytak pmaytak force-pushed the pmaytak/b2cIssuer branch from b25cfc3 to d3f352f Compare August 5, 2020 06:02
@pmaytak pmaytak merged commit d33ab13 into master Aug 5, 2020
@pmaytak pmaytak deleted the pmaytak/b2cIssuer branch August 5, 2020 06:22
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.

3 participants