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

Fixed writing accesses that bypass manager classes #303

Merged

Conversation

gingters
Copy link
Contributor

In our custom IdentityServer implementation, we need to create events for every change on the users and roles.

For that, we decorated the UserStore<TUser> of the AspNetCore Identity model and create our events on every write access. The AspNetCore.Identities UserManager and RoleManager both use the IUserStore and IRoleStore for write access exclusively, so that is the only good place to intercept changes to the users and roles.

In your IIdentityRepository you use the manager classes to create user claims and role claims, but for deleting these you bypass the UserManager / RoleManager classes and use the IdentityDbContext directly to delete UserClaims and RoleClaims.

This pull request changes this to also use the manager classes (and thus internally the Store) to write these changes to the database, so that we are capable of intercepting them and creating the required events when a user or a role changes.

@skoruba skoruba changed the base branch from master to dev June 27, 2019 12:00
@skoruba skoruba self-requested a review June 27, 2019 12:01
@maulik-modi
Copy link

@skoruba , I am also looking for this functionality to capture events. Can you approve the merge?

@skoruba
Copy link
Owner

skoruba commented Oct 5, 2019

Sure, please take a look at my comments - is it necessary to use ConfigureAwait(false).
cc @TomasHubelbauer

@maulik-modi
Copy link

I think answer is it depends -

If you have code in a library that may also run in a UI app, or legacy ASP.NET app, or anywhere else there may be a context, then you should still use ConfigureAwait(false) in that library.

https://blog.stephencleary.com/2017/03/aspnetcore-synchronization-context.html

@skoruba
Copy link
Owner

skoruba commented Oct 5, 2019

OK, but is it required in this case? :)

@maulik-modi
Copy link

I do not think Identity server Admin libraries being used outside Web API or Server side MVC. Hence, ConfigureAwait is unnecessary.

@skoruba
Copy link
Owner

skoruba commented Oct 7, 2019

I have same opinion. Can you please @gingters remove usage of ConfigureAwait(false)? - or I can do it manually. 😊
Thanks guys!

Copy link
Owner

@skoruba skoruba left a comment

Choose a reason for hiding this comment

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

I will merge it and remove ConfigureAwait manually in next commit.

var userClaim = await DbContext.Set<TUserClaim>().Where(x => x.Id == claimId).SingleOrDefaultAsync();

DbContext.UserClaims.Remove(userClaim);
await UserManager.RemoveClaimAsync(user, new Claim(userClaim.ClaimType, userClaim.ClaimValue)).ConfigureAwait(false);
Copy link
Owner

Choose a reason for hiding this comment

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

@TomasHubelbauer - Is it required to use ConfigureAwait(false) here and a few lines bellow? :)
Thanks!

Copy link
Owner

Choose a reason for hiding this comment

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

@gingters - Why do you use ConfigureAwait(false)? :)
Thanks.

@skoruba skoruba merged commit e698633 into skoruba:dev Oct 20, 2019
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.

3 participants