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

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

Closed
5 tasks done
IchordeDionysos opened this issue Jul 4, 2024 · 8 comments · Fixed by #4003
Closed
5 tasks done
Labels
rfc A request for comments to discuss and share ideas.

Comments

@IchordeDionysos
Copy link
Contributor

IchordeDionysos commented Jul 4, 2024

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:

  • Support Unix and ISO 8601 in the updated_at field of the OIDC user info
  • Add support for generic providers

Non-goals:

  • Don't add support for pre-configured providers that don't need this workaround
  • Don't add support for other fields

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.

@IchordeDionysos IchordeDionysos added the rfc A request for comments to discuss and share ideas. label Jul 4, 2024
@aeneasr
Copy link
Member

aeneasr commented Jul 8, 2024

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

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

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

  • Does not support leap seconds (note @alnr: the current Auth0 workaround also does not respect leap seconds, see here)
  • Limited parsing possibilities (only available Jsonnet or other functions supported)
  • Requires complex mapping for each provider that deviates from the standard

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
Copy link
Member

aeneasr commented Jul 8, 2024

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

IchordeDionysos commented Jul 8, 2024

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
Copy link
Member

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

IchordeDionysos commented Jul 8, 2024

@IchordeDionysos IchordeDionysos changed the title OIDC: Allow ISO 8601 date format of updated_at timestamp in user claims Add support for Salesforce which uses the ISO 8601 date format for updated_at timestamp in user claims Jul 16, 2024
@IchordeDionysos
Copy link
Contributor Author

IchordeDionysos commented Jul 16, 2024

@aeneasr @jonas-jonas I've opened a PR which fixes the invalid schema for Salesforce :)
#4003

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
rfc A request for comments to discuss and share ideas.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants