Czy moje testy bezpieczeństwa dla endpointu delete user są napisane poprawnie?

0

Cześć,
moja konfiguracja spring security jest skoncentrowana na zabezpieczaniu endpointów. Podział ról USER/ADMIN. Napisałem takie testy, jestem ciekaw co o nich myślicie.

@SpringBootTest(webEnvironment = SpringBootTest.WebEnvironment.RANDOM_PORT)
@AutoConfigureMockMvc
@ActiveProfiles("test")
public class DeleteUserControllerSecurityTest {
    @Autowired
    private MockMvc mockMvc;
    @Autowired
    private DataSource dataSource;
    private Long userId;

    @BeforeEach
    void init() throws Exception {
        try(Connection connection = dataSource.getConnection()) {
            ScriptUtils.executeSqlScript(connection, new ClassPathResource("data_sql/test-data-setup.sql"));

            final String usernameToDelete = "testSecondUsername";
            this.userId = DatabaseTestHelper.getUserIdByUsername(dataSource, usernameToDelete);
        }
    }
    @AfterEach
    void cleanup() {
        JdbcTemplate jdbcTemplate = new JdbcTemplate(dataSource);
        jdbcTemplate.update("DELETE FROM user_roles");
        jdbcTemplate.update("DELETE FROM users");
        jdbcTemplate.update("DELETE FROM roles");
    }
    @Test
    void shouldAllowDeletionForUserWithProperRole() throws Exception {
        final String requestBody = "{\"usernameOrEmail\": \"testAdmin\",\"password\": \"testPassword@123\"}";

        String jwtToken = AuthenticationTestHelper.loginAndGetToken(mockMvc, requestBody);
        Cookie cookie = new Cookie("auth_token", jwtToken);

        DeleteRequestAssertions.assertDeleteStatusReturns(
                mockMvc, "/api/v1/users/" + userId, cookie, 200);
    }
    @Test
    @WithAnonymousUser
    void shouldReturnUnauthorizedForUnauthenticatedUser() throws Exception {
        DeleteRequestAssertions.assertDeleteStatusReturns(
                mockMvc, "/api/v1/users/" + userId, null, 401);
    }
    @Test
    @WithAnonymousUser
    void shouldReturnUnauthorizedMessageForUnauthenticatedUser() throws Exception {
        final String expectedMessage = "Unauthorized access. Please log in.";

        DeleteRequestAssertions.assertDeleteResponse(mockMvc, "/api/v1/users/" + userId,
                null, expectedMessage);
    }
    @Test
    void shouldReturnStatusForbiddenWhenUserWithInsufficientPrivilegesTriesToDeleteUser() throws Exception {
        final String requestBody = "{\"usernameOrEmail\": \"testUsername\",\"password\": \"testPassword@123\"}";

        String jwtToken = AuthenticationTestHelper.loginAndGetToken(mockMvc, requestBody);
        Cookie cookie = new Cookie("auth_token", jwtToken);

        DeleteRequestAssertions.assertDeleteStatusReturns(
                mockMvc, "/api/v1/users/" + userId, cookie, 403);
    }
    @Test
    void shouldReturnForbiddenMessageWhenUserWithInsufficientPrivilegesTriesToDeleteUser() throws Exception {
        final String requestBody = "{\"usernameOrEmail\": \"testUsername\",\"password\": \"testPassword@123\"}";

        final String expectedMessage = "You do not have permission to perform this operation.";

        String jwtToken = AuthenticationTestHelper.loginAndGetToken(mockMvc, requestBody);
        Cookie cookie = new Cookie("auth_token", jwtToken);

        DeleteRequestAssertions.assertDeleteResponse(mockMvc, "/api/v1/users/" + userId,
                cookie, expectedMessage);
    }
}

Klasy pomocnicze

public class DeleteRequestAssertions {
    public static void assertDeleteStatusReturns(MockMvc mockMvc, String endpoint, Cookie cookie,
                                                 int expectedStatusCode) throws Exception {
        MockHttpServletRequestBuilder requestBuilder = delete(endpoint);

        if (cookie != null) {
            requestBuilder.cookie(cookie);
        }
        mockMvc.perform(requestBuilder)
                .andExpect(status().is(expectedStatusCode));
    }
    public static void assertDeleteResponse(MockMvc mockMvc, String endpoint, Cookie jwtCookie,
                                            String expectedMessage) throws Exception {
        MockHttpServletRequestBuilder requestBuilder = delete(endpoint);
        if (jwtCookie != null) {
            requestBuilder.cookie(jwtCookie);
        }
        mockMvc.perform(requestBuilder)
                .andExpect(content().string(containsString(expectedMessage)));
    }
}

public class DatabaseTestHelper {
    public static Long getUserIdByUsername(DataSource dataSource, String username) throws SQLException {
        String query = "SELECT id FROM users WHERE username = ?";
        try (Connection connection = dataSource.getConnection();
             PreparedStatement statement = connection.prepareStatement(query)) {

            statement.setString(1, username);

            ResultSet resultSet = statement.executeQuery();

            if (resultSet.next()) {
                return resultSet.getLong("id");
            } else {
                return null;
            }
        }
    }
}

public class AuthenticationTestHelper {
    public static String loginAndGetToken(MockMvc mockMvc, String requestBody) throws Exception {
        MvcResult mvcResult = mockMvc.perform(post("/api/v1/users/login")
                        .contentType(MediaType.APPLICATION_JSON)
                        .content(requestBody))
                .andExpect(status().isOk())
                .andReturn();

        String responseString = mvcResult.getResponse().getContentAsString();
        return JsonPath.parse(responseString).read("$.accessToken");
    }
}


Dzięki za pomoc.

1

Krótko wzrocznie:

  1. Nazwa shouldAllowDeletionForUserWithProperRole() jest niepotrzebnie ogólna. Czemu "proper role" zamiast admin? shouldAdminDelete albo shouldDeleteWithPermission. Nazwy tych testów są też za długie.
  2. Mi się zdaje, czy te testy nie sprawdzają w ogóle czy zasób został/nie został usunięty? Sprawdzają tylko odpowiedź. Powinieneś dopisać testy które sprawdzają czy faktycznie zasób istnieje lub nie istnieje.
  3. Testy mają za dużo szczegółów związanych z logowaniem. Np test shouldReturnStatusForbiddenWhenUserWithInsufficientPrivilegesTriesToDeleteUser() powinien wyglądać:
    @Test
    void shouldReturnStatusForbiddenWhenUserWithInsufficientPrivilegesTriesToDeleteUser() throws Exception {
       Cookie cookie = JakiśHelper.regularUser();
    
       DeleteRequestAssertions.assertDeleteStatusReturns(
                 mockMvc, deleteUri(userId), cookie, 403);
     }
    
    Nie potrzebujesz w teście podawać konkretnego loginu i hasła, oraz konkretnego sposobu logowania. Wystarczy informacja czy user ma czy nie ma prawa do usuwania - np regularUser(), adminUser(). Abstrakcja (albo poziomy abstrakcji).
  4. Testy sprawdzające status kod oraz message chyba bym połączył w jedno. Nie ma powodu żeby to były osobne testy IMO. Testy statusu i tego czy są usunięte zasoby powinny być osobno. Ale testy statusu i message mogą być razem.

Daleko wzrocznie:

  1. Testy bardzo zależą od Springa.
  2. Testy zależą od bazy
  3. Testy mają dużo szczegółów implementacyjnych
0

Nitpick, ale porównaj:
shouldReturnStatusForbiddenWhenUserWithInsufficientPrivilegesTriesToDeleteUser
should_return_status_forbidden_when_user_with_insufficient_privileges_tries_to_delete_user
Które się łatwiej czyta?

0
Riddle napisał(a):

Nazwa shouldAllowDeletionForUserWithProperRole() jest niepotrzebnie ogólna. Czemu "proper role" zamiast admin? shouldAdminDelete albo shouldDeleteWithPermission. Nazwy tych testów są też za długie.

Staram się tak te nazwy pisać, żeby każda osoba po nazwie testu wiedziała o co w nim chodzi. Będę się starał pisać krótsze.

Riddle napisał(a):
  1. Mi się zdaje, czy te testy nie sprawdzają w ogóle czy zasób został/nie został usunięty? Sprawdzają tylko odpowiedź. Powinieneś dopisać testy które sprawdzają czy faktycznie zasób istnieje lub nie istnieje.

Te testy sprawdzają tylko odpowiedź. Będę musiał dopisać te sprawdzające czy zasób istnieje.

Riddle napisał(a):
  1. Testy mają za dużo szczegółów związanych z logowaniem. Np test shouldReturnStatusForbiddenWhenUserWithInsufficientPrivilegesTriesToDeleteUser() powinien wyglądać:
    @Test
    void shouldReturnStatusForbiddenWhenUserWithInsufficientPrivilegesTriesToDeleteUser() throws Exception {
       Cookie cookie = JakiśHelper.regularUser();
    
       DeleteRequestAssertions.assertDeleteStatusReturns(
                 mockMvc, deleteUri(userId), cookie, 403);
     }
    
    Nie potrzebujesz w teście podawać konkretnego loginu i hasła, oraz konkretnego sposobu logowania. Wystarczy informacja czy user ma czy nie ma prawa do usuwania - np regularUser(), adminUser().

Tutaj fajnie mnie nakierowałeś. Nie pomyślałem żeby tak zrobić.

Riddle napisał(a):
  1. Testy sprawdzające status kod oraz message chyba bym połączył w jedno. Nie ma powodu żeby to były osobne testy IMO. Testy statusu i tego czy są usunięte zasoby powinny być osobno. Ale testy statusu i message mogą być razem.

Tak zrobię.

Riddle napisał(a):

Daleko wzrocznie:

  1. Testy bardzo zależą od Springa.
  2. Testy zależą od bazy
  3. Testy mają dużo szczegółów implementacyjnych

Czyli proponujesz bardziej je wyizolować ?

0
Ornstein napisał(a):
Riddle napisał(a):

Nazwa shouldAllowDeletionForUserWithProperRole() jest niepotrzebnie ogólna. Czemu "proper role" zamiast admin? shouldAdminDelete albo shouldDeleteWithPermission. Nazwy tych testów są też za długie.

Staram się tak te nazwy pisać, żeby każda osoba po nazwie testu wiedziała o co w nim chodzi. Będę się starał pisać krótsze.

Zasadność bardzo dobra. Tylko czasem więcej znaczy mniej.

Nazwy powinny być opisowe i dokładne, ale powinny być też krótkie i proste. Staraj się usunąć niepotrzebne słowa i lepiej dobierać określenia.

Jak mówi pożekadło, there are only two hard things in software.

Riddle napisał(a):
  1. Mi się zdaje, czy te testy nie sprawdzają w ogóle czy zasób został/nie został usunięty? Sprawdzają tylko odpowiedź. Powinieneś dopisać testy które sprawdzają czy faktycznie zasób istnieje lub nie istnieje.

Te testy sprawdzają tylko odpowiedź. Będę musiał dopisać te sprawdzające czy zasób istnieje.

No stanowczo.

Nie potrzebujesz w teście podawać konkretnego loginu i hasła, oraz konkretnego sposobu logowania. Wystarczy informacja czy user ma czy nie ma prawa do usuwania - np regularUser(), adminUser().

Tutaj fajnie mnie nakierowałeś. Nie pomyślałem żeby tak zrobić.

Cieszę się że mogłem pomóc.

Riddle napisał(a):

Daleko wzrocznie:

  1. Testy bardzo zależą od Springa.
  2. Testy zależą od bazy
  3. Testy mają dużo szczegółów implementacyjnych

Czyli proponujesz bardziej je wyizolować ?

To nie jest łatwe, musiałbyś wiedzieć co robisz.

Jak to zrobisz "na czuja", to możliwe że sobie zaszkodzisz.

1 użytkowników online, w tym zalogowanych: 0, gości: 1