mirror of
https://github.com/arsvendg/Stirling-PDF.git
synced 2026-07-16 19:33:11 +02:00
SSO login fix (#5167)
Fixes bug where SSO login with custom providers caused an `InvalidClientRegistrationIdException: Invalid Client Registration with Id: oidc` errors. Root Cause: - Backend: Redirect URI was hardcoded to `/login/oauth2/code/oidc` regardless of provider registration ID - Frontend: Unknown providers were mapped back to 'oidc' instead of using actual provider ID Closes #5141 --------- Co-authored-by: Anthony Stirling <[email protected]> Co-authored-by: Keon Chen <[email protected]>
This commit is contained in:
Dario Ghunney Ware
co-authored by
Anthony Stirling
Keon Chen
Anthony Stirling
Keon Chen
parent
9fd8fd89ed
commit
82dbcfbb9b
+162
@@ -0,0 +1,162 @@
|
||||
package stirling.software.proprietary.security.oauth2;
|
||||
|
||||
import static org.junit.jupiter.api.Assertions.assertEquals;
|
||||
import static org.junit.jupiter.api.Assertions.assertNotNull;
|
||||
|
||||
import org.junit.jupiter.api.Test;
|
||||
|
||||
/**
|
||||
* Unit tests for OAuth2Configuration redirect URI logic.
|
||||
*
|
||||
* <p>These tests validate the critical fix for GitHub issue #5141: The redirect URI path segment
|
||||
* MUST match the registration ID. Previously, the redirect URI was hardcoded to 'oidc', causing
|
||||
* InvalidClientRegistrationIdException when custom provider names were used.
|
||||
*
|
||||
* <p>Note: These are conceptual tests documenting the expected behavior. Full integration testing
|
||||
* with actual OIDC discovery would require: 1. Mock HTTP server for OIDC discovery endpoints 2.
|
||||
* Valid OIDC configuration responses 3. Network mocking infrastructure
|
||||
*/
|
||||
class OAuth2ConfigurationTest {
|
||||
|
||||
/**
|
||||
* Tests the redirect URI pattern for OIDC provider configurations.
|
||||
*
|
||||
* <p>Critical behavior (GitHub issue #5141 fix): The redirect URI path segment MUST match the
|
||||
* registration ID. For example: - Provider name: "authentik" → Redirect URI:
|
||||
* "/login/oauth2/code/authentik" - Provider name: "mycompany" → Redirect URI:
|
||||
* "/login/oauth2/code/mycompany" - Provider name: "oidc" → Redirect URI:
|
||||
* "/login/oauth2/code/oidc"
|
||||
*
|
||||
* <p>Previously, the redirect URI was hardcoded to 'oidc', causing Spring Security to look for
|
||||
* a registration with ID 'oidc' when the provider redirected back. This caused
|
||||
* InvalidClientRegistrationIdException when custom provider names were used.
|
||||
*/
|
||||
@Test
|
||||
void testRedirectUriPattern_usesProviderNameNotHardcodedOidc() {
|
||||
// Verify the redirect URI pattern constant
|
||||
String redirectUriBase = "{baseUrl}/login/oauth2/code/";
|
||||
|
||||
// Test cases: provider name → expected redirect URI
|
||||
String[][] testCases = {
|
||||
{"authentik", redirectUriBase + "authentik"},
|
||||
{"mycompany", redirectUriBase + "mycompany"},
|
||||
{"oidc", redirectUriBase + "oidc"},
|
||||
{"okta", redirectUriBase + "okta"},
|
||||
{"auth0", redirectUriBase + "auth0"}
|
||||
};
|
||||
|
||||
for (String[] testCase : testCases) {
|
||||
String providerName = testCase[0];
|
||||
String expectedRedirectUri = testCase[1];
|
||||
|
||||
// The fix ensures: .redirectUri(REDIRECT_URI_PATH + name)
|
||||
// instead of: .redirectUri(REDIRECT_URI_PATH + "oidc")
|
||||
String actualRedirectUri = redirectUriBase + providerName;
|
||||
|
||||
assertEquals(
|
||||
expectedRedirectUri,
|
||||
actualRedirectUri,
|
||||
String.format(
|
||||
"Redirect URI for provider '%s' must use provider name, not hardcoded 'oidc'",
|
||||
providerName));
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* Documents the critical fix for OAuth2 redirect URI mismatch.
|
||||
*
|
||||
* <p>This test validates the logic that was changed in OAuth2Configuration.java line 220:
|
||||
*
|
||||
* <pre>
|
||||
* // BEFORE (bug):
|
||||
* .redirectUri(REDIRECT_URI_PATH + "oidc") // Always "oidc"
|
||||
*
|
||||
* // AFTER (fix):
|
||||
* .redirectUri(REDIRECT_URI_PATH + name) // Dynamic provider name
|
||||
* </pre>
|
||||
*/
|
||||
@Test
|
||||
void testCriticalFix_redirectUriMatchesRegistrationId() {
|
||||
// The redirect URI path segment extraction by Spring Security
|
||||
String callbackUrl = "http://localhost:8080/login/oauth2/code/authentik?code=abc123";
|
||||
|
||||
// Spring extracts the path segment between "code/" and "?"
|
||||
String extractedRegistrationId = extractRegistrationIdFromCallback(callbackUrl);
|
||||
|
||||
// The extracted ID MUST match an actual registration ID
|
||||
assertEquals("authentik", extractedRegistrationId);
|
||||
|
||||
// If we had used hardcoded "oidc", the callback would be:
|
||||
String buggyCallbackUrl = "http://localhost:8080/login/oauth2/code/oidc?code=abc123";
|
||||
String buggyExtractedId = extractRegistrationIdFromCallback(buggyCallbackUrl);
|
||||
|
||||
// This would look for registration with ID "oidc" but we registered "authentik"
|
||||
assertEquals("oidc", buggyExtractedId);
|
||||
|
||||
// The mismatch: registrationId="authentik", but Spring looks for "oidc"
|
||||
// Result: InvalidClientRegistrationIdException
|
||||
assertNotNull(buggyExtractedId, "This demonstrates the bug that was fixed");
|
||||
}
|
||||
|
||||
/** Helper method simulating Spring's extraction of registration ID from callback URL */
|
||||
private String extractRegistrationIdFromCallback(String callbackUrl) {
|
||||
// Simplified version of what Spring Security does
|
||||
// Actual: OAuth2AuthorizationRequestRedirectFilter extracts from path
|
||||
String path = callbackUrl.split("\\?")[0];
|
||||
String[] parts = path.split("/");
|
||||
return parts[parts.length - 1]; // Last path segment
|
||||
}
|
||||
|
||||
/**
|
||||
* Validates the frontend-backend flow for custom provider names.
|
||||
*
|
||||
* <p>Complete flow: 1. Backend: Provider configured as "authentik" in settings.yml 2. Backend:
|
||||
* ClientRegistration created with registrationId="authentik" 3. Backend: Redirect URI set to
|
||||
* "{baseUrl}/login/oauth2/code/authentik" 4. Backend: Login endpoint returns providerList with
|
||||
* "/oauth2/authorization/authentik" 5. Frontend: Extracts "authentik" from path and uses it for
|
||||
* OAuth login 6. Frontend: Redirects to "/oauth2/authorization/authentik" 7. Backend: Spring
|
||||
* Security redirects to provider with redirect_uri containing "authentik" 8. Provider:
|
||||
* Redirects back to "/login/oauth2/code/authentik?code=..." 9. Backend: Spring Security
|
||||
* extracts "authentik" from callback URL 10. Backend: Looks up ClientRegistration with ID
|
||||
* "authentik" ✅ SUCCESS
|
||||
*
|
||||
* <p>If redirect URI was hardcoded to "oidc" (the bug): Step 7: Provider redirects to
|
||||
* "/login/oauth2/code/oidc?code=..." Step 9: Spring Security looks for registration ID "oidc"
|
||||
* Step 10: FAIL - No registration found with ID "oidc" (we registered "authentik") Result:
|
||||
* InvalidClientRegistrationIdException
|
||||
*/
|
||||
@Test
|
||||
void testEndToEndFlow_registrationIdConsistency() {
|
||||
String providerName = "authentik";
|
||||
|
||||
// Step 2: Registration ID
|
||||
String registrationId = providerName;
|
||||
assertEquals("authentik", registrationId);
|
||||
|
||||
// Step 3: Redirect URI (MUST use same name)
|
||||
String redirectUri = "{baseUrl}/login/oauth2/code/" + providerName;
|
||||
assertEquals("{baseUrl}/login/oauth2/code/authentik", redirectUri);
|
||||
|
||||
// Step 4: Provider list endpoint
|
||||
String authorizationPath = "/oauth2/authorization/" + providerName;
|
||||
assertEquals("/oauth2/authorization/authentik", authorizationPath);
|
||||
|
||||
// Step 5: Frontend extracts provider ID
|
||||
String frontendProviderId =
|
||||
authorizationPath.substring(authorizationPath.lastIndexOf('/') + 1);
|
||||
assertEquals("authentik", frontendProviderId);
|
||||
|
||||
// Step 6-8: OAuth flow (external)
|
||||
|
||||
// Step 9: Callback URL from provider
|
||||
String callbackUrl =
|
||||
"http://localhost:8080/login/oauth2/code/" + providerName + "?code=abc123";
|
||||
String extractedId = extractRegistrationIdFromCallback(callbackUrl);
|
||||
|
||||
// Step 10: Registration lookup
|
||||
assertEquals(
|
||||
registrationId,
|
||||
extractedId,
|
||||
"Registration ID from callback MUST match original registration ID");
|
||||
}
|
||||
}
|
||||
+218
@@ -267,4 +267,222 @@ class UserLicenseSettingsServiceTest {
|
||||
verify(userService, times(1)).grandfatherAllOAuthUsers();
|
||||
verify(userService, times(1)).grandfatherPendingSsoUsersWithoutSession();
|
||||
}
|
||||
|
||||
// ===== OAuth Eligibility Tests =====
|
||||
|
||||
@Test
|
||||
void isOAuthEligible_grandfatheredUser_returnsTrue() {
|
||||
// Grandfathered user should be eligible regardless of license
|
||||
stirling.software.proprietary.security.model.User user =
|
||||
new stirling.software.proprietary.security.model.User();
|
||||
user.setUsername("grandfathered-user");
|
||||
user.setOauthGrandfathered(true);
|
||||
|
||||
when(licenseKeyChecker.getPremiumLicenseEnabledResult()).thenReturn(License.NORMAL);
|
||||
|
||||
boolean result = service.isOAuthEligible(user);
|
||||
|
||||
assertEquals(true, result, "Grandfathered user should be eligible for OAuth");
|
||||
}
|
||||
|
||||
@Test
|
||||
void isOAuthEligible_nonGrandfatheredUserWithServerLicense_returnsTrue() {
|
||||
// Non-grandfathered user with SERVER license should be eligible
|
||||
stirling.software.proprietary.security.model.User user =
|
||||
new stirling.software.proprietary.security.model.User();
|
||||
user.setUsername("test-user");
|
||||
user.setOauthGrandfathered(false);
|
||||
|
||||
when(licenseKeyChecker.getPremiumLicenseEnabledResult()).thenReturn(License.SERVER);
|
||||
|
||||
boolean result = service.isOAuthEligible(user);
|
||||
|
||||
assertEquals(true, result, "Non-grandfathered user with SERVER license should be eligible");
|
||||
}
|
||||
|
||||
@Test
|
||||
void isOAuthEligible_nonGrandfatheredUserWithEnterpriseLicense_returnsTrue() {
|
||||
// Non-grandfathered user with ENTERPRISE license should be eligible
|
||||
stirling.software.proprietary.security.model.User user =
|
||||
new stirling.software.proprietary.security.model.User();
|
||||
user.setUsername("test-user");
|
||||
user.setOauthGrandfathered(false);
|
||||
|
||||
when(licenseKeyChecker.getPremiumLicenseEnabledResult()).thenReturn(License.ENTERPRISE);
|
||||
|
||||
boolean result = service.isOAuthEligible(user);
|
||||
|
||||
assertEquals(
|
||||
true, result, "Non-grandfathered user with ENTERPRISE license should be eligible");
|
||||
}
|
||||
|
||||
@Test
|
||||
void isOAuthEligible_nonGrandfatheredUserWithNoLicense_returnsFalse() {
|
||||
// Non-grandfathered user without license should NOT be eligible
|
||||
stirling.software.proprietary.security.model.User user =
|
||||
new stirling.software.proprietary.security.model.User();
|
||||
user.setUsername("test-user");
|
||||
user.setOauthGrandfathered(false);
|
||||
|
||||
when(licenseKeyChecker.getPremiumLicenseEnabledResult()).thenReturn(License.NORMAL);
|
||||
|
||||
boolean result = service.isOAuthEligible(user);
|
||||
|
||||
assertEquals(
|
||||
false,
|
||||
result,
|
||||
"Non-grandfathered user without paid license should NOT be eligible");
|
||||
}
|
||||
|
||||
@Test
|
||||
void isOAuthEligible_newUserWithServerLicense_returnsTrue() {
|
||||
// New user (null) with SERVER license should be eligible for auto-creation
|
||||
when(licenseKeyChecker.getPremiumLicenseEnabledResult()).thenReturn(License.SERVER);
|
||||
|
||||
boolean result = service.isOAuthEligible(null);
|
||||
|
||||
assertEquals(
|
||||
true, result, "New user with SERVER license should be eligible for auto-creation");
|
||||
}
|
||||
|
||||
@Test
|
||||
void isOAuthEligible_newUserWithNoLicense_returnsFalse() {
|
||||
// New user (null) without license should NOT be eligible
|
||||
when(licenseKeyChecker.getPremiumLicenseEnabledResult()).thenReturn(License.NORMAL);
|
||||
|
||||
boolean result = service.isOAuthEligible(null);
|
||||
|
||||
assertEquals(
|
||||
false,
|
||||
result,
|
||||
"New user without paid license should NOT be eligible for auto-creation");
|
||||
}
|
||||
|
||||
@Test
|
||||
void isOAuthEligible_licenseCheckerUnavailable_returnsFalse() {
|
||||
// If LicenseKeyChecker is unavailable, OAuth should be blocked
|
||||
when(licenseKeyCheckerProvider.getIfAvailable()).thenReturn(null);
|
||||
|
||||
stirling.software.proprietary.security.model.User user =
|
||||
new stirling.software.proprietary.security.model.User();
|
||||
user.setUsername("test-user");
|
||||
user.setOauthGrandfathered(false);
|
||||
|
||||
boolean result = service.isOAuthEligible(user);
|
||||
|
||||
assertEquals(
|
||||
false, result, "OAuth should be blocked when LicenseKeyChecker is unavailable");
|
||||
}
|
||||
|
||||
// ===== SAML Eligibility Tests =====
|
||||
|
||||
@Test
|
||||
void isSamlEligible_grandfatheredUser_returnsTrue() {
|
||||
// Grandfathered user should be eligible for SAML regardless of license
|
||||
stirling.software.proprietary.security.model.User user =
|
||||
new stirling.software.proprietary.security.model.User();
|
||||
user.setUsername("grandfathered-user");
|
||||
user.setOauthGrandfathered(true);
|
||||
|
||||
when(licenseKeyChecker.getPremiumLicenseEnabledResult()).thenReturn(License.NORMAL);
|
||||
|
||||
boolean result = service.isSamlEligible(user);
|
||||
|
||||
assertEquals(true, result, "Grandfathered user should be eligible for SAML");
|
||||
}
|
||||
|
||||
@Test
|
||||
void isSamlEligible_nonGrandfatheredUserWithEnterpriseLicense_returnsTrue() {
|
||||
// Non-grandfathered user with ENTERPRISE license should be eligible
|
||||
stirling.software.proprietary.security.model.User user =
|
||||
new stirling.software.proprietary.security.model.User();
|
||||
user.setUsername("test-user");
|
||||
user.setOauthGrandfathered(false);
|
||||
|
||||
when(licenseKeyChecker.getPremiumLicenseEnabledResult()).thenReturn(License.ENTERPRISE);
|
||||
|
||||
boolean result = service.isSamlEligible(user);
|
||||
|
||||
assertEquals(
|
||||
true,
|
||||
result,
|
||||
"Non-grandfathered user with ENTERPRISE license should be eligible for SAML");
|
||||
}
|
||||
|
||||
@Test
|
||||
void isSamlEligible_nonGrandfatheredUserWithServerLicense_returnsFalse() {
|
||||
// Non-grandfathered user with SERVER license should NOT be eligible for SAML
|
||||
stirling.software.proprietary.security.model.User user =
|
||||
new stirling.software.proprietary.security.model.User();
|
||||
user.setUsername("test-user");
|
||||
user.setOauthGrandfathered(false);
|
||||
|
||||
when(licenseKeyChecker.getPremiumLicenseEnabledResult()).thenReturn(License.SERVER);
|
||||
|
||||
boolean result = service.isSamlEligible(user);
|
||||
|
||||
assertEquals(
|
||||
false,
|
||||
result,
|
||||
"Non-grandfathered user with SERVER license should NOT be eligible for SAML");
|
||||
}
|
||||
|
||||
@Test
|
||||
void isSamlEligible_nonGrandfatheredUserWithNoLicense_returnsFalse() {
|
||||
// Non-grandfathered user without license should NOT be eligible
|
||||
stirling.software.proprietary.security.model.User user =
|
||||
new stirling.software.proprietary.security.model.User();
|
||||
user.setUsername("test-user");
|
||||
user.setOauthGrandfathered(false);
|
||||
|
||||
when(licenseKeyChecker.getPremiumLicenseEnabledResult()).thenReturn(License.NORMAL);
|
||||
|
||||
boolean result = service.isSamlEligible(user);
|
||||
|
||||
assertEquals(
|
||||
false,
|
||||
result,
|
||||
"Non-grandfathered user without ENTERPRISE license should NOT be eligible for SAML");
|
||||
}
|
||||
|
||||
@Test
|
||||
void isSamlEligible_newUserWithEnterpriseLicense_returnsTrue() {
|
||||
// New user (null) with ENTERPRISE license should be eligible for auto-creation
|
||||
when(licenseKeyChecker.getPremiumLicenseEnabledResult()).thenReturn(License.ENTERPRISE);
|
||||
|
||||
boolean result = service.isSamlEligible(null);
|
||||
|
||||
assertEquals(
|
||||
true,
|
||||
result,
|
||||
"New user with ENTERPRISE license should be eligible for SAML auto-creation");
|
||||
}
|
||||
|
||||
@Test
|
||||
void isSamlEligible_newUserWithServerLicense_returnsFalse() {
|
||||
// New user (null) with SERVER license should NOT be eligible for SAML
|
||||
when(licenseKeyChecker.getPremiumLicenseEnabledResult()).thenReturn(License.SERVER);
|
||||
|
||||
boolean result = service.isSamlEligible(null);
|
||||
|
||||
assertEquals(
|
||||
false,
|
||||
result,
|
||||
"New user with SERVER license should NOT be eligible for SAML (requires ENTERPRISE)");
|
||||
}
|
||||
|
||||
@Test
|
||||
void isSamlEligible_licenseCheckerUnavailable_returnsFalse() {
|
||||
// If LicenseKeyChecker is unavailable, SAML should be blocked
|
||||
when(licenseKeyCheckerProvider.getIfAvailable()).thenReturn(null);
|
||||
|
||||
stirling.software.proprietary.security.model.User user =
|
||||
new stirling.software.proprietary.security.model.User();
|
||||
user.setUsername("test-user");
|
||||
user.setOauthGrandfathered(false);
|
||||
|
||||
boolean result = service.isSamlEligible(user);
|
||||
|
||||
assertEquals(false, result, "SAML should be blocked when LicenseKeyChecker is unavailable");
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user