RequiresAnyRole making multiple (redundant?) DB queries

ServiceStack Authentication
1

Hei.

I have noticed a strange/unexpected behaviour in attribute-based role-permissions checks.

Specifically, it seems that every HTTP request to an authentication-required endpoint polls the UserAuthRole table repeatedly, rather than simply checking the session if/when available. I dug into the logic and believe that the method below is the cause, and I wonder, is this behaviour intention, or accidental and suboptimal? For reference, I am using sessions and not jwts

(copied from AuthUserSession.cs)

	// Dale Comment: Current framework method definition
	public override async Task<bool> HasRoleAsync(string role, IAuthRepositoryAsync authRepo, CancellationToken token=default)
	{
	    // // Dale Comment: I am using sessions, so FromToken is false
		if (!FromToken) //If populated from a token it should have the complete list of roles
		{
		    // Dale Comment: this ALWAYS hits the database for every potential role in the RequiresRoleAttribute until detecting a match, resulting in several db calls per request
			if (authRepo is IManageRolesAsync managesRoles)
			{
				if (UserAuthId == null)
				{
					return false;
				}
				return await managesRoles.HasRoleAsync(this.UserAuthId, role, token);
			}
		}
		// Dale Comment: whereas the session already contains a Roles collection and I think we could check the session first and THEN hit the db if necessary??
		// Dale Comment: Should/could this final check be moved ABOVE the database calls?
		return this.Roles != null && this.Roles.Contains(role);
	}

I noticed this accidentally while I was setting up logging and accidentally set to DEBUG level… at which point I noticed the following pattern [cache query, loop through roles until matches final role “MyRole2”]

[10:25:40 CorrelationId: 2cf8d60c-210f-4133-99c0-a89d24c5e670 DBG] SQL: SELECT "Id", "Data", "ExpiryDate", "CreatedDate", "ModifiedDate" FROM "CacheEntry" WHERE "Id" = @Id
PARAMS: Id=urn:iauthsession:QEX4IosqYyee2cv1IIwK
[10:25:40 CorrelationId: 2cf8d60c-210f-4133-99c0-a89d24c5e670 DBG] SQL: SELECT COUNT(*)
FROM "UserAuthRole"
WHERE (("UserAuthId" = @0) AND ("Role" = @1))
PARAMS: @0=14, @1=Admin
[10:25:40 CorrelationId: 2cf8d60c-210f-4133-99c0-a89d24c5e670 DBG] SQL: SELECT COUNT(*)
FROM "UserAuthRole"
WHERE (("UserAuthId" = @0) AND ("Role" = @1))
PARAMS: @0=14, @1=MyRole1
[10:25:40 CorrelationId: 2cf8d60c-210f-4133-99c0-a89d24c5e670 DBG] SQL: SELECT COUNT(*)
FROM "UserAuthRole"
WHERE (("UserAuthId" = @0) AND ("Role" = @1))
PARAMS: @0=14, @1=MyRole2

where the user logged in has the MyRole2 role (ie the final one being checked) and the request message class is declared as follows:

[RequiresAnyRole(nameof(MergeroRoleEnum.MyRole1), nameof(MergeroRoleEnum.MyRole2))]
[Route(
	"/offerings/grid-content",
	HttpMethods.Get,
	Summary = "",
	Notes = ""
)]
public class GetOfferingsGridContentQuery : IReturn<GetOfferingsGridContentResponse> { }
2

Continuing this, I think there would need to be a change in the RequiresAnyRoleAttribute class as well, specifically in the region of:

public virtual async Task<bool> HasAnyRolesAsync(IAuthSession session, IAuthRepositoryAsync authRepo)
    {
        // Dale Comment: Because this loops through required roles one by one, even moving the Session.Roles check above the AuthRepo.DbCheck() logic in the AuthSession wouldn't entirely eliminate redundant db queries. Would need to pass in the full role collection from RequiresAnyRole attribute in a single list, so all could be checked at the same time.

        return session != null && await this.RequiredRoles
            .AnyAsync(requiredRole => session.HasRoleAsync(requiredRole, authRepo)).ConfigAwait();
    }

See: https://github.com/ServiceStack/ServiceStack/blob/dbf2f4c4cac727df526947a2ea0fa7c96acbdebd/src/ServiceStack/RequiresAnyRoleAttribute.cs#L93

3

If your AuthRepository implements IManageRoles (e.g. OrmLite) then the UserSession by default will query the database otherwise it checks the session, if you don’t want that behavior you can overwrite it in your custom AuthUserSession, e.g:

public override async Task<bool> HasRoleAsync(string role, IAuthRepositoryAsync authRepo, CancellationToken token=default)
{
    return this.Roles != null && this.Roles.Contains(role);
}

But it does mean you’re checking the session that’s a snapshot created at authentication so any DB changes to adding/removing roles wont apply until the next time they sign in.

There’s a potential optimization of adding HasAnyRoles/HasAllRoles APIs so it only needs to perform 1 API call, but it requires implementations for all IManageRoles/Async providers, will investigate.

4

You should be enjoying your weekend away from the computer :smiley:

But thanks for the suggestion, I actually implemented that exact approach for now in my override and for my particular software use-case, roles aren’t dynamically assigned or updates so should be fine, but I thought i’d point it out as it probably affects other users and people might not be immediately aware.

Cheers!

5

Turns out I didn’t need to I didn’t need to modify the AuthProviders as I was able to modify the HasRole() test to use session.GetRoles/Async() (calls authRepo.GetRoles/Async()) instead which only needs to fetch all the users roles once to do its tests, if the role check fails it does requery the users roles to load the latest roles to handle if they were added whilst the user was authenticated.

I’ve also replaced the sessions HasRole/Async() implementation to also query GetRolesAsync() so that you only need to modify the implementation in 1 place (either in custom Session or AuthRepo) to provide a custom role test implementation:

public virtual async Task<bool> HasRoleAsync(string role, IAuthRepositoryAsync authRepo, CancellationToken token=default)
{
    var roles = await GetRolesAsync(authRepo, token).ConfigAwait();
    return roles.Contains(role);
}

This change is available from v5.12.1+ that’s now available on MyGet.

1 Like