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
KTOR-5225 Add callbacks to save application state for OAuth2 #3282
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -84,7 +84,8 @@ public sealed class OAuthServerSettings(public val name: String, public val vers | |
public val passParamsInURL: Boolean = false, | ||
public val extraAuthParameters: List<Pair<String, String>> = emptyList(), | ||
public val extraTokenParameters: List<Pair<String, String>> = emptyList(), | ||
public val accessTokenInterceptor: HttpRequestBuilder.() -> Unit = {} | ||
public val accessTokenInterceptor: HttpRequestBuilder.() -> Unit = {}, | ||
public val onStateCreated: suspend (call: ApplicationCall, state: String) -> Unit = { _, _ -> } | ||
) : OAuthServerSettings(name, OAuthVersion.V20) { | ||
|
||
@Deprecated("This constructor will be removed", level = DeprecationLevel.HIDDEN) | ||
|
@@ -117,6 +118,40 @@ public sealed class OAuthServerSettings(public val name: String, public val vers | |
emptyList(), | ||
accessTokenInterceptor | ||
) | ||
|
||
@Deprecated("This constructor will be removed", level = DeprecationLevel.HIDDEN) | ||
public constructor( | ||
name: String, | ||
authorizeUrl: String, | ||
accessTokenUrl: String, | ||
requestMethod: HttpMethod = HttpMethod.Get, | ||
clientId: String, | ||
clientSecret: String, | ||
defaultScopes: List<String> = emptyList(), | ||
accessTokenRequiresBasicAuth: Boolean = false, | ||
nonceManager: NonceManager = GenerateOnlyNonceManager, | ||
authorizeUrlInterceptor: URLBuilder.() -> Unit = {}, | ||
passParamsInURL: Boolean = false, | ||
extraAuthParameters: List<Pair<String, String>> = emptyList(), | ||
extraTokenParameters: List<Pair<String, String>> = emptyList(), | ||
accessTokenInterceptor: HttpRequestBuilder.() -> Unit = {}, | ||
) : this( | ||
name, | ||
authorizeUrl, | ||
accessTokenUrl, | ||
requestMethod, | ||
clientId, | ||
clientSecret, | ||
defaultScopes, | ||
accessTokenRequiresBasicAuth, | ||
nonceManager, | ||
authorizeUrlInterceptor, | ||
passParamsInURL, | ||
extraAuthParameters, | ||
extraTokenParameters, | ||
accessTokenInterceptor, | ||
{ _, _ -> } | ||
) | ||
} | ||
} | ||
|
||
|
@@ -161,15 +196,31 @@ public sealed class OAuthAccessTokenResponse : Principal { | |
* @property tokenType OAuth2 token type (usually Bearer) | ||
* @property expiresIn token expiration timestamp | ||
* @property refreshToken to be used to refresh access token after expiration | ||
* @property state generated state used for the OAuth procedure | ||
* @property extraParameters contains additional parameters provided by the server | ||
*/ | ||
public data class OAuth2( | ||
val accessToken: String, | ||
val tokenType: String, | ||
val expiresIn: Long, | ||
val refreshToken: String?, | ||
val extraParameters: Parameters = Parameters.Empty | ||
) : OAuthAccessTokenResponse() | ||
val extraParameters: Parameters = Parameters.Empty, | ||
) : OAuthAccessTokenResponse() { | ||
|
||
public var state: String? = null | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. just a question: is it expected, that while There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's really difficult to add it without breaking binary compatibility. In practice, I don't expect users to do |
||
private set | ||
|
||
public constructor( | ||
accessToken: String, | ||
tokenType: String, | ||
expiresIn: Long, | ||
refreshToken: String?, | ||
extraParameters: Parameters = Parameters.Empty, | ||
state: String? = null | ||
) : this(accessToken, tokenType, expiresIn, refreshToken, extraParameters) { | ||
this.state = state | ||
} | ||
} | ||
} | ||
|
||
/** | ||
|
@@ -214,6 +265,7 @@ public suspend fun PipelineContext<Unit, ApplicationCall>.oauthRespondRedirect( | |
call.redirectAuthenticateOAuth1a(provider, requestToken) | ||
} | ||
} | ||
|
||
is OAuthServerSettings.OAuth2ServerSettings -> { | ||
call.redirectAuthenticateOAuth2( | ||
provider, | ||
|
@@ -275,6 +327,7 @@ public suspend fun PipelineContext<Unit, ApplicationCall>.oauthHandleCallback( | |
} | ||
} | ||
} | ||
|
||
is OAuthServerSettings.OAuth2ServerSettings -> { | ||
val code = call.oauth2HandleCallback() | ||
if (code == null) { | ||
|
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.
minor: "This constructor will be removed" -> "Binary compatibility with 2.x"?
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.
IMO, deprecation messages should be for users explaining why this is deprecated, not for us explaining why it is kept.
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.
AFAIK, declarations with
level=HIDDEN
will be not visible to user, until they open source code. They even should not feel this change at all, as new parameter has default value, but ok.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.
Yes, but for consistency, I prefer it this way.