search

JonPSmith / AuthPermissions.AspNetCore

This library provides extra authorization and multi-tenant features to an ASP.NET Core application.
https://www.thereformedprogrammer.net/finally-a-library-that-improves-role-authorization-in-asp-net-core/
MIT License
806 stars 161 forks source link

Added option to specify the refresh token to disable #60

Closed idan-h closed 2 years ago

idan-h commented 2 years ago

Hey, I added just the option to disable specific token so it could support multiple logins at once. No database changes needed, All tests pass

JonPSmith commented 2 years ago

Hi @idan-h,

Can you explain how this works?

idan-h commented 2 years ago

If a refresh token is specified it disables the specified token. If not, it does exactly as before (the latest one).

It solves the problem because now you can log in from multiple places and specify the refresh token as you log off, so it disables only the correct refresh token (and won't log you out of the latest session only)

JonPSmith commented 2 years ago

That's interesting. You are saying that the only problem of multiple logins from one user is logging out.

I didn't think that is true, but looking at the RefreshTokenUsingRefreshTokenAsync AuthP code it uses the refresh token value to find the correct RefreshToken to update, which works (using the userId to find the RefreshToken wouldn't handle multiple logins).

However, to complete this you also need to:

NOTE: I am working on version 3.5.0 of AuthP in the dev branch, which only has a few changes. I should this to be ready in a few weeks, so I will merge your updated PR into the 33.5.0 before I release it. My changes won't effect your PR.

idan-h commented 2 years ago

That's interesting. You are saying that the only problem of multiple logins from one user is logging out.

I didn't think that is true, but looking at the RefreshTokenUsingRefreshTokenAsync AuthP code it uses the refresh token value to find the correct RefreshToken to update, which works (using the userId to find the RefreshToken wouldn't handle multiple logins).

However, to complete this you also need to:

  • Change the MarkJwtRefreshTokenAsUsedAsync to ONLY take in the user's refresh. That way it will support multiple logins from one user.
  • Update the Logout Web API in the Example2 AuthenticateController to take in the user's refresh value and call your changed MarkJwtRefreshTokenAsUsedAsync
  • Update the current TestMarkJwtRefreshTokenAsUsedAsync test to your new version.
  • Be good to add a test of logout with two logins of the same user.

NOTE: I am working on version 3.5.0 of AuthP in the dev branch, which only has a few changes. I should this to be ready in a few weeks, so I will merge your updated PR into the 33.5.0 before I release it. My changes won't effect your PR.

Sure, I'll add the example.

I think it is a good idea to leave the option to not specify a refresh token, so the consumer can choose if he wants to provide a refresh token for the logout or not (make it optional).

I already added the tests with 2 logins in the commit, you can check it out.

JonPSmith commented 2 years ago

Hi @idan-h,

Thanks for this. I close to releasing the new version and I have a few questions before I add your logout code:

Why do use [FromBody]LogoutModel model in controller

Your code changes the logout API uses [FromBody]LogoutModel model while the rest of the rest of the controller uses the default binding, which is Form fields, then the request body. I plan to remove the [FromBody] to match the other controller APIs.

I going to change the DisableJwtRefreshToken

A logout can ONLY happen if you are using a refresh token so I'm going to remove the userId as an option. That makes it clearer what it is doing and the code is really simple.

So, does that make sense to you? Let me know and I'll make the changes.

idan-h commented 2 years ago

Hi @idan-h,

Thanks for this. I close to releasing the new version and I have a few questions before I add your logout code:

Why do use [FromBody]LogoutModel model in controller

Your code changes the logout API uses [FromBody]LogoutModel model while the rest of the rest of the controller uses the default binding, which is Form fields, then the request body. I plan to remove the [FromBody] to match the other controller APIs.

I going to change the DisableJwtRefreshToken

A logout can ONLY happen if you are using a refresh token so I'm going to remove the userId as an option. That makes it clearer what it is doing and the code is really simple.

So, does that make sense to you? Let me know and I'll make the changes.

I used [FromBody] because it tried taking the variables from the query instead of the request body for some reason.. it does not matter if you change it as long as it works as intended.

You are right - logout happens only when there is a refresh token, but I don't know if refresh tokens are unique (you tell me), because it can create problems if they aren't and get rid of the user id. Another reason is backwards compatibility.

Generally, the changes are fine by me if the above things are ok.

JonPSmith commented 2 years ago

Thanks for the quick reply - the refresh token is created by a Cryptography.RandomNumberGenerator with 32 bytes which should be unique, but also the refresh token string is the primary key in the database, which will ensure it is unique.

The change is a breaking change, but its small and also fixes the multiple logged-in user's problem.

OK, I will add these changes and release the new version soon. Thanks for your help on this.

idan-h commented 2 years ago

For sure. Let me know if you need any help

JonPSmith commented 2 years ago

Hi @idan-h,

I have released version 3.5.0 with the JWT refresh token changes. I decided that I would have two method to mark JWT refresh token invalid - one for the user to logout that takes the refresh token value and one that takes the userId, which will invalidate all the refresh token linked to the user - see the updated DisableJwtRefreshToken class.

This also solves of this being a breaking change, as anyone using the old MarkJwtRefreshTokenAsUsedAsync will have to alter their code. I have added directions to how to change the in the Version 3.5.0 section in the AuthP roadmap.

Thanks to pointing out that it was only the refresh token logout needed changing to support a user logging in multiple times.