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

Added flexible subdomain support for UserInfo endpoint. Updated docs … #739

Merged
merged 3 commits into from Dec 15, 2022
Merged

Added flexible subdomain support for UserInfo endpoint. Updated docs … #739

merged 3 commits into from Dec 15, 2022

Conversation

SuperOfficeDevNet
Copy link
Contributor

…link.

This PR enabled backwards compatibility with current and very near future changes with how the SuperOffice provider resolves UserInfo endpoint data.

This PR also updates README link to the docs.

Copy link
Member

@martincostello martincostello left a comment

Choose a reason for hiding this comment

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

Could you add another test that covers the case where the webapi_url claim is used?

I also wonder if we're missing some code coverage here, as I'm surprised the change isn't causing any tests to fail as the webapi_url claim is required yet there's no test changes and they're still all passing.

It also kind of feels to me that for this change to be backwards compatible, if the webapi_url claim isn't found, it should just automatically fall back to the previous behaviour, rather than throw.

@martincostello
Copy link
Member

Regarding the webapi_url claim, I was referring to the fact that there wasn't a property for that in the test bundle. However, if it's because it's encoded into the id_token property then that would explain it.

@martincostello martincostello added this to the 6.0.13 milestone Oct 24, 2022
@SuperOfficeDevNet
Copy link
Contributor Author

Regarding the webapi_url claim, I was referring to the fact that there wasn't a property for that in the test bundle. However, if it's because it's encoded into the id_token property then that would explain it.

Yes, that's exactly correct. I added an exception if that claim doesn't get returned from the ProcessIdTokenAndGetContactIdentifierAsync method, but this should never happen. But if we every really do make that mistake, at least the exception message would signal just exactly what we did (or didn't do) to upset all of our partners.

@martincostello martincostello modified the milestones: 6.0.13, 6.0.14, 6.0.15 Nov 1, 2022
@SuperOfficeDevNet
Copy link
Contributor Author

Hi! Any particular reason why this hasn't been merged yet? Seems to have been flags for past milestones, but still just a PR. Any update is appreciate.

@martincostello
Copy link
Member

At first it was an oversight, but now the dev branch targets .NET 7 so there needs to be some additional manual work to port it to a .NET 6 branch for you (as that's what you were originally targeting), and then I just forgot about if I'm honest.

I'll try and sort this out at some point this week. Sorry for the delay.

@martincostello
Copy link
Member

Sorry, but I've had to revert this change as the unit tests are broken by it.

Unfortunately this wasn't spotted before merging due to dotnet/arcade#11589 not causing the builds to report themselves as failed.

Please submit a new PR to re-apply this change including the appropriate changes to the unit tests so they continue to pass (please also run the tests locally when developing, they would have surfaced this issue before you even opened the pull request).

Once submitted, I'll then look at re-merging and porting to a branch to release a version for .NET 6 and publishing the 6.0.15 and 7.0.1 releases.

Apologies for the inconvenience.

@kevinchalet
Copy link
Member

kevinchalet commented Dec 15, 2022

Hopefully dotnet/arcade#11869 will be eventually merged, but in the meantime, should we include the same fix in our local copy of Arcade? (if so, we'll need to remember not to undo this change when we update Arcade... 😢)

@martincostello
Copy link
Member

Sounds like a good idea 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

SuperOffice provider: UserInfo endpoint doesn't support multiple subdomains
4 participants