-
Notifications
You must be signed in to change notification settings - Fork 40
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 Microservice POC #15765
Conversation
auth/src/test/kotlin/gov/cdc/prime/reportstream/auth/controller/AuthControllerTest.kt
Outdated
Show resolved
Hide resolved
...sions/src/main/kotlin/gov/cdc/prime/reportstream/submissions/controllers/HealthController.kt
Outdated
Show resolved
Hide resolved
all in all this is awesome work! nice job couple of quick things
|
http | ||
.authorizeExchange { authorize -> | ||
authorize | ||
.pathMatchers("/health").permitAll() // allow health endpoint without authentication |
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.
see comment in controller re keeping things DRY
auth/src/main/kotlin/gov/cdc/prime/reportstream/auth/controller/HealthController.kt
Outdated
Show resolved
Hide resolved
submissions/src/main/kotlin/gov/cdc/prime/reportstream/submissions/config/SecurityConfig.kt
Show resolved
Hide resolved
incomingUri.fragment | ||
) | ||
} else { | ||
throw IllegalStateException("no configured proxy target in path mappings for path=${incomingUri.path}") |
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.
how is this - and for that matter any - unexpected exception handled? If memory serves spring-boot will return an html document to the user. If that's what's going on and that's not what we want - you can use @RestControllerAdvice to create a global catch-all to ensure all exceptions result in a consistent return value protocol to the user.
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.
Right now it returns a default Spring json response. It's definitely in my todo list for the app to make that better but figured it was ok for now.
one more thing! there's a plugin we can use to auto-generate swagger doc https://springdoc.org/#gradle-plugin It auto generates both the openapi file and creates an endpoint to view the API contract |
I think this is good enough for now. The current idea is that this will not be hooked up to any db or datastore so thats not something we'd have to worry about. |
…ortstream into platform/jamie/auth-poc
…ortstream into platform/jamie/auth-poc
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.
Nice stuff! Since this is just a POC I'm good moving forward with this.
|
||
testImplementation("org.springframework.boot:spring-boot-starter-test") | ||
testImplementation("org.springframework.security:spring-security-test") | ||
testImplementation("org.jetbrains.kotlin:kotlin-test-junit5") |
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.
I think some of these can be consolidated into the shared plugin?
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.
I'm not sure we need this file?
@@ -0,0 +1,7 @@ | |||
distributionBase=GRADLE_USER_HOME |
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.
Same here
auth/src/main/kotlin/gov/cdc/prime/reportstream/auth/config/ApplicationConfig.kt
Outdated
Show resolved
Hide resolved
oauth2: | ||
resourceserver: | ||
opaquetoken: # Set client secret in SPRING_SECURITY_OAUTH2_RESOURCESERVER_OPAQUETOKEN_CLIENT_SECRET env variable | ||
client-id: 0oaek8tip2lhrhHce1d7 |
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.
I think we'll need to figure out how to make this per env too?
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.
I believe that environment variables take precedence over configuration in application.yml so will just need to set the correct env variables per environment.
import kotlin.time.TimeSource | ||
|
||
@RestController | ||
class HealthController( |
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.
Maybe out of scope, but this could also check that okta is available?
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.
interesting idea. It's possible. I would hesitate to tie it directly to the healthcheck endpoint as those can sometimes be used to ensure the app is functioning properly in a k8s environment (Okta going down could take us down with it).
/** | ||
* Implementations are ways to decide the ultimate destination of an incoming request | ||
*/ | ||
interface ProxyURIStrategy { |
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.
Is this what the spring gateway would replace?
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.
Thats the idea.
We could define it in configuration that
/reports -> http://localhost:8000/reports
/submission -> http://localhost:8888/submission
etc
azure.storage.table-name=${AZURE_STORAGE_TABLE_NAME:submission} | ||
spring.security.oauth2.resourceserver.jwt.issuer-uri=https://reportstream.oktapreview.com/oauth2/ausekaai7gUuUtHda1d7 |
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.
Probably a follow up, but this is all going to be per env right?
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.
Yes. This is the Okta preview site. In production we would set an env variable pointing to production Okta.
submissions/src/main/kotlin/gov/cdc/prime/reportstream/submissions/config/SecurityConfig.kt
Outdated
Show resolved
Hide resolved
…plicationConfig.kt Co-authored-by: Michael Kalish <[email protected]>
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.
nice job!
This PR creates a new Spring Microservice focused on authentication via Okta and proxying requests to the right service.
Test Steps:
Changes
Checklist
Testing
./prime test
or./gradlew testSmoke
against local Docker ReportStream container?Linked Issues