-
Notifications
You must be signed in to change notification settings - Fork 270
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
feat: Support additional request parameters for 3LO #1070
base: main
Are you sure you want to change the base?
Changes from 3 commits
92e1041
5c24548
f1e815b
6be02e8
640a745
73a046c
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 |
---|---|---|
|
@@ -112,6 +112,9 @@ public class AuthorizationCodeFlow { | |
/** Refresh listeners provided by the client. */ | ||
private final Collection<CredentialRefreshListener> refreshListeners; | ||
|
||
/** Additional parameters to pass to authorization url. */ | ||
private final Map<String, String> additionalParameters; | ||
|
||
/** | ||
* @param method method of presenting the access token to the resource server (for example {@link | ||
* BearerToken#authorizationHeaderAccessMethod}) | ||
|
@@ -131,7 +134,8 @@ public AuthorizationCodeFlow( | |
GenericUrl tokenServerUrl, | ||
HttpExecuteInterceptor clientAuthentication, | ||
String clientId, | ||
String authorizationServerEncodedUrl) { | ||
String authorizationServerEncodedUrl) | ||
{ | ||
this( | ||
new Builder( | ||
method, | ||
|
@@ -140,7 +144,31 @@ public AuthorizationCodeFlow( | |
tokenServerUrl, | ||
clientAuthentication, | ||
clientId, | ||
authorizationServerEncodedUrl)); | ||
authorizationServerEncodedUrl) | ||
); | ||
} | ||
|
||
public AuthorizationCodeFlow( | ||
AccessMethod method, | ||
HttpTransport transport, | ||
JsonFactory jsonFactory, | ||
GenericUrl tokenServerUrl, | ||
HttpExecuteInterceptor clientAuthentication, | ||
String clientId, | ||
String authorizationServerEncodedUrl, | ||
Map<String, String> additionalParameters) | ||
{ | ||
this( | ||
new Builder( | ||
method, | ||
transport, | ||
jsonFactory, | ||
tokenServerUrl, | ||
clientAuthentication, | ||
clientId, | ||
authorizationServerEncodedUrl, | ||
additionalParameters) | ||
); | ||
} | ||
|
||
/** | ||
|
@@ -156,6 +184,7 @@ protected AuthorizationCodeFlow(Builder builder) { | |
clientId = Preconditions.checkNotNull(builder.clientId); | ||
authorizationServerEncodedUrl = | ||
Preconditions.checkNotNull(builder.authorizationServerEncodedUrl); | ||
additionalParameters = Preconditions.checkNotNull(builder.additionalParameters); | ||
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. no need of null check here because it is an optional thing. You are checking for null before using it. 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. Ah! Removed precondition check. |
||
requestInitializer = builder.requestInitializer; | ||
credentialStore = builder.credentialStore; | ||
credentialDataStore = builder.credentialDataStore; | ||
|
@@ -192,6 +221,11 @@ public AuthorizationCodeRequestUrl newAuthorizationUrl() { | |
url.setCodeChallenge(pkce.getChallenge()); | ||
url.setCodeChallengeMethod(pkce.getChallengeMethod()); | ||
} | ||
if (additionalParameters != null) { | ||
for (Map.Entry<String, String> entry : additionalParameters.entrySet()) { | ||
url.put(entry.getKey(), entry.getValue()); | ||
} | ||
} | ||
return url; | ||
} | ||
|
||
|
@@ -549,6 +583,9 @@ public static class Builder { | |
/** Refresh listeners provided by the client. */ | ||
Collection<CredentialRefreshListener> refreshListeners = Lists.newArrayList(); | ||
|
||
/** Additional authorization url parameters provided by the client **/ | ||
Map<String, String> additionalParameters; | ||
|
||
/** | ||
* @param method method of presenting the access token to the resource server (for example | ||
* {@link BearerToken#authorizationHeaderAccessMethod}) | ||
|
@@ -560,21 +597,42 @@ public static class Builder { | |
* @param clientId client identifier | ||
* @param authorizationServerEncodedUrl authorization server encoded URL | ||
*/ | ||
public Builder( | ||
|
||
public Builder( | ||
AccessMethod method, | ||
HttpTransport transport, | ||
JsonFactory jsonFactory, | ||
GenericUrl tokenServerUrl, | ||
HttpExecuteInterceptor clientAuthentication, | ||
String clientId, | ||
String authorizationServerEncodedUrl) { | ||
setMethod(method); | ||
setTransport(transport); | ||
setJsonFactory(jsonFactory); | ||
setTokenServerUrl(tokenServerUrl); | ||
setClientAuthentication(clientAuthentication); | ||
setClientId(clientId); | ||
setAuthorizationServerEncodedUrl(authorizationServerEncodedUrl); | ||
setAdditionalParameters(Collections.<String, String>emptyMap()); | ||
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 is ok to just leave additional params as null. No need to set as empty map. It is ok to have it as well, no harm. |
||
} | ||
|
||
public Builder( | ||
sai-sunder-s marked this conversation as resolved.
Show resolved
Hide resolved
|
||
AccessMethod method, | ||
HttpTransport transport, | ||
JsonFactory jsonFactory, | ||
GenericUrl tokenServerUrl, | ||
HttpExecuteInterceptor clientAuthentication, | ||
String clientId, | ||
String authorizationServerEncodedUrl) { | ||
String authorizationServerEncodedUrl, | ||
Map<String, String> additionalParameters) { | ||
setMethod(method); | ||
setTransport(transport); | ||
setJsonFactory(jsonFactory); | ||
setTokenServerUrl(tokenServerUrl); | ||
setClientAuthentication(clientAuthentication); | ||
setClientId(clientId); | ||
setAuthorizationServerEncodedUrl(authorizationServerEncodedUrl); | ||
setAdditionalParameters(additionalParameters); | ||
} | ||
|
||
/** Returns a new instance of an authorization code flow based on this builder. */ | ||
|
@@ -717,6 +775,20 @@ public Builder setAuthorizationServerEncodedUrl(String authorizationServerEncode | |
return this; | ||
} | ||
|
||
/** | ||
* Sets the additional url parameters. | ||
* | ||
* <p>Overriding is only supported for the purpose of calling the super implementation and | ||
* changing the return type, but nothing else. | ||
* | ||
* @since 1.11 | ||
*/ | ||
public Builder setAdditionalParameters(Map<String, String> additionalParameters) { | ||
this.additionalParameters = | ||
Preconditions.checkNotNull(additionalParameters); | ||
return this; | ||
} | ||
|
||
/** | ||
* {@link Beta} <br> | ||
* Returns the credential persistence store or {@code null} for none. | ||
|
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.
lets remove this overloaded method. If someone really wants to pass additional parameters, they can create builder, set additionalparam and call build() on the builder. Exactly like what we have done in the test.
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 I see. I have removed overloaded method.
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.
Lets remove this overloaded method