-
Notifications
You must be signed in to change notification settings - Fork 143
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Enforce openid scope on the AuthenticationAPIClient #455
Conversation
val request = BaseAuthenticationRequest(factory.post(url.toString(), credentialsAdapter)) | ||
request.addParameters(parameters) | ||
return request | ||
return loginWithToken(parameters) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
most logic was already available on this method, so I simplified this call
@@ -64,7 +66,7 @@ public class ParameterBuilder private constructor(parameters: Map<String, String | |||
* @return itself | |||
*/ | |||
public fun setScope(scope: String): ParameterBuilder { | |||
return set(SCOPE_KEY, scope) | |||
return set(SCOPE_KEY, OidcUtils.includeRequiredScope(scope)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
when a scope is set through this setter, it will be checked so that it contains "openid"
* | ||
* @return a new builder | ||
*/ | ||
@JvmStatic | ||
public fun newAuthenticationBuilder(): ParameterBuilder { | ||
return newBuilder() | ||
.setScope(SCOPE_OPENID) | ||
.setScope(OidcUtils.DEFAULT_SCOPE) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this method will always add as default the scope of "openid profile email". It can be changed later by the dev.
@@ -55,7 +56,7 @@ internal class OAuthManager( | |||
} | |||
|
|||
fun startAuthentication(context: Context, redirectUri: String, requestCode: Int) { | |||
addRequiredScope(parameters) | |||
OidcUtils.includeDefaultScope(parameters) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
moved the method to a different class, but it's the same logic (test remains unchanged)
} else { | ||
value | ||
} | ||
return addParameter(name, anyValue) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
delegates to the Any
method signature
@@ -1505,6 +1509,30 @@ public class AuthenticationAPIClientTest { | |||
assertThat(body, Matchers.hasEntry("client_id", CLIENT_ID)) | |||
assertThat(body, Matchers.hasEntry("refresh_token", "refreshToken")) | |||
assertThat(body, Matchers.hasEntry("grant_type", "refresh_token")) | |||
assertThat(body, Matchers.not(Matchers.hasKey("scope"))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this ensures the renew auth method is not setting a scope value by default
} | ||
|
||
@Test | ||
public fun shouldRenewAuthWithOAuthTokenAndCustomScope() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this checks that it can still be set a scope by the dev later, if desired
Changes
The requests affected by this change include all
/oauth/token
calls but the one that is used to renew the credentials with a Refresh Token.The logic is as follows:
scope
of "openid profile email" to match theWebAuthProvider
default scope.scope
value is set by the developer, it will be checked that it includes "openid" and it will append "openid" if it's missing.This change is to ensure the tokens response from the server is compliant with the OpenID Connect spec.
This is NOT a breaking change