-
Notifications
You must be signed in to change notification settings - Fork 0
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
Authentication #21
base: main
Are you sure you want to change the base?
Authentication #21
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 4 of 6 files at r1, all commit messages.
Reviewable status: 4 of 6 files reviewed, 4 unresolved discussions (waiting on @culka)
src/main/kotlin/fi/hsl/jore4/timetables/config/RemoteAuthenticationProvider.kt
line 18 at r1 (raw file):
@Configuration class RemoteAuthenticationProvider : AuthenticationProvider {
I feel it would be quite necessary to have tests for this kind of functionality.
src/main/kotlin/fi/hsl/jore4/timetables/config/RemoteAuthenticationProvider.kt
line 27 at r1 (raw file):
} override fun authenticate(authentication: Authentication?): Authentication {
Was thinking if it would make sense to split this to multiple functions.
Though there's really only 2 logical steps so dunno how much this would help.
Something like:
override fun authenticate(authentication: Authentication?): Authentication {
val authResponse = authenticateWithHasuraWebhook(authentication)
return createAuthenticationToken(authResponse)
}
private fun authenticateWithHasuraWebhook(authentication: Authentication?): HttpResponse<String> { ... }
private fun createAuthenticationToken(authResponse: HttpResponse<String>): Authentication { ... }
src/main/kotlin/fi/hsl/jore4/timetables/config/RemoteAuthenticationProvider.kt
line 30 at r1 (raw file):
val authRequest = HttpRequest.newBuilder().run { uri(URI("http://jore4-auth:8080/public/v1/hasura/webhook")) headers("cookie", "SESSION=${authentication?.principal}", ROLE_HEADER, authentication?.credentials.toString(), "Accept", "application/json")
Could split this to multiple header()
calls, I think:
header("cookie", "SESSION=${authentication?.principal}")
header(ROLE\_HEADER, authentication?.credentials.toString())
header("Accept", "application/json")
src/main/kotlin/fi/hsl/jore4/timetables/config/WebSecurityConfig.kt
line 37 at r1 (raw file):
if ("OPTIONS" == request.method) { response.status = HttpServletResponse.SC_OK } else {
Could eliminate this double else structure.
if ("OPTIONS" \== request.method) {
} else if (sessionCookie == null || sessionCookie.value.isBlank()) {
} else if (roleHeader == null) {
} else {
}
src/main/kotlin/fi/hsl/jore4/timetables/config/WebSecurityConfig.kt
line 41 at r1 (raw file):
logger.info("No session cookie in request http request") } else if (roleHeader == null) { logger.info("No role header in http request")
So, these two "just log and do nothing" cases cause the request to fail.
Could use some tests / documentation.
e9c53ce
to
d698d91
Compare
d698d91
to
e12da2d
Compare
e12da2d
to
bccd3e3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 6 files at r1, 2 of 7 files at r2, 2 of 3 files at r3, all commit messages.
Reviewable status: 8 of 12 files reviewed, 11 unresolved discussions (waiting on @culka and @Leitsi)
src/main/kotlin/fi/hsl/jore4/timetables/config/RemoteAuthenticationProvider.kt
line 29 at r1 (raw file):
override fun authenticate(authentication: Authentication?): Authentication { val authRequest = HttpRequest.newBuilder().run { uri(URI("http://jore4-auth:8080/public/v1/hasura/webhook"))
This should not be hard-coded nut rather defined in config.properties
.
src/main/kotlin/fi/hsl/jore4/timetables/config/RemoteAuthenticationProvider.kt
line 30 at r1 (raw file):
val authRequest = HttpRequest.newBuilder().run { uri(URI("http://jore4-auth:8080/public/v1/hasura/webhook")) headers("cookie", "SESSION=${authentication?.principal}", ROLE_HEADER, authentication?.credentials.toString(), "Accept", "application/json")
Put each header on a new line for better readability.
src/main/kotlin/fi/hsl/jore4/timetables/config/RemoteAuthenticationProvider.kt
line 23 at r2 (raw file):
companion object { val logger = KotlinLogging.logger {}
Change logger
to uppercase and introduce it in top level (outside class) like in other files.
src/main/resources/application.properties
line 8 at r2 (raw file):
jore4.db.maxConnections[email protected]@ jooq.sql.dialect[email protected]@ authentication.url[email protected]@
Please add some newlines between properties having different prefix.
src/test/kotlin/fi/hsl/jore4/timetables/config/RemoteAuthenticationProviderTest.kt
line 38 at r2 (raw file):
override fun statusCode(): Int { return status
I would make this one-liner. So simple.
src/test/kotlin/fi/hsl/jore4/timetables/config/RemoteAuthenticationProviderTest.kt
line 46 at r2 (raw file):
override fun previousResponse(): Optional<HttpResponse<String>> { return Optional.empty()
This could also be a one-liner.
src/test/kotlin/fi/hsl/jore4/timetables/config/RemoteAuthenticationProviderTest.kt
line 65 at r2 (raw file):
} override fun sslSession(): Optional<SSLSession> {
These could be made one-liners.
Use authentication via
jore4-auth
-service, using the existing Spring session cookieResolves #1419
This change is