ory / kratos

Next-gen identity server replacing your Auth0, Okta, Firebase with hardened security and PassKeys, SMS, OIDC, Social Sign In, MFA, FIDO, TOTP and OTP, WebAuthn, passwordless and much more. Golang, headless, API-first. Available as a worry-free SaaS with the fairest pricing on the market!
https://www.ory.sh/kratos/?utm_source=github&utm_medium=banner&utm_campaign=kratos
Apache License 2.0
10.98k stars 949 forks source link

Add support for Salesforce which uses the ISO 8601 date format for updated_at timestamp in user claims #3984

Closed IchordeDionysos closed 1 month ago

IchordeDionysos commented 1 month ago

Preflight checklist

Ory Network Project

No response

Context and scope

Not all identity providers follow the OIDC standard (like Auth0, Salesforce, …).

A workaround has been added for the pre-configured Auth0 identity provider. For generic providers, it's not possible to have an ISO 8601 date (e.g. 2013-12-02T18:46:42Z)

Goals and non-goals

Goals:

Non-goals:

The design

When fetching the OIDC claims for a generic provider, check which type the updated_at in the user info object has:

If it's a string and an ISO date, convert it to a Unix timestamp in the user info object. If it's not a string or iSO date, leave the updated_at field untouched.

APIs

No response

Data storage

No response

Code and pseudo-code

func tranformUpdatedAtISODateWorkaround(body []byte) ([]byte, error) {
    // Force updatedAt to be an int if given as a string in the response.
    if updatedAtField := gjson.GetBytes(body, "updated_at"); updatedAtField.Exists() && updatedAtField.Type == gjson.String {
        t, err := time.Parse(time.RFC3339, updatedAtField.String())
        if err != nil {
            return nil, errors.WithStack(herodot.ErrInternalServerError.WithReasonf("bad time format in updated_at"))
        }
        body, err = sjson.SetBytes(body, "updated_at", t.Unix())
        if err != nil {
            return nil, errors.WithStack(herodot.ErrInternalServerError.WithReasonf("%s", err))
        }
    }
    return body, nil
}

Code was taken from existing Auth0 workaround.

Degree of constraint

No response

Alternatives considered

There was some discussion (by @alnr) around whether to convert the date using in the Jsonnet snippet, using the toUnixTimestamp function. The drawback of this solution might be that it does not properly handle leap seconds.

aeneasr commented 1 month ago

Thank you for the suggestion. I agree that this is a problem and at the same time it's disappointing that many upstream providers have this issue.

In my view, a fix should be provider-specific. Also, I think that the transformation should be done using a custom JSON marshaller/unmarshaller as we don't want to be dependent on the field being called updated_at.

There's also some libraries for that: https://github.com/joeshaw/iso8601

IchordeDionysos commented 1 month ago

In my view, a fix should be provider-specific.

The issue is with the fix being applied only to specific providers. If you need to integrate a custom provider not yet available in Kratos, you can't use that provider until out-of-the-box support is added.

IchordeDionysos commented 1 month ago

Potential alternative:

Have an optional user_info_mapper_url that points to a Jsonnet file or other similar mapper that maps the necessary fields used by Kratos of the user info response to the user claims.

This is more complex to integrate, though supporting more providers and different formats would be more abstract.

Benefits

Downsides

Next steps

@aeneasr @alnr or others, do we have some idea how much other implementations deviate from the standard? Is it required to be able to use updatedAt instead of updated_at? Or is the date format the only deviation from the standard?

aeneasr commented 1 month ago

In my view, a fix should be provider-specific.

The issue is with the fix being applied only to specific providers. If you need to integrate a custom provider not yet available in Kratos, you can't use that provider until out-of-the-box support is added.

Yes, that is the case in my view. The OIDC generic provider should only work with spec compliant providers. For anything that needs custom code we need dedicated code in my view.

IchordeDionysos commented 1 month ago

In itself, I don't think something would speak against that approach if two conditions are true:

  1. You would be happy to add support for non-compliant SSO providers (e.g. Salesforce)

  2. ✅ (this is possible) It's possible to set up multiple pre-configured providers of the same type.

e.g. Salesforce itself heavily relies on tenant based authentication, there is not a single Issuer URL that is used for all Salesforce installations.

So you should be able to somehow set up two independent Salesforce SSO connections.

--

The first is more an ongoing commitment to add support for those providers as needed (or at least accept PR for those). The second, I believe, is right now not possible with Ory Kratos and/or Network? So would need some adjustments on how you can configure those pre-configured providers. (Maybe I'm wrong and multiple (e.g. Auth0 providers) are possible via the API but not via the UI?)

jonas-jonas commented 1 month ago
  1. Is possible, it's just not possible in the Ory Console (on the backlog to be fixed, because this is also problematic for other situations/providers). As of now, you can set different IDs for each provider in the configuration via the CLI.
IchordeDionysos commented 1 month ago
  1. Ahh yes thanks ☺️ I've found @vinckr comment in Slack: https://ory-community.slack.com/archives/C02MR4DEEGH/p1719391550931369?thread_ts=1719326079.593729&cid=C02MR4DEEGH
IchordeDionysos commented 1 month ago

@aeneasr @jonas-jonas I've opened a PR which fixes the invalid schema for Salesforce :) https://github.com/ory/kratos/pull/4003