Fix security issue with 2fa in recover module

Author: aarondl

CHANGELOG.md

@@ -3,6 +3,25 @@
 The format is based on [Keep a Changelog](http://keepachangelog.com/en/1.0.0/)
 and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.html).
 
+## [3.2.1] - 2022-10-10
+
+### Security
+
+- Change it such that events are fired around recovery. Some important events
+  were not occurring despite being logged in that would manage state like
+  remember cookies, locking state, etc.
+
+  A significant side effect of this is that when 2fa is currently turned on on
+  an account, it could be bypassed by using the Recover functionality with the
+  `RecoverLoginAfterRecovery` feature which was not otherwise flagged as a
+  dangerous option.
+
+  While it is true that recover uses an email to invoke the flow and therefore
+  some second factor has been utilizied, it should be considered insecure to
+  bypass an authenticator or sms verification for any login and therefore
+  this large change in behavior is being shipped as a security fix (meaning it
+  becomes a minor change).
+
 ## [3.2.0] - 2021-08-11
 
 ### Added

recover/recover.go

@@ -278,14 +278,35 @@ func (r *Recover) EndPost(w http.ResponseWriter, req *http.Request) error {
 	}
 
 	successMsg := "Successfully updated password"
+	_, err = r.Authboss.Events.FireAfter(authboss.EventRecoverEnd, w, req)
+	if err != nil {
+		return err
+	}
+
 	if r.Authboss.Config.Modules.RecoverLoginAfterRecovery {
+		handled, err = r.Events.FireBefore(authboss.EventAuth, w, req)
+		if err != nil {
+			return err
+		} else if handled {
+			return nil
+		}
+
+		handled, err = r.Events.FireBefore(authboss.EventAuthHijack, w, req)
+		if err != nil {
+			return err
+		} else if handled {
+			return nil
+		}
+
 		authboss.PutSession(w, authboss.SessionKey, user.GetPID())
 		successMsg += " and logged in"
-	}
 
-	_, err = r.Authboss.Events.FireAfter(authboss.EventRecoverEnd, w, req)
-	if err != nil {
-		return err
+		handled, err = r.Authboss.Events.FireAfter(authboss.EventAuth, w, req)
+		if err != nil {
+			return err
+		} else if handled {
+			return nil
+		}
 	}
 
 	ro := authboss.RedirectOptions{

recover/recover_test.go

@@ -276,6 +276,36 @@ func TestEndPostSuccessLogin(t *testing.T) {
 		RecoverTokenExpiry: time.Now().UTC().AddDate(0, 0, 1),
 	}
 
+	callbacks := 0
+	h.ab.Events.After(authboss.EventRecoverEnd, func(w http.ResponseWriter, r *http.Request, handled bool) (bool, error) {
+		if callbacks != 0 {
+			t.Error("after recover end was not called 1st")
+		}
+		callbacks += 1
+		return false, nil
+	})
+	h.ab.Events.Before(authboss.EventAuth, func(w http.ResponseWriter, r *http.Request, handled bool) (bool, error) {
+		if callbacks != 1 {
+			t.Error("before auth was not called 2nd")
+		}
+		callbacks += 1
+		return false, nil
+	})
+	h.ab.Events.Before(authboss.EventAuthHijack, func(w http.ResponseWriter, r *http.Request, handled bool) (bool, error) {
+		if callbacks != 2 {
+			t.Error("before auth hijack was not called 3rd")
+		}
+		callbacks += 1
+		return false, nil
+	})
+	h.ab.Events.After(authboss.EventAuth, func(w http.ResponseWriter, r *http.Request, handled bool) (bool, error) {
+		if callbacks != 3 {
+			t.Error("after auth was not called 4th")
+		}
+		callbacks += 1
+		return false, nil
+	})
+
 	r := mocks.Request("POST")
 	w := httptest.NewRecorder()
 
@@ -295,6 +325,9 @@ func TestEndPostSuccessLogin(t *testing.T) {
 	if !strings.Contains(h.redirector.Options.Success, "logged in") {
 		t.Error("should talk about logging in")
 	}
+	if callbacks != 4 {
+		t.Error("all 4 callbacks were not called")
+	}
 }
 
 func TestEndPostValidationFailure(t *testing.T) {