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

Authorization #24

Merged
merged 7 commits into from
Dec 13, 2021
Merged

Authorization #24

merged 7 commits into from
Dec 13, 2021

Conversation

fretje
Copy link
Contributor

@fretje fretje commented Dec 12, 2021

Draft for now... any feedback welcome.

This is how I would do authorization... At least for the AzureAd side of things.

The bulk of the functionality is in AzureAdClaimsPrincipalFactory.CreateUserAsync which gets called by the framework when a claimsprincipal is constructed after a user signs in. There you have an opportunity to add claims to the claimsprincipal in question.
2 calls are done: First IdentityService.GetProfileDetailsAsync to get the current user's Id, which is used to call UserService.GetPermissionsAsync which returns the permissions for that user. Those permissions are added to the claims, so the rest of the authentication logic can take over.

I will also add the rest of the user info into claims, so it can be used in other places in the app (e.g. personcard).

We'll have to add an [Authorize(Policy = <permission>)] attribute to the individual pages with their respective permissions. I've done this now for the brands page to test, also for the dashboard I created a new permission to be able to see what happens when a logged in user doesn't have access to a particular page. I had to update app.razor to accommodate for that, otherwise we got into an login loop...

A couple of remarks:

  • We also will have to include logic into the NavigationMenu so menu items for which a user doens't have access won't be shown.
  • It might be interesting to create a new api endpoint which returns the info from both calls GetProfileDetails and GetPermissions, so only one call needs to be done in stead of 2.

I'll also try to figure out how to do a similar thing for regular Jwt auth...

@iammukeshm
Copy link
Member

For navigation permission checking, we can use

public static Task<AuthorizationResult> AuthorizeAsync(this IAuthorizationService service, ClaimsPrincipal user, string policyName);. This will return a boolean based on the permission.

I have used this earlier this way bool _canViewBrands = (await _authorizationService.AuthorizeAsync(_authenticationStateProviderUser, Permissions.Brands.View)).Succeeded;

@fretje
Copy link
Contributor Author

fretje commented Dec 13, 2021

Jep, that's what I meant.

@@ -5,7 +5,7 @@ public static class ClaimsPrincipalExtensions
=> claimsPrincipal.FindFirstValue(ClaimTypes.Email);

public static string? GetTenant(this ClaimsPrincipal claimsPrincipal)
=> claimsPrincipal.FindFirstValue(ClaimTypes.Name);
=> claimsPrincipal.FindFirstValue(ClaimTypes.Name); // Todo: ?? think this is not right
Copy link
Member

Choose a reason for hiding this comment

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

Yes, has to be claimsPrincipal.FindFirstValue("Tenant") or something.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

should be ok now

@fretje
Copy link
Contributor Author

fretje commented Dec 13, 2021

I am working on getting refresh tokens working as well...

… jwt auth

also some renames and namespace changes
@fretje
Copy link
Contributor Author

fretje commented Dec 13, 2021

Permissions and refresh tokens working...

I've got a strange thing wrt multiple calls at the same time when I set the token expiration time to 2 minutes on the server, Maybe that's to be expected as I'm asking for a refresh token when the expiration time is less than 2 minutes away... but still, it's something strange... happens when I go to the brands page as there, for some reason (another bug, but a good one to test this out) the call to search for brands is done twice on initialization. One of them gets a 401 on the refresh token call as the refresh token is probably not valid anymore, as the refresh calls happen too fast after each other. There's an experiment there with a semaphoreslim, but that's probably not needed (not a solution to the problem anyways).

@fretje fretje marked this pull request as ready for review December 13, 2021 16:13
@fretje
Copy link
Contributor Author

fretje commented Dec 13, 2021

When I put the token expiration time to 3 minutes (and I now also wait until expiration time is only 1 minute away) I can't seem to repeat that problem anymore, so I wouldn't worry too much about it.

The signalr connection actually keeps the access token refreshed, as I see the connection quickly gets disconnected and re-connected every 3 minutes... that means a valid access token should be available for calling the api at all times.

I've added some "snackbars" when signalr is reconnecting... these are temporary i'd say.
We could include a "connection" icon up top in the ui somewhere which denotes the status of the signalr connection (I.e. green when connected, orange when reconnecting and red when disconnected) with the error (if any) on a tooltip?

Still have to implement/test the scenario where the refreshtoken actually gets expired though...

Anyways, the more I read about it, the more it seems that this jwt authentication method is not done in this day and age (It's never good to "roll your own" when it comes to security). You shouldn't save access and refresh tokens on the client, ...

I would put it in the docs that this standard authentication method is made to get started quickly, but should be replaced by something more robust when you actually put your app online!

@fretje
Copy link
Contributor Author

fretje commented Dec 13, 2021

Btw, this should fix #12 as tokens now get refreshed automatically behind the scenes.

Should also fix #22 as now the tenant key is taken from the "tenant" claim.

@iammukeshm
Copy link
Member

Is this PR ready to be merged?

@fretje
Copy link
Contributor Author

fretje commented Dec 13, 2021

Yes if review is ok ;-)

throw new NotImplementedException();

public Task<string> TryRefreshToken() =>
public Task<IResult<TokenResponse>> RefreshTokenAsync(RefreshTokenRequest request) =>
Copy link
Member

Choose a reason for hiding this comment

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

refreshing is not needed here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nope, this should be handled by AzureAdAuthorizationMessageHandler.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The only reason this interface is implemented here is for the "LogOutAsync" method which gets called from some other places (and which is different for azuread then for jwt). Maybe we could split that interface up into 2 interfaces, so azuread only needs to implement one of them?

@iammukeshm iammukeshm merged commit e757ccd into fullstackhero:main Dec 13, 2021
@fretje fretje deleted the Authorization branch December 14, 2021 15:08
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.

None yet

2 participants