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: claims.update_at is float64 #3867

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

fix: claims.update_at is float64 #3867

wants to merge 1 commit into from

Conversation

kghost
Copy link

@kghost kghost commented Apr 11, 2024

Related issue(s)

https://community.ory.sh/t/failed-to-unmarshal-updated-at-for-generic-oidc-provider-auth0/2004

When using one ory+hydra as OIDC provider to authenticated another kratos, I got an error that it is unable to unmarshal Claims.updated_at, because kratos-selfservice-ui-node returns the session and convert updated_at to a float64, because javascript/typescript doesn't have int64 type.

Checklist

  • I have read the contributing guidelines.
  • I have referenced an issue containing the design document if my change
    introduces a new feature.
  • I am following the
    contributing code guidelines.
  • I have read the security policy.
  • I confirm that this pull request does not address a security
    vulnerability. If this pull request addresses a security vulnerability, I
    confirm that I got the approval (please contact
    security@ory.sh) from the maintainers to push
    the changes.
  • I have added tests that prove my fix is effective or that my feature
    works.
  • I have added or changed the documentation.

Further Comments

The proposed fix makes it competible with multiple types of updated_at field.

@CLAassistant
Copy link

CLAassistant commented Apr 11, 2024

CLA assistant check
All committers have signed the CLA.

@alnr
Copy link
Contributor

alnr commented Apr 12, 2024

Shouldn't this rather be fixed in ory/kratos-selfservice-ui-node?

@kghost
Copy link
Author

kghost commented Apr 15, 2024

Shouldn't this rather be fixed in ory/kratos-selfservice-ui-node?

Sure, this problem can be fix there, by this PR makes our Kratos more robust without any downside, so can we consider it an improvement ?

@alnr
Copy link
Contributor

alnr commented Apr 15, 2024

Shouldn't this rather be fixed in ory/kratos-selfservice-ui-node?

Sure, this problem can be fix there, by this PR makes our Kratos more robust without any downside, so can we consider it an improvement ?

The reason I'm hesitant is that Kratos would accept payloads which are arguably malformed. A unix timestamp is always an integer. If we start accepting floats too, we have to decide on how to handle rounding etc. I'd rather not do that and be more strict. Especially since we could never go back to only accepting integers because that would break existing usage.

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.

None yet

3 participants