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.

Sign up for GitHub

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

Jump to bottom

id web extension updates #3100

Merged
merged 9 commits into from
Oct 24, 2024
Merged

id web extension updates #3100

merged 9 commits into from
Oct 24, 2024

Conversation

trwalke
Copy link
Member

@trwalke trwalke commented Oct 23, 2024 •
edited
Loading

Updating extension

Work left (addressing PR feedback)

  • Rename the TokenAcquisitionAddInOptions to TokenAcquisitionExtensionOptions
  • Rename the tokenAcquisitionAddInOptionsMonitor variable to tokenAcquisitionExtensionOptionsMonitor, and addInOptions to tokenAcquisitionExtensionOptions in TokenAcquisition.cs
  • Split the delegages/types in the current file TokenAcquisitionAddIn.cs to their own files.
  • For the moment (until we add more extensions), add the events that are not used by the product in a #if FUTURE.
  • Create a better end to end test (using the ExtraParameters, with extension methods to set a Prefix), and adding a prefix in front of the acquired token?

@trwalke trwalke mentioned this pull request Oct 23, 2024
Merged
5 tasks
jmprieur
jmprieur approved these changes Oct 23, 2024
View reviewed changes
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 so far.
This could be a mergeable increment already

I've asked a couple of questions

src/Microsoft.Identity.Web.TokenAcquisition/TokenAcquisitionExtensionOptions.cs Outdated Show resolved Hide resolved
src/Microsoft.Identity.Web.TokenAcquisition/TokenAcquisitionExtensionOptions.cs Outdated Show resolved Hide resolved
trwalke
trwalke commented Oct 23, 2024
View reviewed changes
tests/Microsoft.Identity.Web.Test/TokenAcquisitionAddInTests.cs
[Fact]
public void InvokeOnBeforeTokenAcquisitionForApp_InvokesEvent()
public async Task InvokeOnBeforeTokenAcquisitionForApp_InvokesEvent()
Copy link
Member Author

@trwalke trwalke Oct 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Had a hard time mocking MSAL components directly with NSubstitute because of MSAL's implementation. Using the Http mocking infrastructure instead.

@trwalke trwalke marked this pull request as ready for review October 23, 2024 16:29
@trwalke trwalke requested a review from a team as a code owner October 23, 2024 16:29
msbw2
msbw2 reviewed Oct 23, 2024
View reviewed changes
Directory.Build.props Outdated Show resolved Hide resolved
@jennyf19
Copy link
Collaborator

I'm surprised there is no API file to update?

@trwalke
Copy link
Member Author

trwalke commented Oct 24, 2024 •
edited
Loading

I'm surprised there is no API file to update?

@jennyf19 I think the extension is added with the new api here https://github.com/AzureAD/microsoft-identity-web/pull/3100/files#diff-811295c0de57c7f8e1b4e4d60c4151ea7de5b527bc12b22796a93ba377ab03e0

Unless I misunderstand what an api file is?

@trwalke trwalke requested review from msbw2 and jennyf19 October 24, 2024 16:47
jennyf19
jennyf19 approved these changes Oct 24, 2024
View reviewed changes
Copy link
Collaborator

@jennyf19 jennyf19 left a comment

Choose a reason for hiding this comment

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

:shipit:

@trwalke trwalke merged commit 7942f6b into jmprieur/IdWebAddIns Oct 24, 2024
1 check passed
@trwalke trwalke deleted the trwalke/IdWebExtensionUpdates branch October 24, 2024 20:15
jmprieur added a commit that referenced this pull request Oct 25, 2024
@jmprieur @jennyf19 @trwalke
Microsoft.Identity.Web token acquisition extensions (#3005)
50bdedf 
* First cut or proto for IdWeb add-ins

* Preparing work for CDT

* fix clone issue

* remove cdt tests

* remove test

* remove endProject

* fix sln

* Apply suggestions from code review

fix sln

* Updarting to MSAL.NET 4.65.2-preview
and fixing Travis's change

* Update NuGet.Config

* Update NuGet.Config

* Update NuGet.Config

* Update NuGet.Config

* Updating the signature of the delegates

* Update to use MSAL.NET preview

* Fixing a cloning issue

* use msal preview version

* id web extension updates (#3100)

* WIP Refactoring code

* Updating test

* Updating test

* Clean up. Removing future code

* Creating partial class

* PR Feedback

* internal api updates

* Update public api

---------

Co-authored-by: trwalke <trwalke@microsoft.com>

* Fix Unshipped files for TokenAcquisition

---------

Co-authored-by: Jenny Ferries <jeferrie@microsoft.com>
Co-authored-by: Travis Walker <travis.walker@microsoft.com>
Co-authored-by: trwalke <trwalke@microsoft.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@westin-m westin-m westin-m left review comments

@jennyf19 jennyf19 jennyf19 approved these changes

@jmprieur jmprieur jmprieur approved these changes

@msbw2 msbw2 Awaiting requested review from msbw2

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@trwalke @jennyf19 @jmprieur @westin-m @msbw2