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
788 stars 159 forks source link

Added an option for expiration to user invitation #55

Closed idan-h closed 2 years ago

idan-h commented 2 years ago

Hey, added an option for expiration to InviteNewUserService: IInviteNewUserService
Tests confirmed

idan-h commented 2 years ago

Also, I noticed there is no check if the roles are valid for the user? From what I have seen, a user can invite another user with app admin role? If so, this needs to be addressed.. would happy to hear your thoughts

JonPSmith commented 2 years ago

Hi @idan-h,

I noticed there is no check if the roles are valid for the user

That isn't correct. This section in the InviteNewUserService checks that invites to a tenant user have their roles checked.

idan-h commented 2 years ago

Hi @idan-h,

I noticed there is no check if the roles are valid for the user

That isn't correct. This section in the InviteNewUserService checks that invites to a tenant user have their roles checked.

yea, I see it now.. Idk how I missed that. Removed the faulty commits.

JonPSmith commented 2 years ago

Hi @idan-h,

I looked at your PR and it didn't link to the AddNewUserDto and didn't include any code to set the expiration. I therefore in version 3.4.0 I added the code to make it more part of the invite a user feature. I appreciated your suggestion and I thing the invite user feature is better for your input.

idan-h commented 2 years ago

Hey, what do you mean by "didn't include any code to set the expiration"? I Added an option to add expiration time to the method CreateInviteUserToJoinAsync by adding a new dto AddNewUserWithExpirationDto and added a check when adding the user.

You can see the test I wrote to understand better

idan-h commented 2 years ago

If you want to change the current dto instead of what I did it is fine by me, but I think that the expiration option is a must in this case (atleast for me).. I wouldn't want a user joining with a link I sent after monthes

JonPSmith commented 2 years ago

Hey, what do you mean by "didn't include any code to set the expiration"?

Sorry, you didn't create a frontend way to set the expiration time. It wasn't that hard to do, but I always want the examples use all the features so that developers how it works. See example in Example3 and Example5.

I appreciate you help and suggestions but it is my decision whether I take a PR. Your idea was really good and the ReleaseNotes contains a (inspired by @idan-h) note against the expiration addition. The idea was yours and I just wrote the code in a way that was more part of the invite user code.

idan-h commented 2 years ago

Hey, what do you mean by "didn't include any code to set the expiration"?

Sorry, you didn't create a frontend way to set the expiration time. It wasn't that hard to do, but I always want the examples use all the features so that developers how it works. See example in Example3 and Example5.

I appreciate you help and suggestions but it is my decision whether I take a PR. Your idea was really good and the ReleaseNotes contains a (inspired by @idan-h) note against the expiration addition. The idea was yours and I just wrote the code in a way that was more part of the invite user code.

Oh, I do not care much about the credit and it wasn't my intention. Sorry if it came that way.

I thought you didn't recognize the need for this option so you didn't include it. My bad.

Anyway, I meant for the expiration to be set by the back-side only (as it is a security measure). But sure, some examples won't hurt.