Bug with RequiredScopes

[danielwertheim]

At https://github.com/IdentityServer/IdentityServer3.AccessTokenValidation/blob/20149710088dd732b0e0fd1dc60a3a062b73f9a4/source/AccessTokenValidation/Plumbing/ScopeRequirementMiddleware.cs#L122

It checks for occurence of any of the required scopes. Shouldn't it look for all required scopes? Something like:

private bool ScopesFound(OwinContext context)
{
    var scopeClaims = context.Authentication.User.FindAll("scope");

    if (scopeClaims == null)
        return false;

    var userScopes = scopeClaims.Select(scope => scope.Value).ToArray();

    return userScopes.Any() && _options.RequiredScopes.All(s => userScopes.Contains(s, StringComparer.Ordinal));
}

If not, then please allow an option for saying that all should be required.

Otherwise it should be something like RequiresAnyOfTheFollowingScopes

[leastprivilege]

We look for ANY scope. If you need ALL semantics - simply write a little middleware that runs after ours to check the scopes.

[danielwertheim]

RequiredScopes kind of hints that all are required. Any chance you could add an option that actually will enforce it?

[leastprivilege]

Unfortunate naming I agree - but changing the names will be a breaking change.

Feel free to propose a PR.

[danielwertheim]

Actually, the whole flow became nicer by writing two custom middlewares running after your. One pre-guard and one for transformations.

[parkinsona]

I just found this behaviour as well. Fortunately our API checks for claims that are returned by both scopes in the claims transformation so we don't need to implement the middleware.

It would be great to have an option that we could pick that says Required Scopes will be treated as an ANY or ALL.