validateScope and veryifyScope are anti-patterns?!

[Uzlopak]

The model says, we should provide the functions validateScope and verifyScope. The implementor has to write a correct function, which filters out invalid or not allowed scopes. This can result in a bad implementation, as the implementor could mess it up.

Despite burdening the implementor with the task to write a correct function, the framework should ask for a getScopesOfUser (name disputable), which returns all scopes of the user and the framework will have (well-tested?!) validateScopes and verifyScopes methods which filters out invalid or not allowed scopes.

[jankapunkt]

So do I understand it correctly, that you want to have an additional function that supports the user to validate their integrity before runtime?

[Uzlopak]

No. Currently the implementor has to implement e.g. validateScope Method

https://oauth2-server.readthedocs.io/en/latest/model/spec.html#model-validatescope

// list of valid scopes
const VALID_SCOPES = ['read', 'write'];

function validateScope(user, client, scope) {
  if (!scope.split(' ').every(s => VALID_SCOPES.indexOf(s) >= 0)) {
    return false;
  }
  return scope;
}

But I say: Why should you implement this function to validateScope?
It should be actually:

// list of valid scopes
const VALID_SCOPES = ['read', 'write'];

function getValidScopesOfUser(user, client, scope) {
   return VALID_SCOPES;
}

The actual validating should be done in the Framework, reducing the risk, that the implementor fucks somehow up.

[HappyZombies]

So we want to implement how we get scope? I don't think we should, because scope can be passed a number of different ways. By us implementing it, we kinda force the user to pass scope to us a certain way.

If we are to assume a query parameter, scope can be...

'scope=test,test2,test3'

Or

'scope=[test,test2,test]'

Depending on the API design that a person chooses, we can expect either of these things. Not to mention that frameworks like express with automatically concerns these to an array.

Unless the RFC says to pass it a certain way? Then we can provide it but implementing the method can override our default behavior

[Uzlopak]

Hmm. Actually frameworks use url package of node to parse the query. But I agree that it could be an issue.

But I also think this argument does not count, because validateScope does not get the request object. It gets user client and scope (derived by oauth2-server from the request) as parameters. So even now the implementor does not have the ability to parse the query string from the original request.

[Uzlopak]

I think we can get rid of validateScope completely.

We would need to add scopes to the client. This would be conform with RFC 6749 as it states
https://datatracker.ietf.org/doc/html/rfc6749#section-1.3.4

The client credentials (or other forms of client authentication) can
   be used as an authorization grant when the authorization scope is
   limited to the protected resources under the control of the client,
   or to protected resources previously arranged with the authorization
   server.

"under the control of the client" => client specific scopes (?)

And then we have an internal method for filterScopes ( name pending) which takes the userScopes, clientScopes and requestedScopes as parameters.

At the moment we call validateScope we have already loaded the client and the user. So why do we need to pass it to validateScope of the model to run standardized business logic? Model also indicates, that it should be just an abstraction layer for databases and should imho not contain business logic.

1. First generate an intersection set of all allowed scopes of the user and all the allowed scopes of the client.
2. Check if all the requested scopes are in the intersection or not.

Example Valid:

Step 1:
userScopes: "read write train eat sleep repeat"
clientScopes: "read train sleep repeat"

=> intersection: "read train sleep repeat"

Step 2:
intersection: "read train sleep repeat"
requestedScope: "read"

result: "read"

Example Invalid:

Step 1:
userScopes: "read write train eat sleep repeat"
clientScopes: "read train sleep repeat"

=> intersection: "read train sleep repeat"

Step 2:
intersection: "read train sleep repeat"
requestedScope: "write"

result: ""

You can now either return an access token with an empty scope or just throw an error as we can do nothing we the access token. I think we should throw an InvalidScopeError, as the requested scope is obviously not allowed. And in implementations, where the scope is not utilized we would get empty strings.

---

In the client credential flow you would just pass the clientScopes as the userScopes, to avoid to have a check which flow we currently use.

I think verifyScope could do the same.

[Uzlopak]

Also the current implementation enforces that if the client credential flow is used, that there is a technical user assigned to the Client. Thats why we have the getUserFromClient-model-function. This is imho wrong, as the client has an id and secret, which equivalent to the Resource Owner Password Flow. The client id is so to say the userid/username.

So there should be no getUserFromClient method at all, as it makes no sense.

[Uzlopak]

I would expect something like this. No need for the intersection.

export function filterScopes(user: Pick<IUser, "scopes">, client: Pick<IClient, "scopes">, requestedScopes: string[]) {
    const result: string[] = [];
    const userScopesSet = new Set(user.scopes);
    const clientScopesSet = new Set(client.scopes);

    for (let i = 0, il = requestedScopes.length; i < il; i++) {
        if (
            userScopesSet.has(requestedScopes[i]) && 
            clientScopesSet.has(requestedScopes[i])
        ) {
            result.push(requestedScopes[i]);
        } else {
            throw new InvalidScopeError()
        }
    }
    return result;
}

[Uzlopak]

Or better: Just validate and if an invalid scope is there, throw

export function validateScopes(user: Pick<IUser, "scopes">, client: Pick<IClient, "scopes">, requestedScopes: string[]) {
	const userScopesSet = new Set(user.scopes);
	const clientScopesSet = new Set(client.scopes);

	for (let i = 0, il = requestedScopes.length; i < il; i++) {
		if (
			!userScopesSet.has(requestedScopes[i]) ||
			!clientScopesSet.has(requestedScopes[i])
		)
		{
			throw new InvalidScopeError(
				"The requested scope is invalid, unknown, malformed, or exceeds the scope granted by the resource owner."
			);
		}
	}
}

[HappyZombies]

To avoid backwards breaking, the method can contain a deprecation warning of some sort but we still run it if it's implemented by some people (aka me lol)

[maricn]

Also the current implementation enforces that if the client credential flow is used, that there is a technical user assigned to the Client. Thats why we have the getUserFromClient-model-function. This is imho wrong, as the client has an id and secret, which equivalent to the Resource Owner Password Flow. The client id is so to say the userid/username.

So there should be no getUserFromClient method at all, as it makes no sense.

sorry for offtopic, but is there any issue or PR to make User not required relationship with Client for client_credentials grant? i also noticed another complaint in the old lib - oauthjs/node-oauth2-server#729

[Uzlopak]

In the Client Credential Flow you expect that the Client itself has right to access the resources. So you would actually have a clientId, which would be the actual "username".

[jorenvandeweyer]

@jankapunkt I think we should not remove the validateScope function at this moment.

1. Everyone has already implemented this function
2. It gives the implementer a lot of freedom, we should only implement the spec & provide the tools.

I'll close this issue for now, we can reopen it later if needed.