feat: session devices upsert (#2733)

kelkarajay · Ajay Kelkar · commit ed1071afe9d0 · 2022-09-16T09:20:18.000+02:00
diff --git a/persistence/sql/migratest/testdata/20220907132836_testdata.sql b/persistence/sql/migratest/testdata/20220907132836_testdata.sql
@@ -5,7 +5,7 @@ VALUES ('7458af86-c1d8-401c-978a-8da89133f98b', '884f556e-eb3a-4b9f-bee3-1134564
         'eVwBt7UAAAAVwBt7XAMw', '5ff66179-c240-4703-b0d8-494592cefff5', true, '123eVwBt7UAAAeVwBt7XAMw', 'aal2',
         '[{"method":"password"},{"method":"totp"}]');
 
-INSERT INTO session_devices (id, nid, session_id, ip_address, user_agent, location, created_at, last_seen)
+INSERT INTO session_devices (id, nid, session_id, ip_address, user_agent, location, created_at, updated_at)
 VALUES ('884f556e-eb3a-4b9f-bee3-11763642c6c0', '884f556e-eb3a-4b9f-bee3-11345642c6c0',
         '7458af86-c1d8-401c-978a-8da89133f98b', '54.155.246.232',
         'Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/51.0.2704.103 Safari/537.36',
diff --git a/persistence/sql/migrations/sql/20220907132836000000_sessions_add_client_fields.cockroach.up.sql b/persistence/sql/migrations/sql/20220907132836000000_sessions_add_client_fields.cockroach.up.sql
@@ -8,7 +8,8 @@ CREATE TABLE "session_devices"
   "session_id" UUID      NOT NULL,
   "nid"        UUID      NOT NULL,
   "created_at" timestamp NOT NULL,
-  "last_seen"  timestamp NOT NULL,
+  "updated_at"  timestamp NOT NULL,
   CONSTRAINT "session_metadata_sessions_id_fk" FOREIGN KEY ("session_id") REFERENCES "sessions" ("id") ON DELETE cascade,
-  CONSTRAINT "session_metadata_nid_fk" FOREIGN KEY ("nid") REFERENCES "networks" ("id") ON DELETE cascade
+  CONSTRAINT "session_metadata_nid_fk" FOREIGN KEY ("nid") REFERENCES "networks" ("id") ON DELETE cascade,
+  CONSTRAINT unique_session_device UNIQUE (nid, session_id, ip_address, user_agent)
 );
diff --git a/persistence/sql/migrations/sql/20220907132836000000_sessions_add_client_fields.mysql.up.sql b/persistence/sql/migrations/sql/20220907132836000000_sessions_add_client_fields.mysql.up.sql
@@ -8,7 +8,8 @@ CREATE TABLE `session_devices`
   `session_id` char(36) NOT NULL,
   `nid`        char(36) NOT NULL,
   `created_at` DATETIME NOT NULL,
-  `last_seen`  DATETIME NOT NULL,
+  `updated_at`  DATETIME NOT NULL,
   FOREIGN KEY (`session_id`) REFERENCES `sessions` (`id`) ON DELETE cascade,
-  FOREIGN KEY (`nid`) REFERENCES `networks` (`id`) ON DELETE cascade
+  FOREIGN KEY (`nid`) REFERENCES `networks` (`id`) ON DELETE cascade,
+  CONSTRAINT unique_session_device UNIQUE (nid, session_id, ip_address, user_agent)
 ) ENGINE = InnoDB;
diff --git a/persistence/sql/migrations/sql/20220907132836000000_sessions_add_client_fields.postgres.up.sql b/persistence/sql/migrations/sql/20220907132836000000_sessions_add_client_fields.postgres.up.sql
@@ -8,7 +8,8 @@ CREATE TABLE "session_devices"
   "nid"        UUID      NOT NULL,
   "session_id" UUID      NOT NULL,
   "created_at" timestamp NOT NULL,
-  "last_seen"  timestamp NOT NULL,
+  "updated_at"  timestamp NOT NULL,
   FOREIGN KEY ("session_id") REFERENCES "sessions" ("id") ON DELETE cascade,
-  FOREIGN KEY ("nid") REFERENCES "networks" ("id") ON DELETE cascade
+  FOREIGN KEY ("nid") REFERENCES "networks" ("id") ON DELETE cascade,
+  CONSTRAINT unique_session_device UNIQUE (nid, session_id, ip_address, user_agent)
 );
diff --git a/persistence/sql/migrations/sql/20220907132836000000_sessions_add_client_fields.sqlite3.up.sql b/persistence/sql/migrations/sql/20220907132836000000_sessions_add_client_fields.sqlite3.up.sql
@@ -7,7 +7,8 @@ CREATE TABLE IF NOT EXISTS "session_devices"
   "session_id" UUID      NOT NULL,
   "nid"        UUID      NOT NULL,
   "created_at" timestamp NOT NULL,
-  "last_seen"  timestamp NOT NULL,
+  "updated_at"  timestamp NOT NULL,
   FOREIGN KEY ("session_id") REFERENCES "sessions" ("id") ON DELETE cascade,
-  FOREIGN KEY ("nid") REFERENCES "networks" ("id") ON DELETE cascade
+  FOREIGN KEY ("nid") REFERENCES "networks" ("id") ON DELETE cascade,
+  CONSTRAINT unique_session_device UNIQUE (nid, session_id, ip_address, user_agent)
 );
diff --git a/persistence/sql/persister_session.go b/persistence/sql/persister_session.go
@@ -24,8 +24,9 @@ func (p *Persister) GetSession(ctx context.Context, sid uuid.UUID) (*session.Ses
 	defer span.End()
 
 	var s session.Session
+	s.Devices = make([]session.Device, 0)
 	nid := p.NetworkID(ctx)
-	if err := p.GetConnection(ctx).Where("id = ? AND nid = ?", sid, nid).First(&s); err != nil {
+	if err := p.GetConnection(ctx).EagerPreload("Devices").Where("id = ? AND nid = ?", sid, nid).First(&s); err != nil {
 		return nil, sqlcon.HandleError(err)
 	}
 
@@ -36,29 +37,10 @@ func (p *Persister) GetSession(ctx context.Context, sid uuid.UUID) (*session.Ses
 		return nil, err
 	}
 
-	devices, err := p.GetSessionDevices(ctx, sid)
-	if err != nil {
-		return nil, err
-	}
-
 	s.Identity = i
-	s.Devices = devices
 	return &s, nil
 }
 
-func (p *Persister) GetSessionDevices(ctx context.Context, sid uuid.UUID) ([]session.Device, error) {
-	ctx, span := p.r.Tracer(ctx).Tracer().Start(ctx, "persistence.sql.GetSessionDevices")
-	defer span.End()
-
-	devices := make([]session.Device, 0)
-	nid := p.NetworkID(ctx)
-	if err := p.GetConnection(ctx).Where("session_id = ? AND nid = ?", sid, nid).All(&devices); err != nil {
-		return nil, sqlcon.HandleError(err)
-	}
-
-	return devices, nil
-}
-
 // ListSessionsByIdentity retrieves sessions for an identity from the store.
 func (p *Persister) ListSessionsByIdentity(ctx context.Context, iID uuid.UUID, active *bool, page, perPage int, except uuid.UUID) ([]*session.Session, error) {
 	ctx, span := p.r.Tracer(ctx).Tracer().Start(ctx, "persistence.sql.ListSessionsByIdentity")
@@ -75,7 +57,7 @@ func (p *Persister) ListSessionsByIdentity(ctx context.Context, iID uuid.UUID, a
 		if active != nil {
 			q = q.Where("active = ?", *active)
 		}
-		if err := q.All(&s); err != nil {
+		if err := q.Eager("Devices").All(&s); err != nil {
 			return sqlcon.HandleError(err)
 		}
 
@@ -85,13 +67,7 @@ func (p *Persister) ListSessionsByIdentity(ctx context.Context, iID uuid.UUID, a
 				return err
 			}
 
-			devices, err := p.GetSessionDevices(ctx, s.ID)
-			if err != nil {
-				return err
-			}
-
 			s.Identity = i
-			s.Devices = devices
 		}
 		return nil
 	}); err != nil {
@@ -107,15 +83,34 @@ func (p *Persister) UpsertSession(ctx context.Context, s *session.Session) error
 
 	s.NID = p.NetworkID(ctx)
 
-	if err := p.Connection(ctx).Find(new(session.Session), s.ID); errors.Is(err, sql.ErrNoRows) {
-		// This must not be eager or identities will be created / updated
-		return errors.WithStack(p.GetConnection(ctx).Create(s))
-	} else if err != nil {
-		return errors.WithStack(err)
-	}
+	return errors.WithStack(p.Transaction(ctx, func(ctx context.Context, tx *pop.Connection) error {
+		if err := tx.Find(new(session.Session), s.ID); errors.Is(err, sql.ErrNoRows) {
+			// This must not be eager or identities will be created / updated
+			err = errors.WithStack(tx.Create(s))
+			if err != nil {
+				return err
+			}
 
-	// This must not be eager or identities will be created / updated
-	return p.GetConnection(ctx).Update(s)
+			for i := range s.Devices {
+				device := &(s.Devices[i])
+				device.SessionID = s.ID
+				device.NID = s.NID
+
+				if err := tx.Create(device); err != nil {
+					return err
+				}
+
+				s.Devices[i] = *device
+			}
+
+			return nil
+		} else if err != nil {
+			return errors.WithStack(err)
+		}
+
+		// This must not be eager or identities will be created / updated
+		return tx.Update(s)
+	}))
 }
 
 func (p *Persister) DeleteSession(ctx context.Context, sid uuid.UUID) error {
diff --git a/session/persistence.go b/session/persistence.go
@@ -23,9 +23,6 @@ type Persister interface {
 	// GetSession retrieves a session from the store.
 	GetSession(ctx context.Context, sid uuid.UUID) (*Session, error)
 
-	// GetSessionDevices retrieves all the device session history from the store
-	GetSessionDevices(ctx context.Context, sid uuid.UUID) ([]Device, error)
-
 	// ListSessionsByIdentity retrieves sessions for an identity from the store.
 	ListSessionsByIdentity(ctx context.Context, iID uuid.UUID, active *bool, page, perPage int, except uuid.UUID) ([]*Session, error)
 
diff --git a/session/session.go b/session/session.go
@@ -57,7 +57,7 @@ type Device struct {
 	CreatedAt time.Time `json:"seen_at" faker:"-" db:"created_at"`
 
 	// Last seen
-	LastSeen time.Time `json:"last_seen" faker:"-" db:"last_seen"`
+	UpdatedAt time.Time `json:"last_seen" faker:"-" db:"updated_at"`
 
 	NID uuid.UUID `json:"-"  faker:"-" db:"nid"`
 }
@@ -119,7 +119,7 @@ type Session struct {
 	Identity *identity.Identity `json:"identity" faker:"identity" db:"-" belongs_to:"identities" fk_id:"IdentityID"`
 
 	// Devices has history of all endpoints where the session was used
-	Devices []Device `json:"devices" faker:"-" db:"-"`
+	Devices []Device `json:"devices" faker:"-" has_many:"session_devices" fk_id:"session_id"`
 
 	// IdentityID is a helper struct field for gobuffalo.pop.
 	IdentityID uuid.UUID `json:"-" faker:"-" db:"identity_id"`
@@ -231,8 +231,6 @@ func (s *Session) SaveSessionDeviceInformation(r *http.Request) {
 
 	device.ID = x.NewUUID()
 	device.SessionID = s.ID
-	device.CreatedAt = time.Now().UTC()
-	device.LastSeen = time.Now().UTC()
 
 	agent := r.Header["User-Agent"]
 	if len(agent) > 0 {
diff --git a/session/session_test.go b/session/session_test.go
@@ -99,7 +99,7 @@ func TestSession(t *testing.T) {
 		assert.Equal(t, authAt, s.AuthenticatedAt)
 		assert.Equal(t, 1, len(s.Devices))
 		assert.Equal(t, s.ID.String(), s.Devices[0].SessionID.String())
-		assert.NotNil(t, s.Devices[0].LastSeen)
+		assert.NotNil(t, s.Devices[0].UpdatedAt)
 		assert.NotNil(t, s.Devices[0].CreatedAt)
 		assert.Equal(t, "54.155.246.155", *s.Devices[0].IPAddress)
 		assert.Equal(t, "Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/51.0.2704.103 Safari/537.36", *s.Devices[0].UserAgent)
