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

[Bug] Microsoft.Identity.Client.Broker has package/targets authoring error for .NET Framework #4108

Closed
1 task done
mjcheetham opened this issue Apr 26, 2023 · 0 comments
Closed
1 task done

[Bug] Microsoft.Identity.Client.Broker has package/targets authoring error for .NET Framework #4108

mjcheetham opened this issue Apr 26, 2023 · 0 comments
Assignees
@gladjohn
Labels
bug GCM P2 scenario:Desktop
Milestone
4.54.0

Comments

@mjcheetham
Copy link
Contributor

mjcheetham commented Apr 26, 2023 •
edited
Loading

Which version of MSAL.NET are you using?
4.52.0

Platform
.NET 4.7.2

What authentication flow has the issue?

  • N/A

Other?

Is this a new or existing app?
The app is in production, and I have upgraded to a new version of MSAL.

Repro
See minimal example here: https://github.com/mjcheetham/msal-broker-package-error

dotnet new classlib --target-framework-override net472 --langVersion latest -n library
dotnet new console --target-framework-override net472 --langVersion latest -n app
dotnet add library package Microsoft.Identity.Client.Broker --version 4.52.0
dotnet add app reference library
dotnet build app

Expected behavior
dotnet build is successful.

Actual behavior
Build error:

/Users/mjcheetham/.nuget/packages/microsoft.identity.client.nativeinterop/0.13.7/buildtransitive/net461/Microsoft.Identity.Client.NativeInterop.targets(6,3): error : Microsoft.Identity.Client.Broker does not support applications which rely on the older package.config. You must use PackageReference [/Users/mjcheetham/scratch/msal-broker-error/app/app.csproj]

The Microsoft.Identity.Client.NativeInterop.targets file has an authoring error:

<!-- No support for older .NET projects that use package.config -->
<Target Name="PackageReferenceCheck" BeforeTargets="Build">
    <Error Text="Microsoft.Identity.Client.Broker does not support applications which rely on the older package.config. You must use PackageReference" Condition="'@(PackageReference)' == ''" />
</Target>

The condition '@(PackageReference)' == '' is trivially true on any projects that have zero package references (whether PackageReference or not), and also reference, transitively, MS.ID.Client.Broker.

Possible solution
Replace the erroneous Condition with one that also permits projects without any dependencies.

Additional context / logs / screenshots / links to code
See the minimal repro here: https://github.com/mjcheetham/msal-broker-package-error

cc @bgavrilMS @ldennington
@mjcheetham mjcheetham changed the title [Bug] Microsoft.Identity.Client.Broker has package/targets authoring error [Bug] Microsoft.Identity.Client.Broker has package/targets authoring error for .NET Framework Apr 26, 2023
@bgavrilMS bgavrilMS added P2 and removed P1 labels Apr 27, 2023
@bgavrilMS bgavrilMS added this to the 4.54.0 milestone Apr 27, 2023
@ldennington ldennington mentioned this issue Apr 27, 2023
Merged
ldennington added a commit to git-ecosystem/git-credential-manager that referenced this issue May 2, 2023
@ldennington
version: replace nerdbank with version file
6ab469f 
Remove Nerdbank.GitVersioning dependency from project in favor of a new
VERSION file, which will be updated ahead of each release. Versioning in
this file will begin with the version we plan to use for the next release:
2.1.0.0.

This change involves the addition of a new custom MSBuild task, which
reads in the contents of the VERSION file, converts it to a Version
object, and then sets the various version-related MSBuild properties with
the correct value (some with the `Revision` component appended, others
without).

Note that there is a bug in MSAL [1] that causes build failures for
projects without dependencies with this change. We add Newtonsoft.Json as
a global dependency in Directory.Build.props to work around this problem
until the fix is released.

[1]: AzureAD/microsoft-authentication-library-for-dotnet#4108
ldennington added a commit to git-ecosystem/git-credential-manager that referenced this issue May 2, 2023
@ldennington
versioning: transition from nerdbank to version file (#1231)
1f7212e 
With the [plan to migrate GCM to a formula for release via
`Homebrew/homebrew-core`](#1102),
it became clear that
[Nerdbank.GitVersioning](https://github.com/dotnet/Nerdbank.GitVersioning)
was no longer an option for versioning the project. This is because
`Nerdbank.GitVersioning` requires history to calculate things like ['Git
height'](https://github.com/dotnet/Nerdbank.GitVersioning#what-is-git-height),
and the formula requires a tarball to build, which, of course, has no
history.

This change pivots GCM to the simpler strategy of using a version
specified in a `VERSION` file at root. This version will be updated by
maintainers prior to release - giving them more granular control of
versioning, which in turn allows for versioning to be more predictable
(i.e. maintainers will know what the version will be before publication
of the release).

The version specified in the file is the one slated for the next
release: `2.1.0.0`.

**Note:** This change [fails on
Windows](https://github.com/ldennington/git-credential-manager/actions/runs/4824686907)
due to a [bug in
MSAL](AzureAD/microsoft-authentication-library-for-dotnet#4108)
unless we ensure all projects have at least 1 dependency. We are working
around this issue by adding Newtonsoft.Json as this dependency (since we
were already shipping it anyway).
@github-project-automation github-project-automation bot moved this from Triage to Fixed in MSAL Customer Trust / QM May 2, 2023
ldennington added a commit to ldennington/git-credential-manager that referenced this issue May 31, 2023
@ldennington
dependencies: update json.net to system.text.json
03b4234 
Update the current Json.NET dependency to System.Text.Json. Note that this
is required only for the following reasons:

- .NET Framework needs the package [1]
- We need a global dependency to work around [2] until the fix [3] is
released.

1: https://learn.microsoft.com/en-us/dotnet/standard/serialization/system-text-json/overview#how-to-get-the-library
2: AzureAD/microsoft-authentication-library-for-dotnet#4108
3: AzureAD/microsoft-authentication-library-for-cpp#3522
@ldennington ldennington mentioned this issue Jun 2, 2023
Merged
ldennington added a commit to git-ecosystem/git-credential-manager that referenced this issue Jun 6, 2023
@ldennington
dependencies: migrate from json.net to system.text.json (#1274)
b550293 
# Overview

Migrate GCM from JSON.NET to `System.Text.Json`.

The first commit updates to .NET 7.0 to take advantage of certain
features like `JsonRequired`. The second updates
`Microsoft.Identity.Client` so that the workaround for [this
issue](AzureAD/microsoft-authentication-library-for-dotnet#4108
) is no longer necessary. The third updates the current global Json.NET
dependency to `System.Text.Json` and ensures it is only used for .NET
Framework. The fourth adds a custom converter to handle Bitbucket's use
of the non-standard 'scopes' property to provide token endpoint results.
The fifth through ninth commits update `Trace2Message`, the Bitbucket
REST API, the GitHub REST API, the Azure DevOps API tests, and
OAuth-specific code to use `System.Text.Json` instead of Json.NET for
JSON serialization/deserialization.

# Performance comparison
Comparison runs were done using
[`dotnet-trace`](https://learn.microsoft.com/en-us/dotnet/core/diagnostics/dotnet-trace)
with the command `printf "protocol=https\nhost=github.com" |
git-credential-manager get`. The machine used was a Mac with a 2.4 GHz
8-Core Intel Core i9 processor running Ventura 13.4. Times are in
milliseconds.

## Individual Run Times
| **Newtonsoft.json** | **System.Text.Json** |
|---------------------|----------------------|
| 364.08              | 304.92               |
| 366.11              | 305.23               |
| 377.31              | 313.09               |
| 381.3               | 313.51               |
| 373.25              | 297.71               |
| 373.98              | 312.78               |
| 370.84              | 317.86               |
| 368.25              | 311.78               |
| 369.79              | 307.69               |

## Run Summary
|             | **Newtonsoft.json** | **System.Text.Json** |
|-------------|---------------------|----------------------|
| **Average** | 371.66          | 309.40          |
| **Median**  | 370.84              | 311.78               |

## Analysis
Per the above, the transition to `System.Text.Json` and .NET 7.0
decreased end-to-end execution times by about 17% on average.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@gladjohn gladjohn

Labels
bug GCM P2 scenario:Desktop
Projects
MSAL Customer Trust / QM
Archived in project
Milestone
4.54.0
Development

No branches or pull requests
3 participants
@mjcheetham @bgavrilMS @gladjohn