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

Feat: Configure refresh token expiry #4525

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

Conversation

thejwuscript
Copy link
Contributor

Related to issue #4409

This PR allows project admins to optionally configure the expiry of a refresh token for a ClientApplication.

A refreshTokenExpiry field is added to the ClientApplication resource. The field is optional to allow backward compatibility. The value is used to generate the exp claim on a refresh token.

UI changes to the Medplum App is not addressed in this PR; it can be included here if necessary, or be made on a separate PR.

@thejwuscript thejwuscript requested a review from a team as a code owner May 7, 2024 15:36
Copy link

vercel bot commented May 7, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
medplum-provider ✅ Ready (Inspect) Visit Preview 💬 Add feedback May 21, 2024 6:56pm
3 Ignored Deployments
Name Status Preview Comments Updated (UTC)
medplum-app ⬜️ Ignored (Inspect) Visit Preview May 21, 2024 6:56pm
medplum-storybook ⬜️ Ignored (Inspect) Visit Preview May 21, 2024 6:56pm
medplum-www ⬜️ Ignored (Inspect) Visit Preview May 21, 2024 6:56pm

Copy link

vercel bot commented May 7, 2024

@thejwuscript is attempting to deploy a commit to the Medplum Team on Vercel.

A member of the Team first needs to authorize it.

"min" : 0,
"max" : "1",
"type" : [{
"code" : "date"
Copy link
Contributor

Choose a reason for hiding this comment

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

for our use case we'd like to specify a value here which is relative to the current time, not a fixed date parameter.

the expiration value is passed to the jose npm package, which treats number and Date values as the fixed expiration time in the claim (reference here), however string values are converted to a time span relative to the current time (reference here) - which matches the existing behaviour with the 2w default.


i think ideally this property would be a string which could either support the full range of jose formats, or perhaps a subset of this (to allow for easier validation in the medplum middleware)

Copy link
Member

Choose a reason for hiding this comment

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

Yes I think if we can change the name of this field to refreshTokenLifetime and make it an expiry duration as a string (eg. '2w', '1h', etc.) that maps to jose formats as @josh- pointed out, that would be ideal 👍

Copy link
Member

Choose a reason for hiding this comment

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

@thejwuscript - just wanted to make sure you saw the above comment from @ThatOneBro . This is our main feedback on the PR

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, thanks very much for the feedback. I'll be making the changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

None yet

5 participants