o/i/apparmorprompting: fix bug in notice backend when re-recording notice

When re-recording a notice which was expired at the time, there was a
bug where the new notice would be added to the ID map, then all
previously-expired notice IDs were removed from the map, so the
newly-added ID would be removed as well. Fix that bug and add a test for
it.

Additionally, return early if there are no notices to be added, and
remove unnecessary debug log.

Signed-off-by: Oliver Calder <oliver.calder@canonical.com>

Author: olivercalder

overlord/ifacestate/apparmorprompting/noticebackend.go

@@ -208,7 +208,9 @@ type addNoticesInfo struct {
 // The notice key equals the info's prompt/rule ID, and the notice ID and type
 // are derived from the receiver.
 func (ntb *noticeTypeBackend) addNotices(infos []addNoticesInfo) error {
-	logger.Debugf("called addNotices() with %d notice infos", len(infos))
+	if len(infos) == 0 {
+		return nil
+	}
 	ntb.rwmu.Lock()
 	defer ntb.rwmu.Unlock()
 
@@ -319,13 +321,17 @@ func (ntb *noticeTypeBackend) doAddNotice(userID uint32, id prompting.IDType, da
 	newUserNotices := appendNotice(userNotices, newNotice, existingIndex, expiredCount)
 
 	ntb.userNotices[userID] = newUserNotices
-	ntb.idToNotice[noticeID] = newNotice
 
+	// Delete expired notices from the ID map before setting the new notice,
+	// in case an expired notice shares the same ID as the new notice (i.e.
+	// when re-recording an expired notice).
 	expiredNotices := userNotices[:expiredCount]
 	for _, expiredNotice := range expiredNotices {
 		delete(ntb.idToNotice, expiredNotice.ID())
 	}
 
+	ntb.idToNotice[noticeID] = newNotice
+
 	rollback = func() {
 		ntb.userNotices[userID] = userNotices
 		if existingNotice != nil {

overlord/ifacestate/apparmorprompting/noticebackend_test.go

@@ -409,6 +409,12 @@ func (s *noticebackendSuite) TestAddNoticeAllExpired(c *C) {
 		// Check that the nesly (re-)recorded notice is the only one
 		c.Check(notices, HasLen, 1, Commentf("trying to add notice %s", id))
 		c.Check(notices[0].Key(), Equals, id.String())
+
+		// Check that the re-recorded notice is retrievable via BackendNotice,
+		// which uses the ID map. This catches a bug where re-recording an
+		// expired notice could leave it absent from the ID map.
+		noticeID := "prompt-" + id.String()
+		c.Check(promptBackend.BackendNotice(noticeID), NotNil, Commentf("could not find re-recorded notice with ID %s via BackendNotice", id))
 	}
 }
 
@@ -540,6 +546,15 @@ func (s *noticebackendSuite) TestAddNotices(c *C) {
 			c.Check(afterNotices[i].LastData(), HasLen, 0)
 		}
 	}
+
+	// Check that all notices, including re-recorded expired ones, are
+	// retrievable via BackendNotice (which uses the ID map). This catches
+	// a bug where re-recording an expired notice could leave it absent from
+	// the ID map if expired notice cleanup ran after setting the new notice.
+	for _, id := range []uint32{1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11} {
+		noticeID := fmt.Sprintf("rule-%016X", id)
+		c.Check(ruleBackend.BackendNotice(noticeID), NotNil, Commentf("could not find notice with ID %d via BackendNotice", id))
+	}
 }
 
 func (s *noticebackendSuite) TestAddNoticesErrors(c *C) {