Add required scopes to token and renewAuth requests

[poovamraj]

Changes

We have added required scope to renewAuth and token requests in AuthenticationAPIClient. Up until now these methods wouldn't have worked without adding scope "openid" to the request. This didn't have effect on internal usage as we always added the default scopes. But usage outside through the API client will fail always until the scope is added.

References

#648

Testing

Please describe how this can be tested by reviewers. Be specific about anything not tested and reasons why. Since this library has unit testing, tests should be added for new functionality and existing tests should complete without errors.

• This change adds unit test coverage

• This change adds integration test coverage

• This change has been tested on the latest version of the platform/language or why not

[Widcket]

If the developer is already adding the openid scope, will this result in it being duplicated?

[poovamraj]

If you see the setScope method, it will use includeRequiredScope which avoids repetition as you mentioned. There are existing internal mechanisms to avoid this.

[d4rken]

@poovamraj

This change caused unexpected behavior in our app.

Previously we specified openid profile email offline_access during login, When the accessToken expired and we refreshed, we do so with scope specified as null. According to docs this means that the previous scope is kept (scope specified during our login).

When using getCredentials and renewing the accessToken:

Before this change newBuilder is used for renew, it uses an empty parameter map, no scope is specified in the request, so the request doesn't contain a scope parameter and the server returns the previous scope.

With this change the newBuilderWithRequiredScope function is used and a default scope of openid is used, scope is no longer null and the server returns a new token with the scope openid and it's no longer the old scope.

Is this intended or a bug?

[poovamraj]

@d4rken The code that checks if the scope has changed uses the value provided in the getCredentials so it will still work as mentioned in the documentation. You can see the code here and here.

[d4rken]

I compared 2.7.0 against 2.9.3 in the debugger.

            if (scope != null) {
                request.addParameter("scope", scope)
            }

This re-adds the scope if one was specified in getCredentials(scope = ... but if none is specified? 🤔 Previously it used storedScope, but now it uses the scope from the new default request constructor?

[poovamraj]

@d4rken I agree, we have to revert this commit. We will cut a release soon (today or tomorrow)

[poovamraj]

@d4rken you can see that it is reverted here - #670. This will ensure the scopes are empty if they are not set unlike adding openid to it.

[d4rken]

Thanks for the quick response.