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

Auth Cookie Sharing topic updates #13195

Merged
merged 4 commits into from
Jul 15, 2019
Merged

Conversation

guardrex
Copy link
Collaborator

@guardrex guardrex commented Jul 8, 2019

Addresses #5495
Fixes #12432

Internal Review Topic

  • No rush on review.
  • This would have been a 2.2 pass to get the samps updated, BUT...
  • Let's drop the samples! Plz! 😄 The long-range plan is to have the ones in engineering cross-linked in here anyway. These samps, especially the NETFX sample, are a pain to maintain. The upgrade of that samp when I did upgrades here for Core 2.0 to 2.1 went fine. However, it just broke again when trying to update it to 2.2 😝 ... like it did back when I did the 1.x to 2.0 round of updates. If you have the engineering link to shared cookie sample apps, I'll place it in here. Even if not, see if this version of the topic without the samples works right now, and we can put the engineering samples link in here later.
  • Let's go ahead and fix Confusion about "Use a common user database" for ASP.NET Core + ASP.NET 4.x case Confusion about "Use a common user database" for ASP.NET Core + ASP.NET 4.x case #12432 ... Can you provide the line for the Use a common user database section that makes sense? I think this came up once before, and the answer might have been no 🎲🎲 - not going to happen on a shared dB given the alternate Identity schemas. How would u like to address it?

@HaoK
Copy link
Member

HaoK commented Jul 8, 2019

Cookie interop samples/tests got dropped from 3.0 branches entirely, so yeah best to just remove them from now. The 2.2 source should still work for the most part

@guardrex
Copy link
Collaborator Author

guardrex commented Jul 8, 2019

@HaoK Sounds good ... much better than occasionally battling with the NETFX sample.

This can be reviewed anytime ... no rush. It's ready whenever u are.

@HaoK
Copy link
Member

HaoK commented Jul 8, 2019

The schemas for Core and 4.x should not be too different, but to share a database, the newest version of identity should 'own' the database, and all older versions of EF should add migrations to account for any schema changes. @ajcvickers is that an apt description of what the recommendation approach for having multiple versions of identity target the same shared database?

@guardrex
Copy link
Collaborator Author

guardrex commented Jul 9, 2019

@HaoK I'm confused on how migrations would address the scenario for using the common dB. It seems like Data Annotations or Fluent API (with an IdentityUser class in the app) would allow the dev to map alternative columns for the earlier-version Identity app to the newer-version Identity common dB.

This isn't a scenario that I've faced professionally, so I'll definitely hold for a sec to hear from you/Arthur on the right way to address it.

@HaoK
Copy link
Member

HaoK commented Jul 9, 2019

Perhaps migrations aren't the right word to use. Here's a link to the old POCO classes that were needed to adapt the old 4.x identity to work against identity core's schema. Basically the idea was adding properties to the POCO to match the identity core schema, but the older identity frameworks would just ignore these columns

https://github.com/aspnet/Identity/tree/master/src/AspNetCoreCompat

@guardrex
Copy link
Collaborator Author

guardrex commented Jul 9, 2019

adding properties to the POCO to match the identity core schema

That's what I was thinking (guessing). Something like this ...

  • DA or Fluent should be able to remap where needed if the names are merely different. The older-identity app needs an IdentityUser class in the app in order to apply annotations. (With Fluent tho, would that even be required?)
  • For any columns in the common dB that only support the newer-Identity app, the older-Identity app will ignore them.
  • For any columns in the common dB that only support the older-Identity app, the newer-Identity app ignores them.
  • WRT to the use of migrations on either side, migrations ignore columns not part of that's app's model schema.

@guardrex
Copy link
Collaborator Author

guardrex commented Jul 9, 2019

I was just curious to see the deltas (Core 3.0.0-preview6.19307.2 vs. NETFX 2.2.2) ...

IdentityUser from Microsoft.Extensions.Identity.Stores ...

public virtual string SecurityStamp { get; set; }
public virtual bool PhoneNumberConfirmed { get; set; }
public virtual string PhoneNumber { get; set; }
public virtual string PasswordHash { get; set; }
public virtual string NormalizedUserName { get; set; }
public virtual string NormalizedEmail { get; set; }
public virtual DateTimeOffset? LockoutEnd { get; set; }
public virtual bool LockoutEnabled { get; set; }
public virtual TKey Id { get; set; }
public virtual bool EmailConfirmed { get; set; }
public virtual string Email { get; set; }
public virtual string ConcurrencyStamp { get; set; }
public virtual int AccessFailedCount { get; set; }
public virtual bool TwoFactorEnabled { get; set; }
public virtual string UserName { get; set; }

IdentityUser from Microsoft.AspNet.Identity.EntityFramework ...

public virtual string Email { get; set; }
public virtual bool EmailConfirmed { get; set; }
public virtual string PasswordHash { get; set; }
public virtual string SecurityStamp { get; set; }
public virtual string PhoneNumber { get; set; }
public virtual bool PhoneNumberConfirmed { get; set; }
public virtual bool TwoFactorEnabled { get; set; }
public virtual DateTime? LockoutEndDateUtc { get; set; }
public virtual bool LockoutEnabled { get; set; }
public virtual int AccessFailedCount { get; set; }
public virtual ICollection<TRole> Roles { get; }
public virtual ICollection<TClaim> Claims { get; }
public virtual ICollection<TLogin> Logins { get; }
public virtual TKey Id { get; set; }
public virtual string UserName { get; set; }

I can see why this gets a bit 😵 ... and roles and claims are handled differently, which makes it even more challenging.

The doc challenge here is that a few sentences that describe the overall scenario probably won't be all that helpful. Is it a viable remark to say that in many scenarios it's better to upgrade older apps and use the newer Identity everywhere? ... that long-term it's actually easier to do that than to build all of this infrastructure and maintain it.

@HaoK
Copy link
Member

HaoK commented Jul 9, 2019

Yeah I don't think we've gotten more than one or two requests asking about this either, so I don't think its something many people are trying to do

aspnetcore/security/cookie-sharing.md Outdated Show resolved Hide resolved
aspnetcore/security/cookie-sharing.md Outdated Show resolved Hide resolved
@guardrex
Copy link
Collaborator Author

@Rick-Anderson Updated and ready to go.

@guardrex guardrex merged commit 1ea9fda into master Jul 15, 2019
@delete-merged-branch delete-merged-branch bot deleted the guardrex/cookie-sharing-2-2 branch July 15, 2019 20:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Confusion about "Use a common user database" for ASP.NET Core + ASP.NET 4.x case
3 participants