Skip to content

Commit

Permalink
Integration test to ensure that transactions cannot be hijacked
Browse files Browse the repository at this point in the history 
- Include some refactoring of SessionFactory
- Remove the potentially dangerous method getTransaction(final String txId) that would allow unauthorized access to transactions

Fixes https://www.pivotaltracker.com/story/show/66014686
  • Loading branch information
@kaimst
kaimst authored and Andrew Woods committed May 6, 2014
1 parent cef5926 commit 48d02cc
Show file tree
Hide file tree
Showing 12 changed files with 369 additions and 250 deletions.
7 changes: 3 additions & 4 deletions fcrepo-http-api/src/main/java/org/fcrepo/http/api/FedoraTransactions.java
View file
Expand Up @@ -83,18 +83,17 @@ public Response createTransaction(@PathParam("path")
}

if (session instanceof TxSession) {
final Transaction t =
txService.getTransaction(((TxSession) session)
.getTxId());
final Transaction t = txService.getTransaction(session);
t.updateExpiryDate();
return noContent().expires(t.getExpires()).build();

}

final Principal userPrincipal = req.getUserPrincipal();
String userName = null;
if (userPrincipal != null) {
userName = userPrincipal.getName();
}

final Transaction t = txService.beginTransaction(session, userName);

return created(
Expand Down
4 changes: 3 additions & 1 deletion fcrepo-http-api/src/test/java/org/fcrepo/http/api/FedoraTransactionsTest.java
View file
Expand Up @@ -31,10 +31,12 @@

import org.fcrepo.kernel.Transaction;
import org.fcrepo.kernel.TxAwareSession;
import org.fcrepo.kernel.TxSession;
import org.fcrepo.kernel.services.TransactionService;
import org.junit.Before;
import org.junit.Test;
import org.mockito.Mock;
import org.mockito.Mockito;

import java.security.Principal;

Expand Down Expand Up @@ -86,7 +88,7 @@ public void shouldStartANewTransaction() throws RepositoryException {
public void
shouldUpdateExpiryOnExistingTransaction()
throws RepositoryException {
when(mockTxService.getTransaction("123")).thenReturn(mockTx);
when(mockTxService.getTransaction(Mockito.any(TxSession.class))).thenReturn(mockTx);
testObj.createTransaction(createPathList(), mockRequest);
verify(mockTx).updateExpiryDate();
}
Expand Down
47 changes: 44 additions & 3 deletions fcrepo-http-api/src/test/java/org/fcrepo/integration/http/api/AbstractResourceIT.java
View file
Expand Up @@ -30,16 +30,30 @@
import java.util.UUID;

import com.hp.hpl.jena.update.GraphStore;
import org.apache.http.HttpHost;
import org.apache.http.HttpResponse;
import org.apache.http.auth.AuthScope;
import org.apache.http.auth.UsernamePasswordCredentials;
import org.apache.http.client.AuthCache;
import org.apache.http.client.ClientProtocolException;
import org.apache.http.client.CredentialsProvider;
import org.apache.http.client.HttpClient;
import org.apache.http.client.methods.HttpPatch;
import org.apache.http.client.methods.CloseableHttpResponse;
import org.apache.http.client.methods.HttpUriRequest;
import org.apache.http.client.methods.HttpPost;
import org.apache.http.client.methods.HttpPut;
import org.apache.http.client.methods.HttpUriRequest;
import org.apache.http.client.methods.HttpPatch;
import org.apache.http.client.protocol.HttpClientContext;
import org.apache.http.entity.BasicHttpEntity;
import org.apache.http.entity.StringEntity;
import org.apache.http.impl.auth.BasicScheme;

import org.apache.http.impl.client.BasicAuthCache;
import org.apache.http.impl.client.HttpClientBuilder;
import org.apache.http.impl.client.BasicCredentialsProvider;
import org.apache.http.impl.client.CloseableHttpClient;
import org.apache.http.impl.client.HttpClients;

import org.apache.http.util.EntityUtils;
import org.junit.Before;
import org.junit.runner.RunWith;
Expand All @@ -63,7 +77,9 @@ public void setLogger() {

protected static final String HOSTNAME = "localhost";

protected static final String serverAddress = "http://" + HOSTNAME + ":" +
protected static final String PROTOCOL = "http";

protected static final String serverAddress = PROTOCOL + "://" + HOSTNAME + ":" +
SERVER_PORT + "/";

protected static HttpClient client = createClient();
Expand Down Expand Up @@ -114,6 +130,31 @@ protected HttpResponse execute(final HttpUriRequest method)
return client.execute(method);
}

// Executes requests with preemptive basic authentication
protected HttpResponse executeWithBasicAuth(final HttpUriRequest request,
final String username,
final String password)
throws IOException {
final HttpHost target = new HttpHost(HOSTNAME, SERVER_PORT, PROTOCOL);
final CredentialsProvider credsProvider = new BasicCredentialsProvider();
credsProvider.setCredentials(
new AuthScope(target.getHostName(), target.getPort()),
new UsernamePasswordCredentials(username, password));
final CloseableHttpClient httpclient = HttpClients.custom()
.setDefaultCredentialsProvider(credsProvider).build();

final AuthCache authCache = new BasicAuthCache();
final BasicScheme basicAuth = new BasicScheme();
authCache.put(target, basicAuth);

final HttpClientContext localContext = HttpClientContext.create();
localContext.setAuthCache(authCache);

final CloseableHttpResponse response = httpclient.execute(request, localContext);
return response;
}


protected int getStatus(final HttpUriRequest method)
throws ClientProtocolException, IOException {
final HttpResponse response = execute(method);
Expand Down
122 changes: 76 additions & 46 deletions fcrepo-http-api/src/test/java/org/fcrepo/integration/http/api/FedoraTransactionsIT.java
View file
Expand Up @@ -27,6 +27,7 @@
import org.apache.http.client.methods.HttpGet;
import org.apache.http.client.methods.HttpPatch;
import org.apache.http.client.methods.HttpPost;
import org.apache.http.client.methods.HttpPut;
import org.apache.http.entity.BasicHttpEntity;
import org.junit.Ignore;
import org.junit.Test;
Expand Down Expand Up @@ -57,12 +58,7 @@ public class FedoraTransactionsIT extends AbstractResourceIT {

@Test
public void testCreateTransaction() throws Exception {
/* create a tx */
final HttpPost createTx = new HttpPost(serverAddress + "fcr:tx");
final HttpResponse response = execute(createTx);
assertEquals(201, response.getStatusLine().getStatusCode());

final String location = response.getFirstHeader("Location").getValue();
final String location = createTransaction();

logger.info("Got location {}", location);
assertTrue(
Expand All @@ -89,11 +85,7 @@ public void testCreateAndTimeoutTransaction() throws Exception {
System.setProperty(TIMEOUT_SYSTEM_PROPERTY, Long.toString(testTimeout));

/* create a tx */
final HttpPost createTx = new HttpPost(serverAddress + "fcr:tx");
final HttpResponse response = execute(createTx);
assertEquals(201, response.getStatusLine().getStatusCode());

final String location = response.getFirstHeader("Location").getValue();
final String location = createTransaction();

final HttpGet getWithinTx = new HttpGet(location);
HttpResponse resp = execute(getWithinTx);
Expand Down Expand Up @@ -171,17 +163,10 @@ public void testCreateDoStuffAndRollbackTransaction() throws Exception {
@Test
public void testCreateDoStuffAndCommitTransaction() throws Exception {
/* create a tx */
final String objectInTxCommit = randomUUID().toString();

final HttpPost createTx = new HttpPost(serverAddress + "fcr:tx");

final HttpResponse response = execute(createTx);
assertEquals(201, response.getStatusLine().getStatusCode());

final String txLocation =
response.getFirstHeader("Location").getValue();
final String txLocation = createTransaction();

/* create a new object inside the tx */
final String objectInTxCommit = randomUUID().toString();
final HttpPost postNew =
new HttpPost(txLocation);
postNew.addHeader("Slug", objectInTxCommit);
Expand Down Expand Up @@ -226,17 +211,10 @@ public void testCreateDoStuffAndCommitTransaction() throws Exception {
@Test
public void testCreateDoStuffAndCommitTransactionSeparateConnections() throws Exception {
/* create a tx */
final String objectInTxCommit = randomUUID().toString();

final HttpPost createTx = new HttpPost(serverAddress + "fcr:tx");

final HttpResponse response = execute(createTx);
assertEquals(201, response.getStatusLine().getStatusCode());

final String txLocation =
response.getFirstHeader("Location").getValue();
final String txLocation = createTransaction();

/* create a new object inside the tx */
final String objectInTxCommit = randomUUID().toString();
client = createClient();
final HttpPost postNew =
new HttpPost(txLocation);
Expand Down Expand Up @@ -293,13 +271,7 @@ public void testIngestNewWithSparqlPatchWithinTransaction() throws Exception {
final String objectInTxCommit = randomUUID().toString();

/* create new tx */
final HttpPost createTx = new HttpPost(serverAddress + "fcr:tx");
final HttpResponse response = execute(createTx);
assertEquals(201, response.getStatusLine().getStatusCode());

/* create a new object inside the tx */
final String txLocation =
response.getFirstHeader("Location").getValue();
final String txLocation = createTransaction();

client = createClient();
final HttpPost postNew =
Expand Down Expand Up @@ -354,22 +326,80 @@ public void testIngestNewWithSparqlPatchWithinTransaction() throws Exception {

@Test
public void testGetNonExistingObject() throws Exception {
/* create new tx */
final HttpPost createTx = new HttpPost(serverAddress + "fcr:tx");
final HttpResponse response = execute(createTx);
assertEquals(201, response.getStatusLine().getStatusCode());

/* try to retrieve a non existing object inside the tx */
final String txLocation =
response.getFirstHeader("Location").getValue();
final String txLocation = createTransaction();
final String newObjectLocation = txLocation + "/idontexist";
final HttpGet httpGet = new HttpGet(newObjectLocation);

client = createClient();
HttpResponse responseFromGet = client.execute(httpGet);
int status = responseFromGet.getStatusLine().getStatusCode();
final HttpResponse responseFromGet = client.execute(httpGet);
final int status = responseFromGet.getStatusLine().getStatusCode();
assertEquals("Status should be 404", 404, status);
}

/**
* Tests that transactions cannot be hijacked
*/
@Test
public void testTransactionHijackingNotPossible() throws Exception {

/* "fedoraAdmin" creates a transaction */
final HttpPost createTx = new HttpPost(serverAddress + "fcr:tx");
final HttpResponse response = executeWithBasicAuth(createTx, "fedoraAdmin", "fedoraAdmin");
assertEquals("Status should be 201 after creating a transaction with user fedoraAdmin",
201, response.getStatusLine().getStatusCode());
final String txLocation = response.getFirstHeader("Location").getValue();

/* "fedoraUser" puts to "fedoraAdmin"'s transaction and fails */
final HttpPut putFedoraUser = new HttpPut(txLocation);
final HttpResponse responseFedoraUser = executeWithBasicAuth(putFedoraUser, "fedoraUser", "fedoraUser");
assertEquals("Status should be 410 because putting on a transaction of a different user is not allowed",
410, responseFedoraUser.getStatusLine().getStatusCode());

/* anonymous user puts to "fedoraAdmin"'s transaction and fails */
final HttpPut putTxAnon = new HttpPut(txLocation);
final HttpResponse responseTxAnon = execute(putTxAnon);
assertEquals("Status should be 410 because putting on a transaction of a different user is not allowed",
410, responseTxAnon.getStatusLine().getStatusCode());

/* transaction is still intact and "fedoraAdmin" - the owner - can successfully put to it */
final String objectInTxCommit = randomUUID().toString();
final HttpPut putToExistingTx = new HttpPut(txLocation + "/" + objectInTxCommit);
final HttpResponse responseFromPutToTx = executeWithBasicAuth(putToExistingTx, "fedoraAdmin", "fedoraAdmin");
assertEquals("Status should be 201 after putting", 201, responseFromPutToTx.getStatusLine().getStatusCode());

}

/**
* Tests that transactions cannot be hijacked,
* even if created by an anonymous user
*/
@Test
public void testTransactionHijackingNotPossibleAnoymous() throws Exception {

/* anonymous user creates a transaction */
final String txLocation = createTransaction();

/* fedoraAdmin attempts to puts to anonymous transaction and fails */
final HttpPut putFedoraAdmin = new HttpPut(txLocation);
final HttpResponse responseFedoraAdmin = executeWithBasicAuth(putFedoraAdmin, "fedoraAdmin", "fedoraAdmin");
assertEquals("Status should be 410 because putting on a transaction of a different user is not permitted",
410, responseFedoraAdmin.getStatusLine().getStatusCode());

/* fedoraUser attempts to put to anonymous transaction and fails */
final HttpPut putFedoraUser = new HttpPut(txLocation);
final HttpResponse responseFedoraUser = executeWithBasicAuth(putFedoraUser, "fedoraUser", "fedoraUser");
assertEquals("Status should be 410 because putting on a transaction of a different user is not permitted",
410, responseFedoraUser.getStatusLine().getStatusCode());

/* transaction is still intact and any anonymous user can successfully put to it */
final String objectInTxCommit = randomUUID().toString();
final HttpPut putToExistingTx = new HttpPut(txLocation + "/" + objectInTxCommit);
final HttpResponse responseFromPutToTx = execute(putToExistingTx);
assertEquals("Status should be 201 after putting", 201, responseFromPutToTx.getStatusLine().getStatusCode());

}



/**
* Tests that transactions are treated as atomic with regards to nodes.
Expand Down

0 comments on commit 48d02cc

Please sign in to comment.