Skip to content
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

SDIT-1336 Add in Spring Security by including the library hmpps-kotlin-spring-boot-starter #196

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

mikehalmamoj
Copy link
Contributor

@mikehalmamoj mikehalmamoj commented Feb 23, 2024

I think I'm going to keep this draft open for a while and keep it up to date with the hmpps-kotlin-spring-boot-starter. But not planning on merging until we have more confidence in the starter - until it's a bit more battle tested.

@mikehalmamoj mikehalmamoj force-pushed the mh-SDIT-1336-secure-endpoint branch from 56e464e to 9fc5b27 Compare February 23, 2024 16:46
@mikehalmamoj mikehalmamoj marked this pull request as draft February 23, 2024 16:47
Comment on lines 13 to 15
@PreAuthorize("hasRole('TEMPLATE_EXAMPLE')")
@GetMapping
fun getTime() = "${LocalDateTime.now()}"
Copy link
Contributor Author

@mikehalmamoj mikehalmamoj Feb 23, 2024

Choose a reason for hiding this comment

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

This is the secure endpoint we were asked to add by @rj-adams. The plan is to call this from the Typescript template so it has an example of calling a secured endpoint.

I like it because it's simple and there's no harm if somebody forgets to remove it - it doesn't poolute the API model. On the other hand we're going to have to add some kind of model if when we get onto the OpenAPI docs ticket as an example of an OpenAPI specification.

@@ -50,20 +58,3 @@ class HmppsTemplateKotlinExceptionHandler {
private val log = LoggerFactory.getLogger(this::class.java)
}
}

data class ErrorResponse(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now comes from the library hmpps-kotlin-spring-boot-starter

Comment on lines 27 to 30
/**
* TODO
* Once you have a client registration defined in properties `spring.security.client.registration` then you'll
* need to uncomment this @Bean and create both a health and authorized web client.
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've got an idea around generating all web clients in the library hmpps-kotlin-spring-boot-starter by reading the client registrations from configuration and automatically creating web clients for each. But this will have to do for now.

Comment on lines 55 to 59
// @Bean
fun authorizedClientManager(
clientRegistrationRepository: ClientRegistrationRepository,
oAuth2AuthorizedClientService: OAuth2AuthorizedClientService,
): OAuth2AuthorizedClientManager {
Copy link
Contributor Author

@mikehalmamoj mikehalmamoj Feb 23, 2024

Choose a reason for hiding this comment

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

If you don't have any client registrations then Spring doesn't create a ClientRegistrationRepository, hence the @Bean annotation is commented out.

Comment on lines 16 to 17
@RestControllerAdvice
class HmppsTemplateKotlinExceptionHandler {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a prime candidate for moving to the library hmpps-kotlin-spring-boot-starter, as long we make it easy to extend.

@@ -9,6 +9,7 @@ generic-service:

env:
APPLICATIONINSIGHTS_CONFIGURATION_FILE: applicationinsights.dev.json
API_BASE_URL_OAUTH: "https://sign-in-dev.hmpps.service.justice.gov.uk/auth"
Copy link
Contributor

Choose a reason for hiding this comment

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

Wondering if we could have a better variable now for HMPPS Auth instead


@Configuration
class WebClientConfiguration(
@Value("\${api.base.url.oauth}") val oauthApiBaseUri: String,
Copy link
Contributor

Choose a reason for hiding this comment

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

could this be hmppsAuthBaseUri instead?

import org.springframework.web.reactive.function.client.WebClientResponseException
import reactor.core.publisher.Mono

abstract class HealthCheck(private val webClient: WebClient) : ReactiveHealthIndicator {
Copy link
Contributor

Choose a reason for hiding this comment

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

should this be extending the Reactive version?

WireMock.get("/auth/health/ping").willReturn(
aResponse()
.withHeader("Content-Type", "application/json")
.withBody(if (status == 200) "pong" else "some error")
Copy link
Contributor

Choose a reason for hiding this comment

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

don't think it matters too much, but these days this should be {"status":"UP"}

* This is just an example of what a secured endpoint might look like.
* Remove this class and associated tests in [TimeResourceIntTest] and replace with your own implementation.
*/
@RestController
Copy link
Contributor

Choose a reason for hiding this comment

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

wondering if you could do something like only enabling for local / dev/ test based on a bean property so it doesn't get deployed to other people's instances if they don't remove it?


@PreAuthorize("hasRole('TEMPLATE_EXAMPLE')")
@GetMapping
fun getTime() = "${LocalDateTime.now()}"
Copy link
Contributor

@petergphillips petergphillips Feb 26, 2024

Choose a reason for hiding this comment

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

Suggested change
fun getTime() = "${LocalDateTime.now()}"
fun getTime() = LocalDateTime.now().toString()

@mikehalmamoj mikehalmamoj force-pushed the mh-SDIT-1336-secure-endpoint branch 5 times, most recently from abeebb9 to 10f4b45 Compare February 27, 2024 16:39
@mikehalmamoj mikehalmamoj force-pushed the mh-SDIT-1336-secure-endpoint branch from 10f4b45 to 4eb7fb7 Compare February 27, 2024 16:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants