-
Notifications
You must be signed in to change notification settings - Fork 4
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
FI-2254 Implement token introspection #112
Conversation
@arscan confirming that this implementation is working with the instructions you provided. Thanks for getting this set up! |
14e2a2d
to
c6daa31
Compare
c6daa31
to
72191c2
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.
Looked through the code and I don't have any real concerns, just a couple trivial notes. I want to run through the g10 test kit with this myself tomorrow though
Token customBearerToken = new Token(customBearerTokenString, | ||
FhirReferenceServerUtils.getScopesListByScopeString(CUSTOM_BEARER_TOKEN_SCOPE_STRING)); | ||
customBearerToken.setClientId("SAMPLE_CLIENT_ID"); | ||
customBearerToken.setExp(java.time.Instant.now().getEpochSecond() + expiresIn); |
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.
Not sure if this is really better but you could leverage the java time libraries to do the time math:
customBearerToken.setExp(Instant.now().plus(4, ChronoUnit.MONTHS).getEpochSecond());
} | ||
|
||
} catch (TokenNotFoundException tokenNotFoundException) { | ||
// This doesn't feel quit right, but am staying consistent |
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.
Reading up a little on the token introspection endpoint, this seems fine to me. I'm not sure even logging the exception is that necessary.
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.
Works well for me! I tested it with both a public client ID and a confidential client ID and secret, everything (except TLS tests of course) passed and ref server output looks good.
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.
The Token Introspection Endpoint URL is not listed here but may be worth including. I know it's not strictly necessary since it's included as part of the .well-known endpoints, but including it would allow someone to run only group 3 Token Introspection without first having to run Group 1 to get the endpoint. Ultimately up to you though!
Summary
Adds token introspection capabilities per openid token introspection standard
Testing guidance
I recommend that @alisawallace verifies the token introspection stuff, and that @Jammjammjamm or @dehall just does a quick check to make sure that I didn't mess up any of the other stuff. g10 tests seem to pass still when i run this (and running g10 helped me identify a problem with the capstatement).
For testing against the new token introspection tests: I recommend:
docker-compose up --build
. Note, the first time, the db may be slow to launch because it is loading data, and you'll see an issue in the console and the site won't load. Kill the container and rerundocker-compose up
, as the faster start to the db after it has been initialized should let the web server load*optiona: POST to this endpoint with no auth headers (I haven't added authorization here), and a body of
token=ABC
, and it should return json to the effect of{'active': false}
In the smart app launch test kit, assuming a 'ruby development setup', do the following:
.env
and.env.development
theCOMPOSE_PROFILES=keycloak
line -- i was having a little bit of trouble with this interferring with the reference server when running it at the same time, so lets just not start keycloak any more.inferno services start
to start the background services (db, redis, etc)inferno start
to start the applicationThis should allow us to pivot over to a more realistic test scenario where we aren't spread across two different auth servers in the test kit.