-
Notifications
You must be signed in to change notification settings - Fork 10
Public Webforms: Authentication #1791
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
Changes from 4 commits
787a76c
ec9ec73
0e9f605
0388456
19bbafa
ab59764
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,37 @@ | ||
| package org.commcare.formplayer.auth; | ||
|
|
||
| import org.commcare.formplayer.util.Constants; | ||
| import org.springframework.http.HttpHeaders; | ||
| import org.springframework.util.Assert; | ||
|
|
||
| /** | ||
| * {@link HqAuth} for a public web apps session. | ||
| * | ||
| * Emits the credential pair HQ requires to recognize a public session on its receiver/restore | ||
| * endpoints: the {@code public_form_session_key} cookie carrying the session key together with the | ||
| * {@code CommCare-Public-Session: true} header. | ||
| */ | ||
| public class PublicFormSessionAuth implements HqAuth { | ||
|
|
||
| private final String sessionKey; | ||
|
|
||
| public PublicFormSessionAuth(String sessionKey) { | ||
| Assert.hasText(sessionKey, "A public form session key is required"); | ||
| this.sessionKey = sessionKey; | ||
| } | ||
|
|
||
| @Override | ||
| public HttpHeaders getAuthHeaders() { | ||
| return new HttpHeaders() { | ||
| { | ||
| add("Cookie", Constants.PUBLIC_FORM_SESSION_COOKIE_NAME + "=" + sessionKey); | ||
| add(Constants.PUBLIC_FORM_SESSION_HEADER, Constants.PUBLIC_FORM_SESSION_HEADER_VALUE); | ||
| } | ||
| }; | ||
| } | ||
|
|
||
| @Override | ||
| public String toString() { | ||
| return "PublicFormSessionAuth"; | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,15 @@ | ||
| package org.commcare.formplayer.auth; | ||
|
|
||
| import lombok.Value; | ||
|
|
||
| /** | ||
| * Typed credential for a public web apps session (one-time link). | ||
| * | ||
| * Wraps the value of the {@code public_form_session_key} cookie so that the | ||
| * {@link org.commcare.formplayer.services.HqUserDetailsService} can distinguish a public session | ||
| * from a regular Django session. | ||
| */ | ||
| @Value | ||
| public class PublicSessionCredential { | ||
| String sessionKey; | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,31 @@ | ||
| package org.commcare.formplayer.beans.auth; | ||
|
|
||
| import com.fasterxml.jackson.annotation.JsonIgnoreProperties; | ||
|
|
||
| import java.io.Serializable; | ||
|
|
||
| /** | ||
| * HMAC-signed body sent to HQ's session_details endpoint for a public web apps session. | ||
| * | ||
| * Serializes to {@code {"publicSessionKey": ..., "domain": ...}}. HQ treats a request with a | ||
| * truthy {@code publicSessionKey} as a public session lookup, in contrast to | ||
| * {@link HqSessionKeyBean} which sends {@code sessionId}. | ||
| */ | ||
| @JsonIgnoreProperties(ignoreUnknown = true) | ||
| public class HqPublicSessionKeyBean implements Serializable { | ||
| private String publicSessionKey; | ||
| private String domain; | ||
|
|
||
| public HqPublicSessionKeyBean(String domain, String publicSessionKey) { | ||
| this.domain = domain; | ||
| this.publicSessionKey = publicSessionKey; | ||
| } | ||
|
|
||
| public String getPublicSessionKey() { | ||
| return publicSessionKey; | ||
| } | ||
|
|
||
| public String getDomain() { | ||
| return domain; | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,7 +1,9 @@ | ||
| package org.commcare.formplayer.services; | ||
|
|
||
| import com.fasterxml.jackson.databind.ObjectMapper; | ||
| import org.commcare.formplayer.auth.PublicSessionCredential; | ||
| import org.commcare.formplayer.auth.UserDomainPreAuthPrincipal; | ||
| import org.commcare.formplayer.beans.auth.HqPublicSessionKeyBean; | ||
| import org.commcare.formplayer.beans.auth.HqSessionKeyBean; | ||
| import org.commcare.formplayer.beans.auth.HqUserDetailsBean; | ||
| import org.commcare.formplayer.exceptions.SessionAuthUnavailableException; | ||
|
|
@@ -38,10 +40,23 @@ public class HqUserDetailsService implements AuthenticationUserDetailsService<Pr | |
| private WebClient webClient; | ||
|
|
||
| public HqUserDetailsBean getUserDetails(String domain, String sessionKey) { | ||
| return requestUserDetails(domain, new HqSessionKeyBean(domain, sessionKey)); | ||
| } | ||
|
|
||
| /** | ||
| * Look up user details for a public web apps session. Sends the session key as | ||
| * {@code publicSessionKey} rather than {@code sessionId} so HQ knows to resolves it against | ||
| * the public form session. | ||
| */ | ||
| public HqUserDetailsBean getPublicUserDetails(String domain, String publicSessionKey) { | ||
| return requestUserDetails(domain, new HqPublicSessionKeyBean(domain, publicSessionKey)); | ||
| } | ||
|
|
||
| private HqUserDetailsBean requestUserDetails(String domain, Object requestBody) { | ||
| HttpHeaders headers = new HttpHeaders(); | ||
| String data = null; | ||
| String data; | ||
| try { | ||
| data = objectMapper.writeValueAsString(new HqSessionKeyBean(domain, sessionKey)); | ||
| data = objectMapper.writeValueAsString(requestBody); | ||
| headers.set("X-MAC-DIGEST", getHmac(data)); | ||
| } catch (Exception e) { | ||
| throw new UserDetailsException(e); | ||
|
|
@@ -73,9 +88,14 @@ private String getHmac(String data) throws Exception { | |
| @Override | ||
| public UserDetails loadUserDetails(PreAuthenticatedAuthenticationToken token) throws UsernameNotFoundException { | ||
| final UserDomainPreAuthPrincipal principal = (UserDomainPreAuthPrincipal) token.getPrincipal(); | ||
| final String sessionId = (String) token.getCredentials(); | ||
| final Object credentials = token.getCredentials(); | ||
| try { | ||
| HqUserDetailsBean userDetails = getUserDetails(principal.getDomain(), sessionId); | ||
| HqUserDetailsBean userDetails; | ||
| if (credentials instanceof PublicSessionCredential publicCredential) { | ||
| userDetails = getPublicUserDetails(principal.getDomain(), publicCredential.getSessionKey()); | ||
| } else { | ||
| userDetails = getUserDetails(principal.getDomain(), (String) credentials); | ||
| } | ||
| if (!userDetails.isAuthorized(principal.getDomain(), principal.getUsername())) { | ||
|
Contributor
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. Nit: I believe at this point
Contributor
Author
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. Yep, fair point 19bbafa |
||
| throw new UsernameNotFoundException("Unable to authenticate user in requested domain"); | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,89 @@ | ||
| package org.commcare.formplayer.aspects; | ||
|
|
||
| import static org.junit.jupiter.api.Assertions.assertEquals; | ||
| import static org.junit.jupiter.api.Assertions.assertNull; | ||
| import static org.junit.jupiter.api.Assertions.assertTrue; | ||
|
|
||
| import org.commcare.formplayer.auth.DjangoAuth; | ||
| import org.commcare.formplayer.auth.HqAuth; | ||
| import org.commcare.formplayer.auth.PublicFormSessionAuth; | ||
| import org.commcare.formplayer.beans.auth.HqUserDetailsBean; | ||
| import org.commcare.formplayer.util.RequestUtils; | ||
| import org.junit.jupiter.api.Test; | ||
| import org.mockito.MockedStatic; | ||
| import org.mockito.Mockito; | ||
|
|
||
| import java.util.Optional; | ||
|
|
||
| /** | ||
| * Unit tests for {@link UserRestoreAspect#getHqAuth} credential selection, in particular the | ||
| * choice between a Django session and a public web apps session. | ||
| */ | ||
| public class UserRestoreAspectTest { | ||
|
|
||
| private final UserRestoreAspect aspect = new UserRestoreAspect(); | ||
|
|
||
| private HqUserDetailsBean bean(boolean isPublicSession, String authToken) { | ||
| HqUserDetailsBean bean = new HqUserDetailsBean("domain", new String[]{"domain"}, "user", | ||
| false, new String[]{}, new String[]{}); | ||
| bean.setPublicSession(isPublicSession); | ||
| bean.setAuthToken(authToken); | ||
| return bean; | ||
| } | ||
|
|
||
| @Test | ||
| public void publicSession_usesPublicFormSessionAuthWithTheSessionKey() { | ||
| try (MockedStatic<RequestUtils> mocked = Mockito.mockStatic(RequestUtils.class)) { | ||
| mocked.when(RequestUtils::getUserDetails).thenReturn(Optional.of(bean(true, "pkey"))); | ||
|
|
||
| HqAuth auth = aspect.getHqAuth(null); | ||
|
|
||
| assertTrue(auth instanceof PublicFormSessionAuth); | ||
| assertEquals("public_form_session_key=pkey", auth.getAuthHeaders().getFirst("Cookie")); | ||
| } | ||
| } | ||
|
|
||
| @Test | ||
| public void publicSession_winsEvenWhenASessionidCookieIsAlsoPresent() { | ||
| try (MockedStatic<RequestUtils> mocked = Mockito.mockStatic(RequestUtils.class)) { | ||
| mocked.when(RequestUtils::getUserDetails).thenReturn(Optional.of(bean(true, "pkey"))); | ||
|
|
||
| // Both signals present: the public credential must win, matching inbound selection. | ||
| HqAuth auth = aspect.getHqAuth("sessionid-value"); | ||
|
|
||
| assertTrue(auth instanceof PublicFormSessionAuth); | ||
| } | ||
| } | ||
|
|
||
| @Test | ||
| public void regularSession_usesDjangoAuth() { | ||
| try (MockedStatic<RequestUtils> mocked = Mockito.mockStatic(RequestUtils.class)) { | ||
| mocked.when(RequestUtils::getUserDetails).thenReturn(Optional.of(bean(false, null))); | ||
|
|
||
| HqAuth auth = aspect.getHqAuth("sessionid-value"); | ||
|
|
||
| assertTrue(auth instanceof DjangoAuth); | ||
| } | ||
| } | ||
|
|
||
| @Test | ||
| public void noUserDetailsWithSessionToken_usesDjangoAuth() { | ||
| try (MockedStatic<RequestUtils> mocked = Mockito.mockStatic(RequestUtils.class)) { | ||
| mocked.when(RequestUtils::getUserDetails).thenReturn(Optional.empty()); | ||
|
|
||
| HqAuth auth = aspect.getHqAuth("sessionid-value"); | ||
|
|
||
| assertTrue(auth instanceof DjangoAuth); | ||
| } | ||
| } | ||
|
|
||
| @Test | ||
| public void noUserDetailsNoSessionToken_returnsNull() { | ||
| try (MockedStatic<RequestUtils> mocked = Mockito.mockStatic(RequestUtils.class)) { | ||
| mocked.when(RequestUtils::getUserDetails).thenReturn(Optional.empty()); | ||
|
|
||
| // SMS requests have neither a public session nor a sessionid cookie. | ||
| assertNull(aspect.getHqAuth(null)); | ||
| } | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,35 @@ | ||
| package org.commcare.formplayer.auth; | ||
|
|
||
| import static org.junit.jupiter.api.Assertions.assertEquals; | ||
| import static org.junit.jupiter.api.Assertions.assertFalse; | ||
| import static org.junit.jupiter.api.Assertions.assertThrows; | ||
|
|
||
| import org.junit.jupiter.api.Test; | ||
| import org.springframework.http.HttpHeaders; | ||
|
|
||
| public class PublicFormSessionAuthTest { | ||
|
|
||
| @Test | ||
| public void getAuthHeaders_emitsPublicCookieAndHeaderOnly() { | ||
| HttpHeaders headers = new PublicFormSessionAuth("session-key-123").getAuthHeaders(); | ||
|
|
||
| assertEquals("public_form_session_key=session-key-123", headers.getFirst("Cookie")); | ||
| assertEquals("true", headers.getFirst("CommCare-Public-Session")); | ||
|
|
||
| // Exactly the two public headers — no Django sessionid/Authorization leaks out. | ||
| assertEquals(2, headers.size()); | ||
| assertFalse(headers.containsKey("sessionid")); | ||
| assertFalse(headers.containsKey("Authorization")); | ||
| } | ||
|
|
||
| @Test | ||
| public void toString_doesNotLeakTheKey() { | ||
| assertFalse(new PublicFormSessionAuth("super-secret-key").toString().contains("super-secret-key")); | ||
| } | ||
|
|
||
| @Test | ||
| public void constructor_rejectsMissingKey() { | ||
| assertThrows(IllegalArgumentException.class, () -> new PublicFormSessionAuth(null)); | ||
| assertThrows(IllegalArgumentException.class, () -> new PublicFormSessionAuth("")); | ||
| } | ||
| } |
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.
Good check to have, but I'd move this before we instantiate HqAuth rather than after, unless there's a specific reason it needs to live here? Also, this throws an
IllegalArgumentException, which likely won't surface as an auth failure, did you confirm it's caught and mapped to the right error path so the user sees an appropriate error message rather than a generic exception?Uh oh!
There was an error while loading. Please reload this page.
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.
Good call, this seems like it was not the right spot. Moved to
loadUserDetailswhere we already throw similar errors with messages. ab59764