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

Authentication #21

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Authentication #21

wants to merge 6 commits into from

Conversation

culka
Copy link
Contributor

@culka culka commented Nov 21, 2023

Use authentication via jore4-auth -service, using the existing Spring session cookie

Resolves #1419


This change is Reviewable

Copy link
Contributor

@Leitsi Leitsi left a 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.

Copy link
Contributor

@jarkkoka jarkkoka left a 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.

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.

3 participants