APIScan | Fix Builder Mistake

[benrr101]

Description: Made a mistake when I wrote #3354 - forgot to store the modified builder in the builder variable.

Testing: Kinda disappointed it didn't get caught in any CI pathways. Ideally I should write up a test for this specific scenario, but I'm concerned this will be a huge undertaking versus fixing it before it gets released.

[Copilot]

Pull Request Overview

This PR corrects the builder pattern in CreateClientAppInstance by ensuring the modified builder is stored back into the builder variable for the Win32 window scenario.

• Assigns the result of WithParentActivityOrWindow back to builder
• Prevents the lost configuration when _iWin32WindowFunc is not null

Comments suppressed due to low confidence (1)

src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/ActiveDirectoryAuthenticationProvider.cs:559

• Add a unit test to verify that when _iWin32WindowFunc is provided, WithParentActivityOrWindow is actually called and the returned builder is used, ensuring this configuration isn't lost.

builder = builder.WithParentActivityOrWindow(_iWin32WindowFunc);

[mdaigle]

I don't think we have any test coverage of UI based app usage and it's hard to do in an automated way. But as far as I know, this is actually the only bit of code that's specific to UI based apps. What do you think about doing a manual verification?

[benrr101]

@mdaigle If I had any idea how to do that ... sure

[mdaigle]

The simplest repro would be a netfx winforms app. Visual studio can create one for you from a template.
If you add some code to create a sql connection using interactive app and pass in the window handle, the interactive auth window should pop up on top of the winforms app window when run. I have some sample projects from when I was doing the MSAL s360 work that I can share with you that should work as-is for this purpose.

[codecov]

Codecov Report

Attention: Patch coverage is 0% with 1 line in your changes missing coverage. Please review.

Project coverage is 67.72%. Comparing base (6d2473d) to head (9b39d4f).
Report is 2 commits behind head on main.

Files with missing lines	Patch %	Lines
...SqlClient/ActiveDirectoryAuthenticationProvider.cs	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3357      +/-   ##
==========================================
+ Coverage   65.10%   67.72%   +2.61%     
==========================================
  Files         300      294       -6     
  Lines       65376    65066     -310     
==========================================
+ Hits        42565    44065    +1500     
+ Misses      22811    21001    -1810     

Flag	Coverage Δ
addons	?
netcore	72.16% <ø> (+3.83%)	⬆️
netfx	66.37% <0.00%> (+0.03%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[paulmedynski]

Is this actually necessary? Chaining typically returns the same object the method was called on, not a new object. Pretty sure WithParentActivityOrWindow() is setting internal state on builder and then returning builder. I'd be very surprised if it was returning a different instance of a Builder.

The API docs don't say either way, but the source code returns this:

https://github.com/AzureAD/microsoft-authentication-library-for-dotnet/blob/428838e8ac9588e34c115e30e410e66855700b52/src/client/Microsoft.Identity.Client/AppConfig/PublicClientApplicationBuilder.cs#L287

I think your original change was fine (i.e. it worked) and maybe that's why there were no test failures. This might be slightly more robust, but I doubt MSAL will change its chaining behaviour, and break a bunch of calling code.

[cheenamalhotra]

Because config is local to builder creation, I think it is necessary to store the updated config in the builder instance.

You can validate the builder assignment need by updating another config that overrides something else that can be validated easily in a standalone console app, where you can directly work with MSAL APIs for testing the theory.

[benrr101]

Backing PR out since as per discussion w/ @paulmedynski the builder just updates it's internal state and returns itself. No need to store the output of any With* call since the builder is modified internally.
Although both storing and not storing are valid, why have an extra PR for something you don't need?