From 2b45e7953787ad46a6937fe44cb24b6c786eb223 Mon Sep 17 00:00:00 2001 From: aeneasr Date: Tue, 5 May 2020 17:08:54 +0200 Subject: [PATCH] refactor: replace oidc jsonschema with jsonnet This patch replaces the previous methodology of merging OIDC data which used JSON Schema with Extensions and JSON Path in favor of a much easier to use approach with JSONNet. Closes #380 BREAKING CHANGE: This is a breaking change as previous OIDC configurations will not work. Please consult the newly written documentation on OpenID Connect to learn how to use OIDC in your login and registration flows. Since the OIDC feature was not publicly broadcasted yet we have chosen not to provide an upgrade path. If you have issues, please reach out on the forums or slack. --- go.mod | 4 +- go.sum | 10 +++- selfservice/strategy/oidc/extension.go | 42 --------------- selfservice/strategy/oidc/extension_test.go | 52 ------------------- selfservice/strategy/oidc/form.go | 6 ++- selfservice/strategy/oidc/provider_config.go | 4 +- .../strategy/oidc/provider_generic_test.go | 2 +- selfservice/strategy/oidc/strategy.go | 48 +++++++---------- selfservice/strategy/oidc/strategy_test.go | 4 +- .../strategy/oidc/stub/hydra.schema.json | 25 --------- .../strategy/oidc/stub/oidc.hydra.jsonnet | 11 ++++ 11 files changed, 52 insertions(+), 156 deletions(-) delete mode 100644 selfservice/strategy/oidc/extension.go delete mode 100644 selfservice/strategy/oidc/extension_test.go delete mode 100644 selfservice/strategy/oidc/stub/hydra.schema.json create mode 100644 selfservice/strategy/oidc/stub/oidc.hydra.jsonnet diff --git a/go.mod b/go.mod index 951789f79151..a4e8f126ffb2 100644 --- a/go.mod +++ b/go.mod @@ -25,10 +25,12 @@ require ( github.com/gobuffalo/packr/v2 v2.8.0 github.com/gobuffalo/pop/v5 v5.0.11 github.com/gobuffalo/uuid v2.0.5+incompatible + github.com/gobwas/glob v0.2.3 // indirect github.com/gofrs/uuid v3.2.0+incompatible github.com/golang/gddo v0.0.0-20190904175337-72a348e765d2 github.com/golang/mock v1.3.1 github.com/google/go-github/v27 v27.0.1 + github.com/google/go-jsonnet v0.15.1-0.20200415122941-8a0084e64395 github.com/google/uuid v1.1.1 github.com/gorilla/context v1.1.1 github.com/gorilla/sessions v1.1.3 @@ -54,7 +56,7 @@ require ( github.com/ory/mail/v3 v3.0.0 github.com/ory/sdk/swagutil v0.0.0-20200425113349-92ce176f501e github.com/ory/viper v1.7.4 - github.com/ory/x v0.0.116 + github.com/ory/x v0.0.117 github.com/phayes/freeport v0.0.0-20180830031419-95f893ade6f2 github.com/pkg/errors v0.9.1 github.com/shurcooL/go v0.0.0-20180423040247-9e1955d9fb6e diff --git a/go.sum b/go.sum index b75d933ea0ed..0b0f3487755e 100644 --- a/go.sum +++ b/go.sum @@ -549,6 +549,8 @@ github.com/gobuffalo/validate/v3 v3.2.0 h1:Zrpkz2kuZ4rGXLaO3IHVlwX512/cUWRvNjw46 github.com/gobuffalo/validate/v3 v3.2.0/go.mod h1:PrhDOdDHxtN8KUgMvF3TDL0r1YZXV4sQnyFX/EmeETY= github.com/gobuffalo/x v0.0.0-20181003152136-452098b06085/go.mod h1:WevpGD+5YOreDJznWevcn8NTmQEW5STSBgIkpkjzqXc= github.com/gobuffalo/x v0.0.0-20181007152206-913e47c59ca7/go.mod h1:9rDPXaB3kXdKWzMc4odGQQdG2e2DIEmANy5aSJ9yesY= +github.com/gobwas/glob v0.2.3 h1:A4xDbljILXROh+kObIiy5kIaPYD8e96x1tgBhUI5J+Y= +github.com/gobwas/glob v0.2.3/go.mod h1:d3Ez4x06l9bZtSvzIay5+Yzi0fmZzPgnTbPcKjJAkT8= github.com/gofrs/uuid v3.1.0+incompatible/go.mod h1:b2aQJv3Z4Fp6yNu3cdSllBxTCLRxnplIgP/c0N/04lM= github.com/gofrs/uuid v3.2.0+incompatible h1:y12jRkkFxsd7GpqdSZ+/KCs/fJbqpEXSGd4+jfEaewE= github.com/gofrs/uuid v3.2.0+incompatible/go.mod h1:b2aQJv3Z4Fp6yNu3cdSllBxTCLRxnplIgP/c0N/04lM= @@ -585,6 +587,10 @@ github.com/google/go-cmp v0.3.1 h1:Xye71clBPdm5HgqGwUkwhbynsUJZhDbS20FvLhQ2izg= github.com/google/go-cmp v0.3.1/go.mod h1:8QqcDgzrUqlUb/G2PQTWiueGozuR1884gddMywk6iLU= github.com/google/go-github/v27 v27.0.1 h1:sSMFSShNn4VnqCqs+qhab6TS3uQc+uVR6TD1bW6MavM= github.com/google/go-github/v27 v27.0.1/go.mod h1:/0Gr8pJ55COkmv+S/yPKCczSkUPIM/LnFyubufRNIS0= +github.com/google/go-jsonnet v0.15.0 h1:lEUXTDnVsHu+CLLzMeWAdWV4JpCgkJeDqdVNS8RtyuY= +github.com/google/go-jsonnet v0.15.0/go.mod h1:ex9QcU8vzXQUDeNe4gaN1uhGQbTYpOeZ6AbWdy6JbX4= +github.com/google/go-jsonnet v0.15.1-0.20200415122941-8a0084e64395 h1:PftVLaNFPyiHId46033ADWFgXAWIwSDK9ESNRIKdj1Q= +github.com/google/go-jsonnet v0.15.1-0.20200415122941-8a0084e64395/go.mod h1:sOcuej3UW1vpPTZOr8L7RQimqai1a57bt5j22LzGZCw= github.com/google/go-querystring v1.0.0 h1:Xkwi/a1rcvNg1PPYe5vI8GbeBY/jrVuDX5ASuANWTrk= github.com/google/go-querystring v1.0.0/go.mod h1:odCYkC5MyYFN7vkCjXpyrEuKhc/BUO6wN/zVPAxq5ck= github.com/google/martian v2.1.0+incompatible h1:/CP5g8u/VJHijgedC/Legn3BAbAaWPgecwXBIDzw5no= @@ -915,8 +921,8 @@ github.com/ory/x v0.0.85/go.mod h1:s44V8t3xyjWZREcU+mWlp4h302rTuM4aLXcW+y5FbQ8= github.com/ory/x v0.0.88/go.mod h1:wrnJRjIfYXFY/AUiuUlcIUpLBDxFtWc+8x6toAeLZXU= github.com/ory/x v0.0.93/go.mod h1:lfcTaGXpTZs7IEQAW00r9EtTCOxD//SiP5uWtNiz31g= github.com/ory/x v0.0.110/go.mod h1:DJfkE3GdakhshNhw4zlKoRaL/ozg/lcTahA9OCih2BE= -github.com/ory/x v0.0.116 h1:gq47UBzFe9l8n4CToLFMAkjNwqTR+oq1JZYxhA0T5dM= -github.com/ory/x v0.0.116/go.mod h1:ImFneVZHXPCeI1EYXLzRylIkOUMQnWT9Xwuasd8QHxw= +github.com/ory/x v0.0.117 h1:Ronk2N5NwgPn7OytRlS9rrGJCyg+MbJDH6xLlXCHQAo= +github.com/ory/x v0.0.117/go.mod h1:ImFneVZHXPCeI1EYXLzRylIkOUMQnWT9Xwuasd8QHxw= github.com/parnurzeal/gorequest v0.2.15/go.mod h1:3Kh2QUMJoqw3icWAecsyzkpY7UzRfDhbRdTjtNwNiUE= github.com/pborman/uuid v1.2.0 h1:J7Q5mO4ysT1dv8hyrUGHb9+ooztCXu1D8MY8DZYsu3g= github.com/pborman/uuid v1.2.0/go.mod h1:X/NO0urCmaxf9VXbdlT7C2Yzkj2IKimNn4k+gtPdI/k= diff --git a/selfservice/strategy/oidc/extension.go b/selfservice/strategy/oidc/extension.go deleted file mode 100644 index 90e9fd0624fd..000000000000 --- a/selfservice/strategy/oidc/extension.go +++ /dev/null @@ -1,42 +0,0 @@ -package oidc - -import ( - "sync" - - "github.com/tidwall/sjson" - - "github.com/ory/jsonschema/v3" - - "github.com/ory/kratos/identity" - "github.com/ory/kratos/schema" -) - -type ValidationExtensionRunner struct { - l sync.Mutex - i *identity.Identity -} - -func NewValidationExtensionRunner(i *identity.Identity) *ValidationExtensionRunner { - return &ValidationExtensionRunner{i: i} -} - -func (r *ValidationExtensionRunner) Run(ctx jsonschema.ValidationContext, config schema.ExtensionConfig, value interface{}) error { - r.l.Lock() - defer r.l.Unlock() - - if len(config.Mappings.Identity.Traits) > 0 { - for _, t := range config.Mappings.Identity.Traits { - res, err := sjson.SetBytes(r.i.Traits, t.Path, value) - if err != nil { - return ctx.Error("ory.sh\\/kratos/mappings/identity/traits", "unable to apply mapping: %s", err) - } - - r.i.Traits = res - } - } - return nil -} - -func (r *ValidationExtensionRunner) Finish() error { - return nil -} diff --git a/selfservice/strategy/oidc/extension_test.go b/selfservice/strategy/oidc/extension_test.go deleted file mode 100644 index 3e01c44f60d3..000000000000 --- a/selfservice/strategy/oidc/extension_test.go +++ /dev/null @@ -1,52 +0,0 @@ -package oidc_test - -import ( - "fmt" - "os" - "testing" - - "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" - - "github.com/ory/jsonschema/v3" - - "github.com/ory/kratos/identity" - "github.com/ory/kratos/schema" - "github.com/ory/kratos/selfservice/strategy/oidc" -) - -func TestValidationExtensionRunner(t *testing.T) { - for k, tc := range []struct { - expectErr error - schema string - doc string - expect string - }{ - { - doc: "./stub/extension/payload.json", - schema: "file://stub/extension/schema.json", - expect: `{"email": "someone@email.org","names": ["peter","pan"]}`, - }, - } { - t.Run(fmt.Sprintf("case=%d", k), func(t *testing.T) { - c := jsonschema.NewCompiler() - runner, err := schema.NewExtensionRunner(schema.ExtensionRunnerOIDCMetaSchema) - require.NoError(t, err) - - var i identity.Identity - runner. - AddRunner(oidc.NewValidationExtensionRunner(&i)). - Register(c) - - doc, err := os.Open(tc.doc) - require.NoError(t, err) - - err = c.MustCompile(tc.schema).Validate(doc) - if tc.expectErr != nil { - require.EqualError(t, err, tc.expectErr.Error()) - } - - assert.JSONEq(t, tc.expect, string(i.Traits)) - }) - } -} diff --git a/selfservice/strategy/oidc/form.go b/selfservice/strategy/oidc/form.go index c638b0c13629..1482a4c8a3d9 100644 --- a/selfservice/strategy/oidc/form.go +++ b/selfservice/strategy/oidc/form.go @@ -10,6 +10,8 @@ import ( "github.com/tidwall/sjson" "github.com/ory/x/decoderx" + + "github.com/ory/kratos/identity" ) func decoderRegistration(ref string) (decoderx.HTTPDecoderOption, error) { @@ -28,9 +30,9 @@ func decoderRegistration(ref string) (decoderx.HTTPDecoderOption, error) { // merge merges the userFormValues (extracted from the initial POST request) prefixed with `traits` (encoded) with the // values coming from the OpenID Provider (openIDProviderValues). -func merge(userFormValues string, openIDProviderValues json.RawMessage, option decoderx.HTTPDecoderOption) (json.RawMessage, error) { +func merge(userFormValues string, openIDProviderValues json.RawMessage, option decoderx.HTTPDecoderOption) (identity.Traits, error) { if userFormValues == "" { - return openIDProviderValues, nil + return identity.Traits(openIDProviderValues), nil } var decodedForm struct { diff --git a/selfservice/strategy/oidc/provider_config.go b/selfservice/strategy/oidc/provider_config.go index 29379110f7ae..af0c71ff2bd3 100644 --- a/selfservice/strategy/oidc/provider_config.go +++ b/selfservice/strategy/oidc/provider_config.go @@ -43,7 +43,9 @@ type Configuration struct { // Scope specifies optional requested permissions. Scope []string `json:"scope"` - SchemaURL string `json:"schema_url"` + // Normalize specifies the JSONNet code snippet which uses the OpenID Connect Provider's data (e.g. GitHub or Google + // profile information) to hydrate the identity's data. + Normalize string `json:"normalizer"` } func (p Configuration) Redir(public *url.URL) string { diff --git a/selfservice/strategy/oidc/provider_generic_test.go b/selfservice/strategy/oidc/provider_generic_test.go index f3485aa50ef6..f3f71442ae1c 100644 --- a/selfservice/strategy/oidc/provider_generic_test.go +++ b/selfservice/strategy/oidc/provider_generic_test.go @@ -21,7 +21,7 @@ func makeAuthCodeURL(t *testing.T, r *login.Request) string { ClientID: "client", ClientSecret: "secret", IssuerURL: "https://accounts.google.com", - SchemaURL: "file://./stub/hydra.schema.json", + Normalize: "file://./stub/hydra.schema.json", }, public) c, err := p.OAuth2(context.TODO()) require.NoError(t, err) diff --git a/selfservice/strategy/oidc/strategy.go b/selfservice/strategy/oidc/strategy.go index 6e4953b88807..820a6753df78 100644 --- a/selfservice/strategy/oidc/strategy.go +++ b/selfservice/strategy/oidc/strategy.go @@ -10,15 +10,16 @@ import ( "strings" "github.com/gofrs/uuid" + "github.com/google/go-jsonnet" "github.com/julienschmidt/httprouter" "github.com/pkg/errors" "github.com/ory/x/errorsx" + "github.com/ory/x/fetcher" "github.com/ory/x/jsonx" "github.com/ory/herodot" - "github.com/ory/x/stringsx" "github.com/ory/x/urlx" "github.com/ory/kratos/selfservice/flow/login" @@ -85,6 +86,7 @@ type dependencies interface { type Strategy struct { c configuration.Provider d dependencies + f *fetcher.Fetcher validator *schema.Validator } @@ -117,6 +119,7 @@ func NewStrategy( return &Strategy{ c: c, d: d, + f: fetcher.NewFetcher(), validator: schema.NewValidator(), } } @@ -379,57 +382,46 @@ func (s *Strategy) processRegistration(w http.ResponseWriter, r *http.Request, a return } - i := identity.NewIdentity(configuration.DefaultIdentityTraitsSchemaID) - runner, err := schema.NewExtensionRunner(schema.ExtensionRunnerOIDCMetaSchema, NewValidationExtensionRunner(i)) + jn, err := s.f.Fetch(provider.Config().Normalize) if err != nil { s.handleError(w, r, a.GetID(), nil, err) return } - var doc bytes.Buffer - if err := json.NewEncoder(&doc).Encode(claims); err != nil { + var jsonClaims bytes.Buffer + if err := json.NewEncoder(&jsonClaims).Encode(claims); err != nil { s.handleError(w, r, a.GetID(), nil, err) return } - // Validate the claims first (which will also copy the values around based on the schema) - if err := s.validator.Validate( - stringsx.Coalesce( - provider.Config().SchemaURL, - ), - doc.Bytes(), - schema.WithExtensionRunner(runner), - ); err != nil { - s.d.Logger(). - WithField("provider", provider.Config().ID). - WithField("schema_url", provider.Config().SchemaURL). - WithField("claims", fmt.Sprintf("%+v", claims)). - Error("Unable to validate claims against provider schema. Your schema should work regardless of these values.") - // Force a system error because this can not be resolved by the user. - s.handleError(w, r, a.GetID(), nil, errors.WithStack(herodot.ErrInternalServerError.WithTrace(err).WithReasonf("%s", err))) + vm := jsonnet.MakeVM() + vm.ExtCode("claims", jsonClaims.String()) + snippetTraits, err := vm.EvaluateSnippet(provider.Config().Normalize, jn.String()) + if err != nil { + s.handleError(w, r, a.GetID(), nil, err) return } + i := identity.NewIdentity(configuration.DefaultIdentityTraitsSchemaID) + i.Traits = identity.Traits(snippetTraits) option, err := decoderRegistration(s.c.DefaultIdentityTraitsSchemaURL().String()) if err != nil { s.handleError(w, r, a.GetID(), nil, err) return } - traits, err := merge( + i.Traits, err = merge( x.SessionGetStringOr(r, s.d.CookieManager(), sessionName, sessionFormState, ""), - json.RawMessage(i.Traits), option, + json.RawMessage(snippetTraits), option, ) if err != nil { s.handleError(w, r, a.GetID(), nil, err) return } - i.Traits = identity.Traits(traits) - // Validate the identity itself if err := s.d.IdentityValidator().Validate(i); err != nil { - s.handleError(w, r, a.GetID(), traits, err) + s.handleError(w, r, a.GetID(), i.Traits, err) return } @@ -440,7 +432,7 @@ func (s *Strategy) processRegistration(w http.ResponseWriter, r *http.Request, a Provider: provider.Config().ID, }, }); err != nil { - s.handleError(w, r, a.GetID(), traits, errors.WithStack(herodot.ErrInternalServerError.WithReasonf("Unable to encode password options to JSON: %s", err))) + s.handleError(w, r, a.GetID(), i.Traits, errors.WithStack(herodot.ErrInternalServerError.WithReasonf("Unable to encode password options to JSON: %s", err))) return } @@ -451,7 +443,7 @@ func (s *Strategy) processRegistration(w http.ResponseWriter, r *http.Request, a }) if err := s.d.RegistrationExecutor().PostRegistrationHook(w, r, identity.CredentialsTypeOIDC, a, i); err != nil { - s.handleError(w, r, a.GetID(), traits, err) + s.handleError(w, r, a.GetID(), i.Traits, err) return } } @@ -533,7 +525,7 @@ func (s *Strategy) provider(id string) (Provider, error) { } } -func (s *Strategy) handleError(w http.ResponseWriter, r *http.Request, rid uuid.UUID, traits json.RawMessage, err error) { +func (s *Strategy) handleError(w http.ResponseWriter, r *http.Request, rid uuid.UUID, traits []byte, err error) { if x.IsZeroUUID(rid) { s.d.SelfServiceErrorManager().Forward(r.Context(), w, r, err) return diff --git a/selfservice/strategy/oidc/strategy_test.go b/selfservice/strategy/oidc/strategy_test.go index 28370ae6bb11..62ab35c2268b 100644 --- a/selfservice/strategy/oidc/strategy_test.go +++ b/selfservice/strategy/oidc/strategy_test.go @@ -126,7 +126,7 @@ func TestStrategy(t *testing.T) { ClientID: "client", ClientSecret: "secret", IssuerURL: remotePublic + "/", - SchemaURL: "file://./stub/hydra.schema.json", + Normalize: "file://./stub/oidc.hydra.jsonnet", }, { Provider: "generic", @@ -134,7 +134,7 @@ func TestStrategy(t *testing.T) { ClientID: "client", ClientSecret: "secret", IssuerURL: strings.Replace(remotePublic, "127.0.0.1", "localhost", 1) + "/", - SchemaURL: "file://./stub/hydra.schema.json", + Normalize: "file://./stub/oidc.hydra.jsonnet", }, }, }, diff --git a/selfservice/strategy/oidc/stub/hydra.schema.json b/selfservice/strategy/oidc/stub/hydra.schema.json deleted file mode 100644 index 6b293b0bd554..000000000000 --- a/selfservice/strategy/oidc/stub/hydra.schema.json +++ /dev/null @@ -1,25 +0,0 @@ -{ - "$id": "https://example.com/person.schema.json", - "$schema": "http://json-schema.org/draft-07/schema#", - "title": "Person", - "type": "object", - "properties": { - "sub": { - "type": "string", - "ory.sh/kratos": { - "mappings": { - "identity": { - "traits": [ - { - "path": "subject" - } - ] - } - } - } - } - }, - "required": [ - "sub" - ] -} diff --git a/selfservice/strategy/oidc/stub/oidc.hydra.jsonnet b/selfservice/strategy/oidc/stub/oidc.hydra.jsonnet new file mode 100644 index 000000000000..fa137de3334a --- /dev/null +++ b/selfservice/strategy/oidc/stub/oidc.hydra.jsonnet @@ -0,0 +1,11 @@ +local subject = std.toString(std.extVar('claims').sub); + +if std.length(subject) == 0 then error 'claim sub not set' + +{ + identity: { + traits: { + subject: subject, + }, + }, +}