From 685b45582a3d8d6af327f741e7e62611fe3cf408 Mon Sep 17 00:00:00 2001 From: alexGNX <46828169+alexGNX@users.noreply.github.com> Date: Mon, 9 May 2022 16:45:08 +0000 Subject: [PATCH] feat(saml): fix unit tests + lints --- cmd/remote/status.go | 3 +++ cmd/remote/version.go | 3 +++ selfservice/flow/saml/handler.go | 3 +++ .../flow/saml/helpertest/helpertest.go | 19 +++++++++++++++---- .../strategy/saml/strategy/strategy_login.go | 4 +--- .../saml/strategy/strategy_registration.go | 2 +- .../saml/strategy/test/strategy_test.go | 8 +++++--- selfservice/strategy/saml/strategy/types.go | 2 +- 8 files changed, 32 insertions(+), 12 deletions(-) diff --git a/cmd/remote/status.go b/cmd/remote/status.go index 73fb402ac3f6..9f6de31f9d49 100644 --- a/cmd/remote/status.go +++ b/cmd/remote/status.go @@ -41,6 +41,9 @@ var statusCmd = &cobra.Command{ Args: cobra.NoArgs, RunE: func(cmd *cobra.Command, args []string) error { c, err := cliclient.NewClient(cmd) + if err != nil { + return err + } state := &statusState{} defer cmdx.PrintRow(cmd, state) diff --git a/cmd/remote/version.go b/cmd/remote/version.go index ba5115ceeec5..b8de4856eb85 100644 --- a/cmd/remote/version.go +++ b/cmd/remote/version.go @@ -29,6 +29,9 @@ var versionCmd = &cobra.Command{ Args: cobra.NoArgs, RunE: func(cmd *cobra.Command, args []string) error { c, err := cliclient.NewClient(cmd) + if err != nil { + return err + } resp, _, err := c.MetadataApi.GetVersion(cmd.Context()).Execute() if err != nil { diff --git a/selfservice/flow/saml/handler.go b/selfservice/flow/saml/handler.go index d78fb387466a..7c85340ae484 100644 --- a/selfservice/flow/saml/handler.go +++ b/selfservice/flow/saml/handler.go @@ -273,6 +273,9 @@ func (h *Handler) instantiateMiddleware(config config.Config) error { // Crewjam library use default route for ACS and metadat but we want to overwrite them metadata, err := url.Parse(publicUrlString + RouteSamlMetadata) + if err != nil { + return err + } samlMiddleWare.ServiceProvider.MetadataURL = *metadata // The EntityID in the AuthnRequest is the Metadata URL diff --git a/selfservice/flow/saml/helpertest/helpertest.go b/selfservice/flow/saml/helpertest/helpertest.go index 1def12d4c3f6..e07e9474e68a 100644 --- a/selfservice/flow/saml/helpertest/helpertest.go +++ b/selfservice/flow/saml/helpertest/helpertest.go @@ -182,21 +182,32 @@ func InitMiddlewareWithoutMetadata(t *testing.T, idpSsoUrl string, idpEntityId s return InitMiddleware(t, idpInformation) } -func GetAndDecryptAssertion(t *testing.T, samlResponseFile string, key *rsa.PrivateKey) (*crewjamsaml.Assertion, error) { +func GetAndDecryptAssertion(samlResponseFile string, key *rsa.PrivateKey) (*crewjamsaml.Assertion, error) { // Load saml response test file samlResponse, err := ioutil.ReadFile(samlResponseFile) + if err != nil { + return nil, err + } // Decrypt saml response assertion doc := etree.NewDocument() err = doc.ReadFromBytes(samlResponse) - require.NoError(t, err) + if err != nil { + return nil, err + } + responseEl := doc.Root() el := responseEl.FindElement("//EncryptedAssertion/EncryptedData") plaintextAssertion, err := xmlenc.Decrypt(key, el) + if err != nil { + return nil, err + } assertion := &crewjamsaml.Assertion{} err = xml.Unmarshal(plaintextAssertion, assertion) - require.NoError(t, err) + if err != nil { + return nil, err + } - return assertion, err + return assertion, nil } diff --git a/selfservice/strategy/saml/strategy/strategy_login.go b/selfservice/strategy/saml/strategy/strategy_login.go index 82537965910f..9ebce44bfb69 100644 --- a/selfservice/strategy/saml/strategy/strategy_login.go +++ b/selfservice/strategy/saml/strategy/strategy_login.go @@ -44,12 +44,10 @@ func (s *Strategy) processLogin(w http.ResponseWriter, r *http.Request, a *login return nil, nil } -// Method not used but necessary to implement the interface func (s *Strategy) Login(w http.ResponseWriter, r *http.Request, f *login.Flow, ss *session.Session) (i *identity.Identity, err error) { - return nil, nil + return nil, flow.ErrStrategyNotResponsible } -// Method not used but necessary to implement the interface func (s *Strategy) PopulateLoginMethod(r *http.Request, requestedAAL identity.AuthenticatorAssuranceLevel, l *login.Flow) error { if l.Type != flow.TypeBrowser { return nil diff --git a/selfservice/strategy/saml/strategy/strategy_registration.go b/selfservice/strategy/saml/strategy/strategy_registration.go index 0ca9534e93ea..bf0b446b786a 100644 --- a/selfservice/strategy/saml/strategy/strategy_registration.go +++ b/selfservice/strategy/saml/strategy/strategy_registration.go @@ -87,7 +87,7 @@ func (s *Strategy) GetRegistrationIdentity(r *http.Request, ctx context.Context, return i, err } - // Create new uniq credentials identifier for user is database + // Create new unique credentials identifier for user is database creds, err := NewCredentialsForSAML(claims.Subject, provider.Config().ID) if err != nil { return i, err diff --git a/selfservice/strategy/saml/strategy/test/strategy_test.go b/selfservice/strategy/saml/strategy/test/strategy_test.go index 88d4d8931fe1..c56340ea5536 100644 --- a/selfservice/strategy/saml/strategy/test/strategy_test.go +++ b/selfservice/strategy/saml/strategy/test/strategy_test.go @@ -28,7 +28,7 @@ func TestGetAndDecryptAssertion(t *testing.T) { middleware, _, _, _ := helpertest.InitMiddlewareWithMetadata(t, "file://testdata/idp_saml_metadata.xml") - assertion, err := helpertest.GetAndDecryptAssertion(t, "./testdata/SP_SamlResponse.xml", middleware.ServiceProvider.Key) + assertion, err := helpertest.GetAndDecryptAssertion("./testdata/SP_SamlResponse.xml", middleware.ServiceProvider.Key) require.NoError(t, err) assert.Check(t, assertion != nil) @@ -44,7 +44,8 @@ func TestGetAttributesFromAssertion(t *testing.T) { middleware, strategy, _, _ := helpertest.InitMiddlewareWithMetadata(t, "file://testdata/idp_saml_metadata.xml") - assertion, _ := helpertest.GetAndDecryptAssertion(t, "./testdata/SP_SamlResponse.xml", middleware.ServiceProvider.Key) + assertion, err := helpertest.GetAndDecryptAssertion("./testdata/SP_SamlResponse.xml", middleware.ServiceProvider.Key) + require.NoError(t, err) mapAttributes, err := strategy.GetAttributesFromAssertion(assertion) @@ -181,7 +182,8 @@ func TestGetRegistrationIdentity(t *testing.T) { "file://testdata/idp_saml_metadata.xml") provider, _ := strategy.Provider(context.Background()) - assertion, _ := helpertest.GetAndDecryptAssertion(t, "./testdata/SP_SamlResponse.xml", middleware.ServiceProvider.Key) + assertion, err := helpertest.GetAndDecryptAssertion("./testdata/SP_SamlResponse.xml", middleware.ServiceProvider.Key) + require.NoError(t, err) attributes, _ := strategy.GetAttributesFromAssertion(assertion) claims, _ := provider.Claims(context.Background(), strategy.D().Config(context.Background()), attributes) diff --git a/selfservice/strategy/saml/strategy/types.go b/selfservice/strategy/saml/strategy/types.go index 3c8ad11cca2d..d4e9fa47e270 100644 --- a/selfservice/strategy/saml/strategy/types.go +++ b/selfservice/strategy/saml/strategy/types.go @@ -17,7 +17,7 @@ type CredentialsConfig struct { Providers []ProviderCredentialsConfig `json:"providers"` } -//Create an uniq identifier for user in database. Its look like "id + the id of the saml provider" +//Create a unique identifier for user in database. Its look like "id + the id of the saml provider" func NewCredentialsForSAML(subject string, provider string) (*identity.Credentials, error) { var b bytes.Buffer if err := json.NewEncoder(&b).Encode(CredentialsConfig{