Skip to content
This repository has been archived by the owner on Nov 1, 2023. It is now read-only.

SDIT-1113: 🔒️ Secure automated refresh endpoint and test #511

Merged
merged 1 commit into from
Oct 6, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions helm_deploy/prisoner-offender-search/values.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,10 @@ generic-service:
deny all;
return 401;
}
location /prisoner-index/automated-reconcile {
deny all;
return 401;
}
tlsSecretName: prisoner-offender-search-cert

resources:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,15 +8,15 @@ import java.time.LocalDate

class PossibleMatchesSearchResourceTest : AbstractSearchDataIntegrationTest() {
@Test
fun `search for possible matches access forbidden when no authority`() {
fun `search for possible matches access unauthorised when no authority`() {
webTestClient.post().uri("/prisoner-search/possible-matches")
.header("Content-Type", "application/json")
.exchange()
.expectStatus().isUnauthorized
}

@Test
fun `search for possible matches access forbidden when no role`() {
fun `search for possible matches access forbidden for endpoint POST #prisoner-search#possible-matches when no role`() {
webTestClient.post().uri("/prisoner-search/possible-matches")
.body(BodyInserters.fromValue(gson.toJson(PossibleMatchCriteria(null, null, null, null, "A1234AB"))))
.headers(setAuthorisation())
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,15 +8,15 @@ import java.time.LocalDate

class PrisonerMatchResourceTest : AbstractSearchDataIntegrationTest() {
@Test
fun `access forbidden when no authority`() {
fun `access unauthorised when no authority`() {
webTestClient.post().uri("/match-prisoners")
.header("Content-Type", "application/json")
.exchange()
.expectStatus().isUnauthorized
}

@Test
fun `access forbidden when no role`() {
fun `access forbidden for endpoint POST #prisoner-search#match-prisoners when no role`() {
webTestClient.post().uri("/match-prisoners")
.body(BodyInserters.fromValue(gson.toJson(MatchRequest("john", "smith", null, null, null, "A7089EY"))))
.headers(setAuthorisation())
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -134,7 +134,7 @@ class PrisonerSearchByBookingIdsResourceTest : QueueIntegrationTest() {
}

@Test
fun `access forbidden for ids search when no role`() {
fun `access forbidden for endpoint POST #prisoner-search#booking-ids when no role`() {
webTestClient.post().uri("/prisoner-search/booking-ids")
.body(BodyInserters.fromValue(gson.toJson(BookingIds(listOf(1)))))
.headers(setAuthorisation())
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,7 @@ class PrisonerSearchByPrisonerNumbersResourceTest : QueueIntegrationTest() {
}

@Test
fun `access forbidden for prison number search when no role`() {
fun `access forbidden for endpoint POST #prisoner-search#prisoner-numbers when no role`() {
webTestClient.post().uri("/prisoner-search/prisoner-numbers")
.body(BodyInserters.fromValue(gson.toJson(PrisonerNumbers(arrayListOf("ABC")))))
.headers(setAuthorisation())
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,15 +8,15 @@ import java.time.LocalDate

class PrisonerSearchByReleaseDateResourceTest : AbstractSearchDataIntegrationTest() {
@Test
fun `access forbidden when no authority`() {
fun `access unauthorised when no authority`() {
webTestClient.post().uri("/prisoner-search/release-date-by-prison")
.header("Content-Type", "application/json")
.exchange()
.expectStatus().isUnauthorized
}

@Test
fun `access forbidden when no role`() {
fun `access forbidden for endpoint POST #prisoner-search#release-date-by-prison when no role`() {
webTestClient.post().uri("/prisoner-search/release-date-by-prison")
.body(BodyInserters.fromValue(gson.toJson(ReleaseDateSearch(latestReleaseDate = LocalDate.now()))))
.headers(setAuthorisation())
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,15 +7,15 @@ import uk.gov.justice.digital.hmpps.prisonersearch.services.PrisonSearch

class PrisonerSearchBySinglePrisonResourceTest : AbstractSearchDataIntegrationTest() {
@Test
fun `access forbidden when no authority`() {
fun `access unauthorised when no authority`() {
webTestClient.post().uri("/prisoner-search/match")
.header("Content-Type", "application/json")
.exchange()
.expectStatus().isUnauthorized
}

@Test
fun `access forbidden when no role`() {
fun `access forbidden for endpoint POST #prisoner-search#match when no role`() {
webTestClient.post().uri("/prisoner-search/match")
.body(BodyInserters.fromValue(gson.toJson(PrisonSearch("A7089EY", "john", "smith", "MDI"))))
.headers(setAuthorisation())
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,15 +35,15 @@ class PrisonerSearchResourceTest : AbstractSearchDataIntegrationTest() {
}

@Test
fun `search by prisonId access forbidden when no authority`() {
fun `search by prisonId access unauthorised when no authority`() {
webTestClient.get().uri("/prisoner-search/prison/MDI")
.header("Content-Type", "application/json")
.exchange()
.expectStatus().isUnauthorized
}

@Test
fun `search by prisonId access forbidden when no role`() {
fun `search by prisonId access forbidden for endpoint GET #prisoner-search#prison#{prisonId} when no role`() {
webTestClient.get().uri("/prisoner-search/prison/MDI")
.headers(setAuthorisation())
.header("Content-Type", "application/json")
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,82 @@
package uk.gov.justice.digital.hmpps.prisonersearch.resource

import org.assertj.core.api.Assertions.assertThat
import org.junit.jupiter.api.Test
import org.springframework.beans.factory.annotation.Autowired
import org.springframework.context.ApplicationContext
import org.springframework.security.access.prepost.PreAuthorize
import org.springframework.web.servlet.mvc.method.RequestMappingInfo
import org.springframework.web.servlet.mvc.method.annotation.RequestMappingHandlerMapping
import uk.gov.justice.digital.hmpps.prisonersearch.integration.IntegrationTest
import java.io.File
import kotlin.reflect.full.memberFunctions

class ResourceSecurityTest : IntegrationTest() {
@Autowired
private lateinit var context: ApplicationContext

private val unprotectedDefaultMethods = setOf(
"GET /v3/api-docs.yaml",
"GET /swagger-ui.html",
"GET /v3/api-docs",
"GET /v3/api-docs/swagger-config",
" /error",
)
private val testClasses = setOf(
PrisonerSearchByBookingIdsResourceTest::class,
PrisonerSearchByReleaseDateResourceTest::class,
PossibleMatchesSearchResourceTest::class,
PrisonerSearchBySinglePrisonResourceTest::class,
PrisonerMatchResourceTest::class,
RestrictedPatientsSearchResourceTest::class,
PrisonerSearchByPrisonerNumbersResourceTest::class,
PrisonerSearchResourceTest::class,
)

@Test
fun `Ensure all endpoints protected with PreAuthorize`() {
// need to exclude any that are forbidden in helm configuration
val exclusions = File("helm_deploy").walk().filter { it.name.equals("values.yaml") }.flatMap { file ->
file.readLines().map { line ->
line.takeIf { it.contains("location") }?.substringAfter("location ")?.substringBefore(" {")
}
}.filterNotNull().flatMap { path -> listOf("GET", "POST", "PUT", "DELETE").map { "$it $path" } }
.toMutableSet().also {
it.addAll(unprotectedDefaultMethods)
}

testClasses.flatMap { clazz ->
clazz.nestedClasses.toMutableList().apply { add(clazz) }.flatMap { testClazz ->
testClazz.memberFunctions
.filter { it.name.contains("access forbidden") }
.map {
it.name
.substringAfter("for endpoint ")
.substringBefore(" when no role")
.replace("#", "/")
}
}
Comment on lines +50 to +58
Copy link
Contributor

@mikehalmamoj mikehalmamoj Oct 6, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not keen on this - for all I know the test has the right name but tests something entirely different. Not sure of a better way though 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed. Ideally though wouldn't be needed at all since the resources will have the annotations. However didn't want to start changing this project since we're in the process of ripping it all out anyway.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would still be relevant for Prison API though

}.also { exclusions.addAll(it) }

context.getBeansOfType(RequestMappingHandlerMapping::class.java).forEach { (_, mapping) ->
mapping.handlerMethods.forEach { (mappingInfo, method) ->
val classAnnotation = method.beanType.getAnnotation(PreAuthorize::class.java)
val annotation = method.getMethodAnnotation(PreAuthorize::class.java)
println("Found $mappingInfo with class annotation $classAnnotation and annotation $annotation")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

was this line a temporary data dump?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes it was, will remove in next PR

if (classAnnotation == null && annotation == null) {
mappingInfo.getMappings().forEach {
assertThat(exclusions.contains(it)).withFailMessage {
"Found $mappingInfo of type $method with no PreAuthorize annotation"
}.isTrue()
}
}
}
}
}
}

private fun RequestMappingInfo.getMappings() =
methodsCondition.methods.map { it.name }.flatMap {
method ->
pathPatternsCondition.patternValues.map { "$method $it" }
}
Original file line number Diff line number Diff line change
Expand Up @@ -16,15 +16,15 @@ class RestrictedPatientsSearchResourceTest : AbstractSearchDataIntegrationTest()
@Nested
inner class Authorisation {
@Test
fun `access forbidden when no authority`() {
fun `access unauthorised when no authority`() {
webTestClient.post().uri("/restricted-patient-search/match-restricted-patients")
.header("Content-Type", "application/json")
.exchange()
.expectStatus().isUnauthorized
}

@Test
fun `access forbidden when no role`() {
fun `access forbidden for endpoint POST #restricted-patient-search#match-restricted-patients when no role`() {
webTestClient.post().uri("/restricted-patient-search/match-restricted-patients")
.body(BodyInserters.fromValue(gson.toJson(RestrictedPatientSearchCriteria(null, null, null))))
.headers(setAuthorisation())
Expand Down