-
Notifications
You must be signed in to change notification settings - Fork 16
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
fix: support JWT exp offset as a config parameter and add instead of subtract #39
Conversation
src/newrelic_logging/auth.py
Outdated
@@ -231,6 +235,12 @@ def validate_jwt_config(auth: dict) -> dict: | |||
if not auth['audience']: | |||
raise ConfigException('audience', 'missing JWT audience') | |||
|
|||
if auth['exp_offset'] < 0: |
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.
Can it be 0? I would've thought <= 0
here
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.
Oh but your error message just says "not be negative". So if that's what you meant, nevermind
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.
@jbeveland27 specifically want to prevent 0 minutes because I think that will cause the same error that this PR is supposed to address #31 which is happening in that case due to a negative offset. At 0, it would basically say the expiration is "now" rather than "5 minutes ago" (which is the source of the original problem). So I think 0 is still not valid.
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.
@jbeveland27 realized what you were saying now. I have changed the code to be <=
and adjusted the error message.
README.md
Outdated
_must_ be a positive integer. | ||
|
||
The expiration offset can also be specified using the | ||
`{optional-prefix}SF_EXPIRATION_OFFSET` environment variable. |
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.
what's the {optional-prefix}
?
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.
@jbeveland27 That is explained here so hopefully the user is able to connect these two parts of the doc.
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.
Ah got it. So my 2 cents: the README is massive and the prefix is called auth_env_prefix
, so maybe either leave this out or make this {auth_env_prefix}
for clarity?
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.
Good call. Done.
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.
Adds
exp
value of the JSON Claims Set for the JWT as a configuration parameterFixes
exp
value of the JSON Claims Set for the JWT, add to current time instead of subtract