Skip to content

Commit c0f9bec

Browse files
Damans227Damans227
committed
check every keycloak login code instead of reusing the last token
1 parent 17294ca commit c0f9bec

2 files changed

Lines changed: 112 additions & 37 deletions

File tree

‎plugins/user-authenticators/oauth2/src/main/java/org/apache/cloudstack/oauth2/keycloak/KeycloakOAuth2Provider.java‎

Lines changed: 58 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -24,13 +24,15 @@
2424
import java.util.ArrayList;
2525
import java.util.Base64;
2626
import java.util.List;
27+
import java.util.concurrent.TimeUnit;
2728

2829
import javax.inject.Inject;
2930
import javax.ws.rs.core.HttpHeaders;
3031

3132
import org.apache.cloudstack.auth.UserOAuth2Authenticator;
3233
import org.apache.cloudstack.oauth2.dao.OauthProviderDao;
3334
import org.apache.cloudstack.oauth2.vo.OauthProviderVO;
35+
import org.apache.commons.codec.digest.DigestUtils;
3436
import org.apache.commons.lang3.StringUtils;
3537
import org.apache.cxf.rs.security.jose.jws.JwsJwtCompactConsumer;
3638
import org.apache.cxf.rs.security.jose.jwt.JwtClaims;
@@ -46,6 +48,8 @@
4648
import com.cloud.exception.CloudAuthenticationException;
4749
import com.cloud.utils.component.AdapterBase;
4850
import com.cloud.utils.exception.CloudRuntimeException;
51+
import com.github.benmanes.caffeine.cache.Cache;
52+
import com.github.benmanes.caffeine.cache.Caffeine;
4953
import com.google.gson.JsonElement;
5054
import com.google.gson.JsonObject;
5155
import com.google.gson.JsonParser;
@@ -54,13 +58,17 @@ public class KeycloakOAuth2Provider extends AdapterBase implements UserOAuth2Aut
5458

5559
public static final String KEYCLOAK_PROVIDER = "keycloak";
5660

57-
protected String idToken = null;
58-
5961
@Inject
6062
OauthProviderDao oauthProviderDao;
6163

6264
private CloseableHttpClient httpClient;
6365

66+
private final Cache<String, String> validatedEmailCache =
67+
Caffeine.newBuilder()
68+
.expireAfterWrite(60, TimeUnit.SECONDS)
69+
.maximumSize(1024)
70+
.build();
71+
6472
public KeycloakOAuth2Provider() {
6573
this(HttpClientBuilder.create().build());
6674
}
@@ -95,11 +103,14 @@ public boolean verifyUser(String email, String secretCode, Long domainId) {
95103
throw new CloudAuthenticationException("Keycloak provider is not registered, so user cannot be verified");
96104
}
97105

98-
String verifiedEmail = verifySecretCodeAndFetchEmail(secretCode, domainId);
106+
String verifiedEmail = consumeValidatedEmailFromCache(secretCode);
107+
if (StringUtils.isBlank(verifiedEmail)) {
108+
verifiedEmail = verifySecretCodeAndFetchEmail(secretCode, domainId);
109+
consumeValidatedEmailFromCache(secretCode);
110+
}
99111
if (StringUtils.isBlank(verifiedEmail) || !email.equals(verifiedEmail)) {
100112
throw new CloudRuntimeException("Unable to verify the email address with the provided secret");
101113
}
102-
clearIdToken();
103114

104115
return true;
105116
}
@@ -116,46 +127,47 @@ public String verifySecretCodeAndFetchEmail(String secretCode, Long domainId) {
116127
throw new CloudAuthenticationException("Keycloak provider is not registered, so user cannot be verified");
117128
}
118129

119-
if (StringUtils.isBlank(idToken)) {
120-
String auth = provider.getClientId() + ":" + provider.getSecretKey();
121-
String encodedAuth = Base64.getEncoder().encodeToString(auth.getBytes(StandardCharsets.UTF_8));
130+
String auth = provider.getClientId() + ":" + provider.getSecretKey();
131+
String encodedAuth = Base64.getEncoder().encodeToString(auth.getBytes(StandardCharsets.UTF_8));
122132

123-
List<NameValuePair> params = new ArrayList<>();
124-
params.add(new BasicNameValuePair("grant_type", "authorization_code"));
125-
params.add(new BasicNameValuePair("code", secretCode));
126-
params.add(new BasicNameValuePair("redirect_uri", provider.getRedirectUri()));
133+
List<NameValuePair> params = new ArrayList<>();
134+
params.add(new BasicNameValuePair("grant_type", "authorization_code"));
135+
params.add(new BasicNameValuePair("code", secretCode));
136+
params.add(new BasicNameValuePair("redirect_uri", provider.getRedirectUri()));
127137

128-
HttpPost post = new HttpPost(provider.getTokenUrl());
129-
post.setHeader(HttpHeaders.AUTHORIZATION, "Basic " + encodedAuth);
138+
HttpPost post = new HttpPost(provider.getTokenUrl());
139+
post.setHeader(HttpHeaders.AUTHORIZATION, "Basic " + encodedAuth);
130140

131-
try {
132-
post.setEntity(new UrlEncodedFormEntity(params));
133-
} catch (UnsupportedEncodingException e) {
134-
throw new CloudRuntimeException("Unable to generate URL parameters: " + e.getMessage());
135-
}
136-
137-
try (CloseableHttpResponse response = httpClient.execute(post)) {
138-
String body = EntityUtils.toString(response.getEntity());
141+
try {
142+
post.setEntity(new UrlEncodedFormEntity(params));
143+
} catch (UnsupportedEncodingException e) {
144+
throw new CloudRuntimeException("Unable to generate URL parameters: " + e.getMessage());
145+
}
139146

140-
if (response.getStatusLine().getStatusCode() != 200) {
141-
throw new CloudRuntimeException("Keycloak error during token generation: " + body);
142-
}
147+
String idToken;
148+
try (CloseableHttpResponse response = httpClient.execute(post)) {
149+
String body = EntityUtils.toString(response.getEntity());
143150

144-
JsonObject json = JsonParser.parseString(body).getAsJsonObject();
145-
JsonElement fetchedIdToken = json.get("id_token");
146-
if (fetchedIdToken == null) {
147-
throw new CloudRuntimeException("No id_token found in token");
148-
}
149-
String idTokenAsString = fetchedIdToken.getAsString();
150-
validateIdToken(idTokenAsString , provider);
151+
if (response.getStatusLine().getStatusCode() != 200) {
152+
throw new CloudRuntimeException("Keycloak error during token generation: " + body);
153+
}
151154

152-
this.idToken = idTokenAsString ;
153-
} catch (IOException e) {
154-
throw new CloudRuntimeException("Unable to connect to Keycloak server", e);
155+
JsonObject json = JsonParser.parseString(body).getAsJsonObject();
156+
JsonElement fetchedIdToken = json.get("id_token");
157+
if (fetchedIdToken == null) {
158+
throw new CloudRuntimeException("No id_token found in token");
155159
}
160+
idToken = fetchedIdToken.getAsString();
161+
validateIdToken(idToken, provider);
162+
} catch (IOException e) {
163+
throw new CloudRuntimeException("Unable to connect to Keycloak server", e);
156164
}
157165

158-
return obtainEmail(idToken, provider);
166+
String email = obtainEmail(idToken, provider);
167+
if (StringUtils.isNotBlank(email)) {
168+
validatedEmailCache.put(getCacheKey(secretCode), email);
169+
}
170+
return email;
159171
}
160172

161173
@Override
@@ -183,8 +195,17 @@ private String obtainEmail(String idTokenStr, OauthProviderVO provider) {
183195
return (String) claims.getClaim("email");
184196
}
185197

186-
protected void clearIdToken() {
187-
idToken = null;
198+
private String getCacheKey(final String secretCode) {
199+
return DigestUtils.sha256Hex(secretCode);
200+
}
201+
202+
private String consumeValidatedEmailFromCache(final String secretCode) {
203+
final String key = getCacheKey(secretCode);
204+
final String email = validatedEmailCache.getIfPresent(key);
205+
if (email != null) {
206+
validatedEmailCache.invalidate(key);
207+
}
208+
return email;
188209
}
189210

190211
public void setHttpClient(CloseableHttpClient httpClient) {

‎plugins/user-authenticators/oauth2/src/test/java/org/apache/cloudstack/oauth2/keycloak/KeycloakOAuth2ProviderTest.java‎

Lines changed: 54 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,8 @@
2020
import static org.junit.Assert.assertTrue;
2121
import static org.mockito.ArgumentMatchers.any;
2222
import static org.mockito.Mockito.mock;
23+
import static org.mockito.Mockito.times;
24+
import static org.mockito.Mockito.verify;
2325
import static org.mockito.Mockito.when;
2426

2527
import java.io.ByteArrayInputStream;
@@ -218,6 +220,58 @@ public void testVerifyUserEmail() throws IOException {
218220
assertTrue("User successfully verified", result);
219221
}
220222

223+
@Test
224+
public void testNewCodeIsExchangedAfterFailedLogin() throws IOException {
225+
when(oauthProviderDao.findByProviderAndDomainWithGlobalFallback("keycloak", null)).thenReturn(mockProviderVO);
226+
CloseableHttpResponse first = tokenResponseFor("first@example.com");
227+
CloseableHttpResponse second = tokenResponseFor("second@example.com");
228+
when(httpClient.execute(any(HttpPost.class))).thenReturn(first, second);
229+
230+
// the first login stops after the code exchange, so verifyUser is never called for it
231+
assertEquals("first@example.com", provider.verifySecretCodeAndFetchEmail("code-1"));
232+
assertEquals("second@example.com", provider.verifySecretCodeAndFetchEmail("code-2"));
233+
verify(httpClient, times(2)).execute(any(HttpPost.class));
234+
}
235+
236+
@Test(expected = CloudRuntimeException.class)
237+
public void testNewCodeDoesNotReuseEmailFromEarlierCode() throws IOException {
238+
when(oauthProviderDao.findByProviderAndDomainWithGlobalFallback("keycloak", null)).thenReturn(mockProviderVO);
239+
CloseableHttpResponse first = tokenResponseFor("first@example.com");
240+
CloseableHttpResponse second = tokenResponseFor("second@example.com");
241+
when(httpClient.execute(any(HttpPost.class))).thenReturn(first, second);
242+
243+
provider.verifySecretCodeAndFetchEmail("code-1");
244+
provider.verifyUser("first@example.com", "code-2");
245+
}
246+
247+
@Test
248+
public void testVerifyUserUsesEmailFetchedForSameCode() throws IOException {
249+
when(oauthProviderDao.findByProviderAndDomainWithGlobalFallback("keycloak", null)).thenReturn(mockProviderVO);
250+
CloseableHttpResponse response = tokenResponseFor("user@example.com");
251+
when(httpClient.execute(any(HttpPost.class))).thenReturn(response);
252+
253+
assertEquals("user@example.com", provider.verifySecretCodeAndFetchEmail("code-1"));
254+
assertTrue(provider.verifyUser("user@example.com", "code-1"));
255+
verify(httpClient, times(1)).execute(any(HttpPost.class));
256+
}
257+
258+
private CloseableHttpResponse tokenResponseFor(String email) throws IOException {
259+
String header = "{\"alg\":\"none\"}";
260+
String payload = "{\"aud\":[\"test-client\"],\"email\":\"" + email + "\",\"iss\":\"http://keycloak\",\"sub\":\"12345\"}";
261+
String fakeJwt = Base64.getUrlEncoder().withoutPadding().encodeToString(header.getBytes()) + "."
262+
+ Base64.getUrlEncoder().withoutPadding().encodeToString(payload.getBytes()) + ".not-checked-signature";
263+
264+
CloseableHttpResponse response = mock(CloseableHttpResponse.class);
265+
StatusLine statusLine = mock(StatusLine.class);
266+
HttpEntity entity = mock(HttpEntity.class);
267+
when(statusLine.getStatusCode()).thenReturn(200);
268+
when(response.getStatusLine()).thenReturn(statusLine);
269+
String body = "{\"id_token\":\"" + fakeJwt + "\", \"access_token\":\"acc-123\"}";
270+
when(entity.getContent()).thenReturn(new ByteArrayInputStream(body.getBytes(StandardCharsets.UTF_8)));
271+
when(response.getEntity()).thenReturn(entity);
272+
return response;
273+
}
274+
221275
@Test
222276
public void testGetDescription() {
223277
assertEquals("Keycloak OAuth2 Provider Plugin", provider.getDescription());

0 commit comments

Comments
 (0)