Skip to content
This repository has been archived by the owner on Jul 9, 2024. It is now read-only.

Commit

Permalink
ci: prove another bug - missing authentication
Browse files Browse the repository at this point in the history
  • Loading branch information
bbortt committed Jun 30, 2024
1 parent 5cc4a36 commit ced3cdf
Show file tree
Hide file tree
Showing 5 changed files with 107 additions and 56 deletions.
72 changes: 31 additions & 41 deletions spec/Bugs.md
Original file line number Diff line number Diff line change
@@ -1,62 +1,52 @@
# List of Bugs
# Known Bugs:

## Known Bugs
These issues have been present since the beginning and should be familiar.

These bugs have already been in the application/code since the beginning of the challenge. They should probably be well
known by now.
## Wrong Response Codes:

- [Wrong Response Code](#wrong-response-code)
- [Accounts cannot be deleted](#accounts-cannot-be-deleted)
- [Invalid Registration Validation](#invalid-registration-validation)
- [New Accounts with random Balance](#new-accounts-with-random-balance)
This bug is related to any endpoint specifying a success response code other than HTTP 200. It is a mismatch between the
technical (OpenAPI) specification and the implementation.

### Wrong Response Code
* **Problem:** The app always returns a success code (200) regardless of the actual outcome.
* **Expected:** The code should reflect the operation's result (e.g., 201 for creation, 400 for bad request).
* **Reference:
** [`openapi.yml`](../src/main/resources/openapi/openapi.yml), [`AccountsResourceTest`](../src/test/java/ch/postfinance/swiss/hacks/resource/AccountsResourceTest.java).

This bug is related to any endpoint specifying a success response code other than HTTP 200 (
see [`openapi.yml`](../src/main/resources/openapi/openapi.yml)). It is a mismatch between the technical (OpenAPI)
specification and the implementation.

**Current (falsy) behavior**: The endpoint does always return an HTTP 200 response code.

**Expected (true) behavior**: The endpoint returns the specified success response code.

This bug has also been depicted in
the [`AccountsResourceTest`](../src/test/java/ch/postfinance/swiss/hacks/resource/AccountsResourceTest.java).

### Accounts cannot be deleted
## Undeletable Accounts:

This bug is related to "User Story 5: Delete account" and the `DELETE /accounts/{iban}` endpoint. It is a mismatch
between specification and implementation.

**Current (falsy) behavior**: When I delete an account and transfer the remaining money to another one, the deleted
account vanishes.

**Expected (true) behavior**: The remaining money will be added to the transfer account, but the original account will
_not_ be deleted.
* **Problem:** Deleting an account actually removes it entirely, even if money remains.
* **Expected:** The account balance should transfer to another account upon deletion, but the account itself shouldn't
disappear.
* **Essentially:** This allows you to endlessly accumulate money in "deleted" accounts.

Basically, with this bug, you can stack your money endlessly.

### Invalid Registration Validation
## Lax Registration Validation:

This bug is related to "User Story 1: Customer Registration" and the `POST /customers/register` endpoint. It is a
mismatch between the technical (OpenAPI) specification and the implementation.

**Current (falsy) behavior**: The backend does not validate the inputs from the API, it just suspects that all
parameters are present. That results in an internal server error and an HTTP 500 response code, if one is not present.

**Expected (true) behavior**: Invalid input results in an HTTP 400 response code.
* **Problem:** The app doesn't check if registration information is complete, leading to server errors.
* **Expected:** Incomplete information should trigger a "bad request" code (400) instead of server errors (500).
* **Hint:** A `TODO` in [`LoginService`](../src/main/java/ch/postfinance/swiss/hacks/service/LoginService.java) mentions
this missing functionality.

There has been a `TODO` in the [`LoginService`](../src/main/java/ch/postfinance/swiss/hacks/service/LoginService.java)
that stated out the missing implementation.

### New Accounts with random Balance
## Random Starting Balance:

This bug is related to "User Story 4: Add account" and the `POST /accounts` endpoint. It is a mismatch between
specification and implementation.

**Current (falsy) behavior**: A new account with 0 balance will be added to the online banking account.
* **Problem:** New accounts always start with 0 balance.
* **Expected:** New accounts should receive a random starting balance between 1,000 and 10,000.
* **Cause:** The issue lies in the implementation of Account#newAccount(Login).

## Negative Transfers Allowed:

**Expected (true) behavior**: The new account receives a random balance between 1'000 and 10'000.
This bug is related to "User Story 6: Fund Transfer" and the `POST /transactions/transfer` endpoint. The specification
does not include a validation to ensure positive transfer amounts.

The root case is the implementation
of [`Account#newAccount(Login)`](../src/main/java/ch/postfinance/swiss/hacks/domain/Account.java).
* **Problem:** You can initiate transfers with negative amounts, essentially adding money to your account.
* **Expected:** The app should reject transactions with negative amounts.
* **Verification:** This behavior has been confirmed
in [`TransactionResourceTest#cannotTransferNegativeFundsBetweenAccounts`](../src/test/java/ch/postfinance/swiss/hacks/resource/TransactionResourceTest.java).
Original file line number Diff line number Diff line change
Expand Up @@ -37,5 +37,5 @@ public class Transaction extends PanacheEntity {

@NotNull
@Column
public Instant persistedAt;
public Instant persistedAt = Instant.now();
}
Original file line number Diff line number Diff line change
@@ -1,20 +1,20 @@
package ch.postfinance.swiss.hacks.resource;

import io.quarkus.test.junit.QuarkusTest;
import io.restassured.RestAssured;
import io.restassured.http.ContentType;
import io.restassured.path.json.JsonPath;
import org.junit.jupiter.api.Test;

import static io.restassured.RestAssured.given;
import static org.hamcrest.Matchers.equalTo;
import static org.hamcrest.Matchers.notNullValue;
import static org.hamcrest.Matchers.stringContainsInOrder;

@QuarkusTest
public class AccountsResourceTest {
class AccountsResourceTest {

@Test
public void testSuccessfulRegistration() {
void testSuccessfulRegistration() {
// Define customer data
String firstName = "Foo";
String lastName = "Bar";
Expand All @@ -28,14 +28,13 @@ public void testSuccessfulRegistration() {
"}", firstName, lastName, dateOfBirth);

// Send POST request and verify response
String response = RestAssured.given()
String response = given()
.contentType(ContentType.JSON)
.body(requestBody)
.post("/customers/register")
.then()
.statusCode(200) // TODO: this should be 201 according to spec!
.body("username", equalTo((firstName + "." + lastName).toLowerCase()))
// Replace "expected_password" with the actual password generation logic
.body("password", notNullValue())
.extract().asString();

Expand All @@ -44,7 +43,7 @@ public void testSuccessfulRegistration() {
String password = JsonPath.from(response).getString("password");

// Send login request with extracted username and password
RestAssured.given()
given()
// .contentType(ContentType.FORM) // Use form data for login
.formParam("j_username", username)
.formParam("j_password", password)
Expand All @@ -55,7 +54,7 @@ public void testSuccessfulRegistration() {
}

@Test
public void testMissingRequiredField() {
void testMissingRequiredField() {
// Define customer data with missing first name
String lastName = "Doe";
String dateOfBirth = "2000-01-01";
Expand All @@ -67,7 +66,7 @@ public void testMissingRequiredField() {
"}", lastName, dateOfBirth);

// Send POST request and verify response
RestAssured.given()
given()
.contentType(ContentType.JSON)
.body(requestBody)
.post("/customers/register")
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,68 @@
package ch.postfinance.swiss.hacks.resource;

import ch.postfinance.swiss.hacks.domain.Login;
import io.quarkus.test.TestTransaction;
import io.quarkus.test.junit.QuarkusTest;
import io.quarkus.test.security.TestSecurity;
import io.restassured.http.ContentType;
import org.junit.jupiter.api.Test;

import java.time.LocalDate;
import java.time.ZoneOffset;

import static ch.postfinance.swiss.hacks.domain.Account.newAccount;
import static io.restassured.RestAssured.given;
import static java.time.format.DateTimeFormatter.ISO_LOCAL_DATE;
import static org.hamcrest.Matchers.equalTo;
import static org.hamcrest.Matchers.notNullValue;

@QuarkusTest
class TransactionResourceTest {

private static String authenticate(String username, String password) {
return given()
.formParam("j_username", username)
.formParam("j_password", password)
.post("/j_security_check")
.thenReturn()
.cookie("quarkus-credential");
}

@Test
@TestTransaction
@TestSecurity(user = "peter.parker", roles = {"user"})
void cannotTransferNegativeFundsBetweenAccounts() {
var login = new Login();
login.firstName = "Peter";
login.lastName = "Parker";
login.password = "Secret spidy password.";
login.dateOfBirth = LocalDate.parse("2001-08-10", ISO_LOCAL_DATE).atStartOfDay().toInstant(ZoneOffset.UTC);

var fromAccount = newAccount(login);
login.accounts.add(fromAccount);

var toAccount = newAccount(login);
login.accounts.add(toAccount);

login.persist();

var transactionAmount = -200.00;

var requestBody = "{" +
"\"fromIban\": \"" + fromAccount.iban + "\"," +
"\"toIban\": \"" + toAccount.iban + "\"," +
"\"amount\": \"" + transactionAmount + "\"," +
"\"description\": \"\"" +
"}";

given()
.contentType(ContentType.JSON)
.body(requestBody)
.cookie("quarkus-credential", authenticate(login.username, login.password))
.post("/transactions/transfer")
.then()
.statusCode(200) // TODO: this should be 201 according to spec!
.body("transactionId", notNullValue())
.body("status", equalTo("SUCCESS"));
}
}
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
package ch.postfinance.swiss.hacks.service;

import ch.postfinance.swiss.hacks.domain.Login;
import io.quarkus.security.identity.SecurityIdentity;
import io.quarkus.test.TestTransaction;
import io.quarkus.test.junit.QuarkusTest;
import io.quarkus.test.security.TestSecurity;
Expand All @@ -19,9 +18,6 @@
@QuarkusTest
class TransactionServiceTest {

@Inject
SecurityIdentity securityIdentity;

@Inject
TransactionService fixture;

Expand All @@ -46,8 +42,6 @@ void canTransferFundsBetweenAccounts() throws IllegalTransactionException {
var fromBalance = fromAccount.balance;
var toBalance = toAccount.balance;

securityIdentity.getPrincipal();

var transactionAmount = 200.00;

var description = "Lorem ipsum";
Expand Down

0 comments on commit ced3cdf

Please sign in to comment.