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

Adjust behaviour of WithIssuedAt ParserOption (#411) #412

Closed
wants to merge 1 commit into from

Conversation

sknr
Copy link

@sknr sknr commented Oct 17, 2024

Hi @oxisto,

thanks for your quick reply to my Issue (#411).

I created a fix with a handling similar to verifyExpiresAt which is what I also expected from verifyIssuedAt.

Hope that makes sense to you.

Let me know, if you have any further questions.

@oxisto
Copy link
Collaborator

oxisto commented Oct 18, 2024

I am a bit worried about changing the default behaviour here. As I see it we have two options to enable this:

  1. Change the default to the claim to be required when we use WithIssuedAt, this would be in line with the other WithIssuer, WithAudience, etc., which all switch to the claim to be REQUIRED if you specify them
  2. Keep the function as is and introduce a new flag WithIssuedAtRequired, that only controls the required aspect. We already have a WithExpiredAtRequired, so this would be in line with this option, but sort of deviates from the others

There is probably not a good 100 % solution without too much breakage. This calls for some input form our specialist for maintaining API stability @mfridman ;)

@sknr
Copy link
Author

sknr commented Oct 18, 2024

That’s understandable. Please feel free to ping me again if we gather more opinions regarding the two solutions.
I'm happy to implement either one of them.

@sknr
Copy link
Author

sknr commented Oct 21, 2024

@oxisto, @mfridman,

Thank you for sharing your thoughts. Since I can’t implement solution one, I’d prefer not to add another method to the public API and risk polluting it further. Therefore, I’ll handle the case of a nil “iat” claim internally instead of introducing a new flag. I hope that works for everyone. Let me know if you have any concerns!

@sknr sknr closed this Oct 21, 2024
@mfridman
Copy link
Member

Thanks for opening an issue. We'll think through these little details if we ever get around to another major version.

We take backwards compatibility fairly seriously, and appreciate your understanding.

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.

3 participants