Skip to content

Update IdentityServer4 -> Duende #33590

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
Merged
merged 15 commits into from
Jul 13, 2021
Merged

Update IdentityServer4 -> Duende #33590

merged 15 commits into from
Jul 13, 2021

Conversation

@HaoK
Copy link
Member

@HaoK HaoK commented Jun 16, 2021 •
edited
Loading

Fixes #32881

HaoK
HaoK commented Jun 16, 2021
View reviewed changes
...Authorization.IdentityServer/src/Microsoft.AspNetCore.ApiAuthorization.IdentityServer.csproj
<Reference Include="Microsoft.AspNetCore.Identity.EntityFrameworkCore" />
<Reference Include="Microsoft.AspNetCore.Identity.UI" />
<Reference Include="Newtonsoft.Json" />
<SuppressBaselineReference Include="IdentityServer4.AspNetIdentity" />
Copy link
Member Author

@HaoK HaoK Jun 16, 2021

Choose a reason for hiding this comment

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

@aspnet-build is this something I should be checking in (SupressBaselineReference?)

Copy link
Contributor

@dougbu dougbu Jul 3, 2021

Choose a reason for hiding this comment

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

It's alright if the pseudo-breaking change is approved. I'm a bit more concerned about having a shipping package depend on an unsigned assembly. Thoughts @Pilchie❔

Copy link
Member

@wtgodbe wtgodbe Jul 6, 2021

Choose a reason for hiding this comment

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

This would be my only concern as well. I couldn't find other instances of shipping projects doing this in our repo or dotnet/runtime

Copy link
Member Author

@HaoK HaoK Jul 6, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

@blowdart seems to be okay with this, is there anyone else we should ping about this? To clarify its signed, but doesn't have a strong name, poor choice in wording by me

Copy link
Contributor

@leastprivilege leastprivilege Jul 7, 2021

Choose a reason for hiding this comment

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

Our Nuget package is signed. The assemblies do not have a strong name. What would be the benefit of that for a library targeting .NET Core consumers only?

@Pilchie Pilchie added the area-identity Includes: Identity and providers label Jun 17, 2021
@HaoK HaoK changed the title [WIP] Update to Duende Update to IdentityServer4 -> Duende Jul 3, 2021
@HaoK HaoK changed the title Update to IdentityServer4 -> Duende Update IdentityServer4 -> Duende Jul 3, 2021
@HaoK HaoK requested review from a team, blowdart and javiercn July 3, 2021 05:18
@HaoK HaoK marked this pull request as ready for review July 3, 2021 05:18
@HaoK HaoK requested review from Pilchie and dougbu as code owners July 3, 2021 05:18
dougbu
dougbu reviewed Jul 3, 2021
View reviewed changes
Copy link
Contributor

@dougbu dougbu left a comment

Choose a reason for hiding this comment

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

Also, why are you adding the empty src/Identity/ApiAuthorization.IdentityServer/src/ApiAuthorizationDbContext.cs file❔

...Authorization.IdentityServer/src/Microsoft.AspNetCore.ApiAuthorization.IdentityServer.csproj
<Reference Include="Microsoft.AspNetCore.Identity.EntityFrameworkCore" />
<Reference Include="Microsoft.AspNetCore.Identity.UI" />
<Reference Include="Newtonsoft.Json" />
<SuppressBaselineReference Include="IdentityServer4.AspNetIdentity" />
Copy link
Contributor

@dougbu dougbu Jul 3, 2021

Choose a reason for hiding this comment

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

It's alright if the pseudo-breaking change is approved. I'm a bit more concerned about having a shipping package depend on an unsigned assembly. Thoughts @Pilchie❔

src/Identity/ApiAuthorization.IdentityServer/src/Data/ApiAuthorizationDbContext.cs
/// <value>
/// The keys.
/// </value>
public DbSet<Key> Keys { get; set; }
Copy link
Contributor

@dougbu dougbu Jul 3, 2021

Choose a reason for hiding this comment

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

I see a couple of new DbSet<T> properties in this PR but haven't noticed new references to them. What are they for❔

Copy link
Member Author

@HaoK HaoK Jul 3, 2021

Choose a reason for hiding this comment

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

Yeah it comes from a new requirement from the updated version of IPersistedGrantDbContext, which now store Keys, I'm not sure exactly what they are used for, but its part of the contract for IdentityServer Stores

@blowdart
Copy link
Contributor

blowdart commented Jul 6, 2021

Which one is the unsigned package @dougbu mentions?

@HaoK
Copy link
Member Author

HaoK commented Jul 6, 2021

@blowdart all of the identity server packages are unsigned, by design it appears: DuendeArchive/IdentityServer4#1653

@blowdart
Copy link
Contributor

blowdart commented Jul 6, 2021

Oh strong named rather than signed. Yea not worried then considering it has no security benefits, and it's "only" a dependency rather than shipping as part of the product.

@HaoK HaoK merged commit c754959 into main Jul 13, 2021
@HaoK HaoK deleted the haok/duende branch July 13, 2021 19:23
@ghost ghost added this to the 6.0-preview7 milestone Jul 13, 2021
HaoK added a commit that referenced this pull request Jul 13, 2021
@HaoK
Revert "Update IdentityServer4 -> Duende (#33590)"
f5faef6 
This reverts commit c754959.
@dougbu dougbu mentioned this pull request Jul 13, 2021
Merged
HaoK added a commit that referenced this pull request Jul 14, 2021
@HaoK
Revert "Update IdentityServer4 -> Duende (#33590)" (#34317)
c9c9a9e 
This reverts commit c754959.
HaoK added a commit that referenced this pull request Jul 14, 2021
@HaoK
Revert "Revert "Update IdentityServer4 -> Duende (#33590)" (#34317)"
5c3b89a 
This reverts commit c9c9a9e.
HaoK added a commit that referenced this pull request Jul 19, 2021
@HaoK
Revert "Revert "Update IdentityServer4 -> Duende (#33590)" (#34317)"
ace37a6 
This reverts commit c9c9a9e.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@wtgodbe wtgodbe wtgodbe left review comments

@javiercn javiercn javiercn approved these changes

@blowdart blowdart Awaiting requested review from blowdart

@Pilchie Pilchie Awaiting requested review from Pilchie

+4 more reviewers

@bricelam bricelam bricelam left review comments

@leastprivilege leastprivilege leastprivilege left review comments

@dougbu dougbu dougbu left review comments

@behzad888 behzad888 behzad888 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

area-identity Includes: Identity and providers

Projects

None yet

Milestone

6.0-preview7

Development

Successfully merging this pull request may close these issues.

Update IdentityServer to Duende IdentityServer for .NET 6

10 participants

@HaoK @blowdart @Pilchie @bricelam @leastprivilege @dougbu @javiercn @behzad888 @wtgodbe