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

Add Identity/Auth to template #300 #487

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

ahmedanwar100
Copy link

Jwt Authentication added with default Microsoft Identity

@ahmedanwar100
Copy link
Author

Hi @ardalis, can you please review it

Copy link
Owner

@ardalis ardalis left a comment

Choose a reason for hiding this comment

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

Overall looks great, thanks!

Only request is to don't put Identity stuff in Core (instead you can reference it using a UserId property on an appropriate entity, like in our case perhaps Contributor) and also keep IdentityDbContext separate from the regular/existing AppdbContext. Can you make those changes and I'll merge?

//We can use EncryptingCredentials options in SecurityTokenDescriptor and hence our JWT token will not be parsed by jwt.io site and it will only be decrypted only by our code.
//Hence you can secure your token and who can see it
var encryptionKey = Encoding.UTF8.GetBytes(_siteSetting.JwtSettings.EncryptKey); //must be 16 character
var encryptingCredentials = new EncryptingCredentials(new SymmetricSecurityKey(encryptionKey), SecurityAlgorithms.Aes128KW, SecurityAlgorithms.Aes128CbcHmacSha256);

Check warning

Code scanning / CodeQL

Useless assignment to local variable

This assignment to [encryptingCredentials](1) is useless, since its value is never read.
{
return async context =>
{
var signInManager = context.HttpContext.RequestServices.GetRequiredService<SignInManager<User>>();

Check warning

Code scanning / CodeQL

Useless assignment to local variable

This assignment to [signInManager](1) is useless, since its value is never read.
Comment on lines 78 to 81
catch
{
return null;
}

Check notice

Code scanning / CodeQL

Generic catch clause

Generic catch clause.
// context.Fail("This token has no security stamp");

//Get UserId and check if user exists in db
var userId = claimsIdentity.GetUserId<int>();

Check warning

Code scanning / CodeQL

Dereferenced variable may be null

Variable [claimsIdentity](1) may be null at this access because of [this](2) assignment.
{
if (request is null)
ThrowError("Request body is empty");
var user = await _userManager.FindByEmailAsync(request.Email); //userName/email can be used to find a unique user

Check warning

Code scanning / CodeQL

Dereferenced variable may be null

Variable [request](1) may be null at this access as suggested by [this](2) null check.
ThrowError("username or password is incorrect");

var jwt = await _jwtService.GenerateAsync(user);
user.LastLoginDate = DateTime.UtcNow;

Check warning

Code scanning / CodeQL

Dereferenced variable may be null

Variable [user](1) may be null at this access as suggested by [this](2) null check.
//TODO:CHECK FOR USERNAME EMPTY OR NULL
var newUser = new User
{
Age = request.Age,

Check warning

Code scanning / CodeQL

Dereferenced variable may be null

Variable [request](1) may be null at this access as suggested by [this](2) null check.
@ahmedanwar100
Copy link
Author

ahmedanwar100 commented Apr 13, 2023 via email

@ahmedanwar100
Copy link
Author

ahmedanwar100 commented Apr 13, 2023 via email

@ardalis
Copy link
Owner

ardalis commented Apr 13, 2023

No rush, thanks

@ahmedanwar100
Copy link
Author

ahmedanwar100 commented Apr 13, 2023 via email

@ahmedanwar100
Copy link
Author

@ardalis can you please review it

});
if (!addRoleResult.Succeeded) { ThrowValidationErrors(addRoleResult.Errors); }
}
else

Choose a reason for hiding this comment

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

This else does not look like it is necessary.

If I read correctly, you check to see if there is a role by that name already, if it doesn't you create it and only if it does exist do you then assign it to that user in this else clause.

Just removing the else around the inner code looks like it will solve the problem.

Copy link

@DerekChasse DerekChasse left a comment

Choose a reason for hiding this comment

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

Like I mentioned in one of my comments, I've essentially used the code in this pull request to implement Identity in my application which is based on the Clean Arch Template.

I can speak to the fact that the code written does in fact work, so there's that. 😄

For what it's worth, I moved the custom IdentityUser and IdentityRole implementations to the core project instead of the Infrastructure project. To do that, I needed to take a dependency on Microsoft.Extensions.Identity.Stores in the core project. In my eyes, that was a worth tradeoff allowing me to use the existing Event and Handler frameworks for these entities too. I did try to do things within the Infrastructure project but it was never as clearly defined as I wanted it to be.

@ardalis , I'll let you give the final thumbs up, but this is what I can share at least.

I suppose that this will lead to the inevitable ask for clarification on how to connect these two concepts. Tenancy and AppDatabase entity ownership etc...

/// </summary>
internal class JwtJwtBearerHelperEvents
{
public JwtBearerEvents GetJwtBeareEvents()

Choose a reason for hiding this comment

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

Nit: Missing 'r' in Bearer.

OnChallenge = OnOnChallengeJwtBearerEvent()
};
}
private Func<AuthenticationFailedContext, Task> OnAuthenticationFailedJwtBearerEvent()

Choose a reason for hiding this comment

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

Nit: Not sure what the 'JwtBearerEvent' suffix adds to this, I'd drop it for simplification.

@@ -0,0 +1,382 @@
// <auto-generated />
Copy link

@DerekChasse DerekChasse May 20, 2023

Choose a reason for hiding this comment

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

Are these migrations necessary? For what it's worth, I've essentially used this pull request to bootstrap my applications and I do not use Migrations. Will these cause problems or confusion for new users of the template?

@ahmedanwar100
Copy link
Author

@ardalis, any plan to review it 😁

@ardalis
Copy link
Owner

ardalis commented Sep 1, 2023

My plan is still to pull this into the new template - just need to find some time.

@ahmedanwar100
Copy link
Author

Okay, then please find some time 🙂

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