Incorrect Scope Format in API Response: Single String Instead of Array for Multiple Scopes about vapor-oauth HOT 8 CLOSED

Once more, sorry that I didn't see it in the first place that there happens a mapping based on spaces in the RequestObject

from vapor-oauth.

Hi @vamsii777,

I checked the scope issue in more detail and found that there is no issue.

AuthorizeGetHandler.swift

private func validateRequest(_ request: Request) async throws -> (Response?, AuthorizationGetRequestObject?) 

(...)

let scopes: [String]
        
        if let scopeQuery: String = request.query[OAuthRequestParameters.scope] {
            scopes = scopeQuery.components(separatedBy: " ")
        } else {
            scopes = []
        }
(...)

As you can see it accepts scopes sent from the client only separated by spaces (" ").

I found different specifications as we use ["scope","scope"]; "scope,scope"; "scope scope" as possibilities. According to
https://www.rfc-editor.org/rfc/rfc6749#section-3.3
it says:

The value of the scope parameter is expressed as a list of space-
delimited, case-sensitive strings. The strings are defined by the
authorization server. If the value contains multiple space-delimited
strings, their order does not matter, and each string adds an
additional access range to the requested scope.

I was not aware that we follow only this guideline and don't allow scopes as "scope,scope"

So, there is nothing that needs to be fixed and sorry that I created this confusion in the first place.

from vapor-oauth.

But there is one more topic that needs to be considered with OpenID Connect.

In the AccessToken we have also "aud" which represents the clientID.

Based on the userinfo endpoint specification
https://openid.net/specs/openid-connect-core-1_0.html#UserInfoResponse
a client would for example request

"email phone address profile customized1 customized2"

The OpenID Provider should then return only the valid user attributes FOR THIS CLIENT.

At the moment we are returning all user attributes because the UserManager doesn't know anything except the userID:

func getUser(userID: String) async throws -> VaporOAuth.OAuthUser? {

If we would have an optional parameter clientID the UserManager could look up the scopes for this client (AccessToken aud) and check what parameters it should return.

What do you think about extending getUser:

getUser(userID: String, clientID: String?)

I believe it would not be good if the returned attributes are automatically managed as there are also customized scopes allowed and the OpenID specification describes that out of security reasons not all requested attributes per scope must be returned. So, this should be handled by the consumer = customized UserManager

from vapor-oauth.

@mynona so, about the scopes there's no need to change apart from that being string in the JWT right? The previous branch feature/jwt should be alright for the changes or fixes/incorrect-scope-format?

from vapor-oauth.

In my opinion, you improved the readability of the code a lot, because you do the mapping to [String] in the ScopeValidator. This is why the issue arose in the first place because it was not obvious where "admin openid" was mapped to ["admin", "openid"]. So, I think it is an improvement.

But my second comment is more important: Extending the UserManager to return user attributes based on clientID (and therefore, return only allowed attributes for this client)

Otherwise you would return data that should not even be shared with certain clients. It's all connected.

from vapor-oauth.

After putting more thought in this: the JWT doesn't need to be changed as the payload is created by the consumer of vapor/oauth. I think, this is perfect because then every user of vapor/oauth can configure the payload themselves (and have to take care that they implement it correctly).

This flexibility should remain on the consumer I guess.

from vapor-oauth.

Yes, agreed! Will check on #11 (comment)

from vapor-oauth.

• Merged #15

from vapor-oauth.