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

Rename OIDC expiration-grace property to lifespan-grace #8658

Merged
merged 1 commit into from
Apr 21, 2020

Conversation

sberyozkin
Copy link
Member

@sberyozkin sberyozkin commented Apr 17, 2020

Fixes #8627

I'm renaming it to lifespan-grace as somehow I'm not keen on leeway :-), it sounds a bit too generic. It is a minor issue though (can rename to leeway if preferred). Note we already use jwt.lifespan property for the client_secret_jwt so lifespan-grace seems not too bad.
Pedro's logout PR uses the expiration-grace property so I'll rebase once his PR is in and there will be a test too, we already have a few timeout tests so I'd rather not add another one yet.
It's better to fix this property name now as it is misleading.

I'm not sure it is worth keeping the expiration-grace as deprecated, the users are quite likely not aware that iat is affected by this property, so it feels the best action is to rename it and send a message.

@Ladicek, hi.

I'd rather not assign it some default value > 0, as we'll open a potential CVE, some user's app will be invoked by the stolen token few secs after the exp (which can happen with the leeway), they will start investigating and find out that it is our fault etc. I agree in many cases no one will probably care but I'd rather them set this property themselves :-)

@geoand
Copy link
Contributor

geoand commented Apr 18, 2020

Please rebase the PR onto the latest master in order to pick a CI fix

@sberyozkin
Copy link
Member Author

@geoand sorry, I'll wait till Pedro confirms #8512 changes I've pushed to his branch are good so that I can merge the whole PR - this will most likely require another rebase here. So I'll wait for a bit

@geoand
Copy link
Contributor

geoand commented Apr 18, 2020

Great, thanks!

@Ladicek
Copy link
Contributor

Ladicek commented Apr 20, 2020

Fair enough re the default 0 value of this property.

@sberyozkin
Copy link
Member Author

sberyozkin commented Apr 20, 2020

Quarkus CI / Native Tests - Data is cancelled. Otherwise looks good.
@stuartwdouglas, @pedroigor do you agree with this property name change ? if we are to rename it then it has to make it to 1.4.0.Final IMHO. Thanks

Copy link
Contributor

@Ladicek Ladicek left a comment

Choose a reason for hiding this comment

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

I like this.

Whether it should go to 1.4 or 1.5, I don't mind.

@sberyozkin
Copy link
Member Author

@Ladicek Thanks, I'll merge to 1.5.0 and ask a question in the forum (as I need to update the forum about the OidcTenantConfig package change anyway)

@sberyozkin
Copy link
Member Author

It already has a backport issue so I will keep it as is, but will mention it in the forum

@sberyozkin sberyozkin merged commit 904d7e0 into quarkusio:master Apr 21, 2020
@Ladicek
Copy link
Contributor

Ladicek commented Apr 21, 2020

need to update the forum about the OidcTenantConfig package change anyway

Not sure about the forum, but this change definitely needs to go to the migration guide (https://github.com/quarkusio/quarkus/wiki/Migration-Guides).

@sberyozkin
Copy link
Member Author

@Ladicek I've started modifying the guide but @gsmet has removed the backport label. Guillaume, should we not go ahead with this backport ? I was trying to say in the guide that this property name can lead to the unexpected verification results, the name is very misleading

@Ladicek
Copy link
Contributor

Ladicek commented Apr 21, 2020

Not backporting is probably reasonable; in that case, it will have to be in the 1.5 migration guide.

@sberyozkin
Copy link
Member Author

OK, will get it to the 1.5 guide unless @stuartwdouglas or @pedroigor will prefer otherwise. Thanks

@sberyozkin sberyozkin deleted the oidc_leeway branch September 16, 2020 10:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

OIDC expiration-grace property name is misleading
4 participants