-
Notifications
You must be signed in to change notification settings - Fork 39
fix: Support dynamic redirect_uri in Toolset OAuth signin flow #1418
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
base: development
Are you sure you want to change the base?
Changes from all commits
190221d
db97668
f26ef73
62d9544
11b772a
16cf104
eae109a
71930ac
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 |
|---|---|---|
| @@ -0,0 +1,23 @@ | ||
| package com.epam.aidial.core.credentials.service; | ||
|
|
||
| import com.epam.aidial.core.config.ResourceAuthSettings; | ||
| import lombok.experimental.UtilityClass; | ||
|
|
||
| import java.util.ArrayList; | ||
| import java.util.List; | ||
|
|
||
| @UtilityClass | ||
| public class RedirectUriHelper { | ||
|
|
||
| /** | ||
| * Builds the effective list of allowed redirect URIs by combining the global allowed list | ||
| * with the toolset's own redirect URI (if present and not already included). | ||
| */ | ||
| public List<String> collectAllowedRedirectUris(List<String> globalAllowedUris, ResourceAuthSettings resourceAuthSettings) { | ||
| List<String> uris = new ArrayList<>(globalAllowedUris); | ||
| if (resourceAuthSettings.getRedirectUri() != null && !uris.contains(resourceAuthSettings.getRedirectUri())) { | ||
| uris.add(resourceAuthSettings.getRedirectUri()); | ||
| } | ||
| return uris; | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -7,25 +7,30 @@ | |
| import com.epam.aidial.core.credentials.data.credentials.TokenResponse; | ||
| import lombok.AllArgsConstructor; | ||
| import lombok.extern.slf4j.Slf4j; | ||
| import org.apache.commons.lang3.StringUtils; | ||
|
|
||
| import java.util.List; | ||
|
|
||
| @AllArgsConstructor | ||
| @Slf4j | ||
| public class TokenService { | ||
|
|
||
| private final ResourceAuthorizationClient resourceAuthorizationClient; | ||
| private final List<String> allowedRedirectUris; | ||
|
|
||
| public TokenResponse getToken(String resourceId, | ||
| ResourceAuthSettings resourceAuthSettings, | ||
| ResourceSignInRequest resourceSignInRequest) { | ||
| log.debug("Start Resource {} token retrieval", resourceId); | ||
| String redirectUri = resolveRedirectUri(resourceAuthSettings, resourceSignInRequest); | ||
| TokenRequest tokenRequest = TokenRequest.builder() | ||
| .clientId(resourceAuthSettings.getClientId()) | ||
| .clientSecret(resourceAuthSettings.getClientSecret()) | ||
| .code(resourceSignInRequest.getCode()) | ||
| // TODO: do we need to support different? | ||
| .grantType("authorization_code") | ||
| .codeVerifier(resourceAuthSettings.getCodeVerifier()) | ||
| .redirectUri(resourceAuthSettings.getRedirectUri()) | ||
| .redirectUri(redirectUri) | ||
| .build(); | ||
|
|
||
| TokenResponse tokenResponse = doTokenCall(resourceAuthSettings.getTokenEndpoint(), tokenRequest.buildFormData()); | ||
|
|
@@ -36,7 +41,7 @@ public TokenResponse getToken(String resourceId, | |
| public TokenResponse getToken(String resourceId, | ||
| ResourceAuthSettings resourceAuthSettings, | ||
| String refreshToken) { | ||
| log.debug("Start Resource {} reresh token retrieval", resourceId); | ||
| log.debug("Start Resource {} refresh token retrieval", resourceId); | ||
| RefreshTokenRequest tokenRequest = RefreshTokenRequest.builder() | ||
| .clientId(resourceAuthSettings.getClientId()) | ||
| .clientSecret(resourceAuthSettings.getClientSecret()) | ||
|
|
@@ -49,6 +54,27 @@ public TokenResponse getToken(String resourceId, | |
| return tokenResponse; | ||
| } | ||
|
|
||
| private String resolveRedirectUri(ResourceAuthSettings resourceAuthSettings, | ||
| ResourceSignInRequest resourceSignInRequest) { | ||
| String requestRedirectUri = resourceSignInRequest.getRedirectUri(); | ||
|
|
||
| if (StringUtils.isNotBlank(requestRedirectUri)) { | ||
| List<String> effectiveAllowedUris = getEffectiveAllowedUris(resourceAuthSettings); | ||
|
Collaborator
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 doesn't make sense to build a new list and check if the url is presented there. |
||
| if (!effectiveAllowedUris.contains(requestRedirectUri)) { | ||
| throw new IllegalArgumentException( | ||
| "Provided redirect_uri is not in the list of allowed redirect URIs"); | ||
| } | ||
| return requestRedirectUri; | ||
| } | ||
|
|
||
| // Fallback to toolset's own redirect_uri (backward compatible) | ||
| return resourceAuthSettings.getRedirectUri(); | ||
| } | ||
|
|
||
| private List<String> getEffectiveAllowedUris(ResourceAuthSettings resourceAuthSettings) { | ||
| return RedirectUriHelper.collectAllowedRedirectUris(allowedRedirectUris, resourceAuthSettings); | ||
| } | ||
|
|
||
| private TokenResponse doTokenCall(String tokenEndpoint, String tokenRequest) { | ||
| return resourceAuthorizationClient.executePost( | ||
| tokenEndpoint, tokenRequest, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -6,6 +6,7 @@ | |
| import com.epam.aidial.core.credentials.data.registration.ClientRegistration; | ||
| import com.epam.aidial.core.credentials.data.registration.ClientRegistrationRequest; | ||
| import com.epam.aidial.core.credentials.data.registration.ClientRegistrationResponse; | ||
| import com.epam.aidial.core.credentials.service.RedirectUriHelper; | ||
| import com.epam.aidial.core.credentials.service.ResourceAuthorizationClient; | ||
| import com.epam.aidial.core.credentials.service.metadata.AuthorizationServerMetadataService; | ||
| import com.epam.aidial.core.credentials.service.metadata.ProtectedResourceMetadataService; | ||
|
|
@@ -40,6 +41,7 @@ public class DynamicResourceRegistrationStrategy implements ResourceRegistration | |
| private final AuthorizationServerMetadataService authorizationServerMetadataService; | ||
| private final ResourceAuthorizationClient resourceAuthorizationClient; | ||
| private final ProtectedResourceMetadataService protectedResourceMetadataService; | ||
| private final List<String> allowedRedirectUris; | ||
|
|
||
| /** | ||
| * Registers a protected resource dynamically using the authorization server's dynamic client registration | ||
|
|
@@ -72,7 +74,7 @@ public ClientRegistration register(String resourceId, String resourceEndpoint, R | |
|
|
||
| ClientRegistrationRequest clientRegistrationRequest = ClientRegistrationRequest.builder() | ||
| .clientName(resourceId) | ||
| .redirectUris(List.of(resourceAuthSettings.getRedirectUri())) | ||
| .redirectUris(collectRedirectUris(resourceAuthSettings)) | ||
| .build(); | ||
|
|
||
| ClientRegistrationResponse clientRegistrationResponse = resourceAuthorizationClient.executePost( | ||
|
|
@@ -97,4 +99,8 @@ public ClientRegistration register(String resourceId, String resourceEndpoint, R | |
| log.info("Finished dynamic registration for Resource: {}", resourceId); | ||
| return clientRegistration; | ||
| } | ||
|
|
||
| private List<String> collectRedirectUris(ResourceAuthSettings resourceAuthSettings) { | ||
| return RedirectUriHelper.collectAllowedRedirectUris(allowedRedirectUris, resourceAuthSettings); | ||
|
Collaborator
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. I think the more simple way is just to create a hashed set and add the url there. |
||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -62,7 +62,6 @@ protected ResourceAuthSettingsValidationFields getValidationFields(ResourceAuthS | |
| private ResourceAuthSettingsValidationFields getOauthWithStaticRegistrationValidationFields() { | ||
| return ResourceAuthSettingsValidationFields.builder() | ||
| .requiredFields(Set.of( | ||
| ResourceAuthSettingsField.REDIRECT_URI, | ||
| ResourceAuthSettingsField.CLIENT_ID, | ||
| ResourceAuthSettingsField.CLIENT_SECRET, | ||
| ResourceAuthSettingsField.AUTHORIZATION_ENDPOINT, | ||
|
|
@@ -90,7 +89,7 @@ private ResourceAuthSettingsValidationFields getOauthWithStaticRegistrationValid | |
| */ | ||
| private ResourceAuthSettingsValidationFields getOauthWithDynamicRegistrationValidationFields() { | ||
| return ResourceAuthSettingsValidationFields.builder() | ||
| .requiredFields(Set.of(ResourceAuthSettingsField.REDIRECT_URI)) | ||
|
Collaborator
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. Why is redirect uri not required any longer? |
||
| .requiredFields(Set.of()) | ||
| .forbiddenFields(Set.of( | ||
| ResourceAuthSettingsField.CLIENT_ID, | ||
| ResourceAuthSettingsField.CLIENT_SECRET, | ||
|
|
@@ -120,7 +119,6 @@ private ResourceAuthSettingsValidationFields getOauthWithDynamicRegistrationVali | |
| private ResourceAuthSettingsValidationFields getOauthWithNoAuthTypeChangeValidationFields() { | ||
| return ResourceAuthSettingsValidationFields.builder() | ||
| .requiredFields(Set.of( | ||
| ResourceAuthSettingsField.REDIRECT_URI, | ||
| ResourceAuthSettingsField.CLIENT_ID, | ||
| ResourceAuthSettingsField.AUTHORIZATION_ENDPOINT, | ||
| ResourceAuthSettingsField.TOKEN_ENDPOINT) | ||
|
|
||
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.
I think we don't need the helper any longer