From 83b591d8cf3d180ad9d48a72bd92ffdb3a8192ac Mon Sep 17 00:00:00 2001 From: arekkas Date: Fri, 15 Jun 2018 16:47:11 +0200 Subject: [PATCH] rules: Properly handle conflicts on PUT and POST Previously, PUT and POST did not result in errors (409) when non-existing resources were modified, or existing resources were created. This patch resolves that. Closes #38 --- cmd/rules_import.go | 9 +++--- helper/errors.go | 5 ++++ rule/manager_memory.go | 11 ++++++- rule/manager_sql.go | 65 ++++++++++++++++++++++++++++++++++++------ rule/manager_test.go | 4 +++ 5 files changed, 80 insertions(+), 14 deletions(-) diff --git a/cmd/rules_import.go b/cmd/rules_import.go index 699077cec9..9e4d908dd1 100644 --- a/cmd/rules_import.go +++ b/cmd/rules_import.go @@ -71,9 +71,10 @@ Usage example: for _, r := range rules { fmt.Printf("Importing rule %s...\n", r.ID) client := oathkeeper.NewSDK(endpoint) - out, response, err := client.GetRule(r.ID) - if err == nil { - response.Body.Close() + + shouldUpdate := false + if _, response, _ := client.GetRule(r.ID); err == nil && response.StatusCode == http.StatusOK { + shouldUpdate = true } rh := make([]swagger.RuleHandler, len(r.Authenticators)) @@ -104,7 +105,7 @@ Usage example: }, } - if out != nil { + if shouldUpdate { out, response, err := client.UpdateRule(r.ID, sr) checkResponse(response, err, http.StatusOK) fmt.Printf("Successfully imported rule %s...\n", out.Id) diff --git a/helper/errors.go b/helper/errors.go index 8ae825d218..dc9cab5ae7 100644 --- a/helper/errors.go +++ b/helper/errors.go @@ -52,4 +52,9 @@ var ( CodeField: http.StatusNotFound, StatusField: http.StatusText(http.StatusNotFound), } + ErrResourceConflict = &herodot.DefaultError{ + ErrorField: "The request could not be completed due to a conflict with the current state of the target resource", + CodeField: http.StatusConflict, + StatusField: http.StatusText(http.StatusConflict), + } ) diff --git a/rule/manager_memory.go b/rule/manager_memory.go index 2cb50b8de6..bfd2b769ec 100644 --- a/rule/manager_memory.go +++ b/rule/manager_memory.go @@ -54,10 +54,19 @@ func (m *MemoryManager) GetRule(id string) (*Rule, error) { } func (m *MemoryManager) CreateRule(rule *Rule) error { - return m.UpdateRule(rule) + if _, ok := m.Rules[rule.ID]; ok { + return errors.WithStack(helper.ErrResourceConflict) + } + + m.Rules[rule.ID] = *rule + return nil } func (m *MemoryManager) UpdateRule(rule *Rule) error { + if _, ok := m.Rules[rule.ID]; !ok { + return errors.WithStack(helper.ErrResourceConflict) + } + m.Rules[rule.ID] = *rule return nil } diff --git a/rule/manager_sql.go b/rule/manager_sql.go index e61270218a..d4251b77e2 100644 --- a/rule/manager_sql.go +++ b/rule/manager_sql.go @@ -107,6 +107,14 @@ func (s *SQLManager) CreateRule(rule *Rule) error { return errors.WithStack(err) } + if err := s.createRule(tx, rule); err != nil { + return err + } + + return commit(tx) +} + +func (s *SQLManager) createRule(tx *sqlx.Tx, rule *Rule) error { sr := toSqlRule(rule) if err := s.create(tx, "oathkeeper_rule", sqlParams, sr, "surrogate_id"); err != nil { return err @@ -120,13 +128,6 @@ func (s *SQLManager) CreateRule(rule *Rule) error { if err := s.createMany(tx, "oathkeeper_rule_credentials_issuer", sqlParamsHandler, sr.CredentialsIssuers, rule.ID); err != nil { return err } - - if err := tx.Commit(); err != nil { - if rErr := tx.Rollback(); rErr != nil { - return errors.Wrap(rErr, err.Error()) - } - return errors.WithStack(err) - } return nil } @@ -169,11 +170,57 @@ func (s *SQLManager) create(tx *sqlx.Tx, table string, params []string, value in } func (s *SQLManager) UpdateRule(rule *Rule) error { - return s.CreateRule(rule) + _, err := s.GetRule(rule.ID) + if errors.Cause(err) == helper.ErrResourceNotFound { + return errors.WithStack(helper.ErrResourceConflict) + } else if err != nil { + return err + } + + tx, err := s.db.Beginx() + if err != nil { + return errors.WithStack(err) + } + + if err := s.deleteRule(tx, rule.ID); err != nil { + return err + } + + if err := s.createRule(tx, rule); err != nil { + return err + } + + return commit(tx) +} + +func commit(tx *sqlx.Tx) error { + if err := tx.Commit(); err != nil { + if rErr := tx.Rollback(); rErr != nil { + return errors.Wrap(rErr, err.Error()) + } + return errors.WithStack(err) + } + return nil } func (s *SQLManager) DeleteRule(id string) error { - if _, err := s.db.Exec(s.db.Rebind(`DELETE FROM oathkeeper_rule WHERE surrogate_id=?`), id); err != nil { + tx, err := s.db.Beginx() + if err != nil { + return errors.WithStack(err) + } + + if err := s.deleteRule(tx, id); err != nil { + return err + } + + return commit(tx) +} + +func (s *SQLManager) deleteRule(tx *sqlx.Tx, id string) error { + if _, err := tx.Exec(s.db.Rebind(`DELETE FROM oathkeeper_rule WHERE surrogate_id=?`), id); err != nil { + if rErr := tx.Rollback(); rErr != nil { + return errors.Wrap(rErr, err.Error()) + } return errors.WithStack(err) } return nil diff --git a/rule/manager_test.go b/rule/manager_test.go index bfb17658b0..6d6c053ece 100644 --- a/rule/manager_test.go +++ b/rule/manager_test.go @@ -25,6 +25,7 @@ import ( _ "github.com/go-sql-driver/mysql" _ "github.com/lib/pq" + "github.com/ory/oathkeeper/helper" "github.com/ory/oathkeeper/pkg" "github.com/ory/sqlcon/dockertest" "github.com/stretchr/testify/assert" @@ -56,6 +57,9 @@ func TestManagers(t *testing.T) { _, err := manager.GetRule("1") require.Error(t, err) + // Updating of a non-existent rule should throw 409 + require.EqualError(t, manager.UpdateRule(&r3), helper.ErrResourceConflict.Error()) + require.NoError(t, manager.CreateRule(&r1)) require.NoError(t, manager.CreateRule(&r2)) require.NoError(t, manager.CreateRule(&r3))