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
764 stars 155 forks source link

Logical problem with IDisableJwtRefreshToken #54

Closed idan-h closed 1 year ago

idan-h commented 1 year ago

Hey, I have a logical problem with IDisableJwtRefreshToken.MarkJwtRefreshTokenAsUsedAsync.
Lets say a user logs in, and the access token is valid for 30 minutes. Then, after 10 minutes the user logs in from another machine and creates another refresh token.
Now, he logs out of the first login, and instead of disabling the first login refresh token, the second login refresh token gets disabled. The result is that he "logged out" on both machines unintendedly. It is also a minor security vulnerability, because someone can utilize the first refresh token (it is deleted from the client side but still valid and able to produce new refresh tokens).

JonPSmith commented 1 year ago

Hi @idan-h,

Yes I can see that is a problem. I think if I use the JWT Token as well it might work, but need to do some research first.

JonPSmith commented 1 year ago

Hi @Idan-h,

I got some time to look at this and looked at Rui Figueiredo article Refresh Tokens in ASP.NET Core Web Api and he says.

The simplest version of this is just to have an extra column for the refresh token in the user’s table. This has the consequence of only allowing the user to be logged-in in one location (there’s only 1 refresh token valid per user at a time). Alternatively, you can maintain several refresh tokens per user and save the geographical location, time, etc from the request that originated them so that you can provide the user with activity reports.

From Rui Figueiredo comments its clear that I can't create a generic answer to the JWT refresh token. In this case you will need to create their own JWT refresh token.

PS. I have updated the JWT Token refresh explained documentation to link back this issue

idan-h commented 1 year ago

We can add 'CreatedByRefreshToken' property to the refresh token entity which contains the refresh token which created the current one, and when logging out revoke all the related tokens recursively.
This way we support multiple loggins at the same time which might be a desired thing.

Also, should probably add an option to specify the refresh token to be revoked.

And can add mechanism which if a revoked refresh token is accessed, we revoke all the refresh tokens it created.

JonPSmith commented 1 year ago

Hi @idan-h,

That is a great solution to the multi-user problem. I have reopened this issue so that when I update the library I will implement that change. I may create a preview version of the NuGet for you to check.

Also, should probably add an option to specify the refresh token to be revoked.

There is already a IDisableJwtRefreshToken service, but it wasn't in the documentation - it is now.

idan-h commented 1 year ago

Hi @idan-h,

That is a great solution to the multi-user problem. I have reopened this issue so that when I update the library I will implement that change. I may create a preview version of the NuGet for you to check.

Also, should probably add an option to specify the refresh token to be revoked.

There is already a IDisableJwtRefreshToken service, but it wasn't in the documentation - it is now.

Great, let me know if you need help I meant, adding a function with parameter to the IDisableJwtRefreshToken service, which specifies the refresh token to revoke.

Something like MarkJwtRefreshTokenAsUsedByRefreshTokenAsync(string refreshToken)

JonPSmith commented 1 year ago

Hi @idan-h,

I'm working on the next release of the AuthP and I looked this JWT refresh token with two logins. My search hasn't found anyone who is asking for this feature and my solution would need a database migration.

You posed this as a logical problem, but you didn't describe a situation where this is feature is really used. For this reason I'm going to release a new version without this issue because it has many useful features / fixes.

idan-h commented 1 year ago

Hi @idan-h,

I'm working on the next release of the AuthP and I looked this JWT refresh token with two logins. My search hasn't found anyone who is asking for this feature and my solution would need a database migration.

You posed this as a logical problem, but you didn't describe a situation where this is feature is really used. For this reason I'm going to release a new version without this issue because it has many useful features / fixes.

Hey, #60 solves it without the recursive part (might be unnecessary), just by adding option to specify the refresh token.