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

fix: support JWT exp offset as a config parameter and add instead of subtract #39

Merged
merged 2 commits into from
May 31, 2024

Conversation

sdewitt-newrelic
Copy link
Contributor

Adds

  • Expose the expiration offset used when calculating the exp value of the JSON Claims Set for the JWT as a configuration parameter

Fixes

  • When setting the exp value of the JSON Claims Set for the JWT, add to current time instead of subtract

README.md Show resolved Hide resolved
@@ -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:
Copy link
Contributor

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

Copy link
Contributor

@jbeveland27 jbeveland27 May 30, 2024

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

Copy link
Contributor Author

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.

Copy link
Contributor Author

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.
Copy link
Contributor

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} ?

Copy link
Contributor Author

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.

Copy link
Contributor

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call. Done.

@sdewitt-newrelic sdewitt-newrelic self-assigned this May 31, 2024
Copy link
Contributor

@jbeveland27 jbeveland27 left a comment

Choose a reason for hiding this comment

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

ezgif-2-234a3b4833

@sdewitt-newrelic sdewitt-newrelic merged commit 147b5dc into main May 31, 2024
5 checks passed
@jbeveland27 jbeveland27 deleted the fix/jwt-expired-auth-code branch May 31, 2024 21:54
@kanwaljit-mq
Copy link

Issue still there. Not sure how to reopen the bug. so leaving a comment
File "/nr-code/app/src/newrelic_logging/api.py", line 81, in aut
henticate
self.authenticator.authenticate(session)
File "/nr-code/app/src/newrelic_logging/auth.py", line 199, in a
uthenticate
self.authenticate_with_jwt(session)
File "/nr-code/app/src/newrelic_logging/auth.py", line 109, in a
uthenticate_with_jwt
exp = int((
AttributeError: 'tuple' object has no attribute 'timestamp'

error

No tupple exists yet we are trying to access it.

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

Successfully merging this pull request may close these issues.

Getting "expired authorization code" error while creating JWT token
3 participants