i/p/requestprompts: add timeout for prompts (#14390)

* i/p/requestprompts: add timeout for prompts When there has been no retrieval of prompt details for a given user after a particular duration, expire all outstanding prompts for that user, as this suggests that there is no prompt client running for that user. If a user retrieves prompt details or interacts with prompts in some way, such as by retrieving all prompts, a prompt by ID, or attempting to reply to a prompt, bump the timeout to a much longer timeout, since this indicates that a prompt client is running for that user. Signed-off-by: Oliver Calder <oliver.calder@canonical.com> * i/p/requestprompts: add helper to send reply for prompt Signed-off-by: Oliver Calder <oliver.calder@canonical.com> * i/p/requestprompts: do not expire prompts on access race Signed-off-by: Oliver Calder <oliver.calder@canonical.com> * i/p/requestprompts: avoid data races around close and expiration When `Close` is called, explicitly stop the expiration timers, and when the expiration timer fires and acquires the DB lock, return early if the DB has already been closed. Signed-off-by: Oliver Calder <oliver.calder@canonical.com> * i/p/requestprompts: leave prompts unchanged if timeout race happens Signed-off-by: Oliver Calder <oliver.calder@canonical.com> * i/p/requestprompts: use testutil.Mock instead of testutil.Backup Signed-off-by: Oliver Calder <oliver.calder@canonical.com> * i/p/requestprompts: reorder user entry creation and map insertion Signed-off-by: Oliver Calder <oliver.calder@canonical.com> * i/p/requestprompts: move timeout callback into dedicated method Signed-off-by: Oliver Calder <oliver.calder@canonical.com> * daemon,i/p/requestprompts,o/i/apparmorprompting: explicitly indicate client activity Callers of the requestprompts methods can specify whether they indicate that a prompting client is present and active, and thus the prompt expiration timeout should be reset. Previously, any call to these methods was implicitly treated as activity. Now, these functions have the ability to be called internally or while testing or debugging in a way which does not cause the timeout to be reset. Signed-off-by: Oliver Calder <oliver.calder@canonical.com> * daemon,i/p/requestprompts,o/i/apparmorprompting: add client activity fields for adding/patching rules Signed-off-by: Oliver Calder <oliver.calder@canonical.com> * daemon: add prompting client activity helper and improve comments Signed-off-by: Oliver Calder <oliver.calder@canonical.com> * i/p/requestprompts: make internal errors noticef instead of debugf Signed-off-by: Oliver Calder <oliver.calder@canonical.com> * i/p/requestprompts: add helper method to reset expiration timer Signed-off-by: Oliver Calder <oliver.calder@canonical.com> * many: remove clientActivity plumbing for rule-related functions Signed-off-by: Oliver Calder <oliver.calder@canonical.com> * i/p/requestprompts: unlock prompt db on early return from timeout callback Signed-off-by: Oliver Calder <oliver.calder@canonical.com> * i/p/requestprompts: use testtime timer interface and test timer Signed-off-by: Oliver Calder <oliver.calder@canonical.com> * i/p/requestprompts: fix typo and simplify mockable function Signed-off-by: Oliver Calder <oliver.calder@canonical.com> * i/p/requestprompts: simplify reset helper and clarify comment Signed-off-by: Oliver Calder <oliver.calder@canonical.com> * i/p/requestprompts: remove racy timer stop during close and clarify behavior The `Close` method calling `Stop` on the expiration timers races with those timers firing, and it's unnecessary since the the callback function checks whether the DB has been closed and returns early if it has. Thus, remove the explicit `Stop` and add some comments about the reasoning behind this. Additionally, add a `isClosed` helper so functions don't have to directly call a method on the contained `maxIDMmap` to check whether the prompt DB has been closed. Signed-off-by: Oliver Calder <oliver.calder@canonical.com> * i/p/requestprompts: improve timeout arithmetic in tests Signed-off-by: Oliver Calder <oliver.calder@canonical.com> * i/p/requestprompts: add comment to use clear() once on Go 1.21+ Signed-off-by: Oliver Calder <oliver.calder@canonical.com> --------- Signed-off-by: Oliver Calder <oliver.calder@canonical.com>
canonical · Dec 4, 2024 · e4a90bb · e4a90bb
1 parent 11f4060
commit e4a90bb
Show file tree

Hide file tree

Showing 7 changed files with 642 additions and 142 deletions.
diff --git a/daemon/api_prompting.go b/daemon/api_prompting.go
@@ -100,6 +100,13 @@ func getUserID(r *http.Request) (uint32, Response) {
 	return uint32(userIDInt), nil
 }
 
+// isClientActivity returns true if the request comes a prompting handler
+// service.
+func isClientActivity(c *Command, r *http.Request) bool {
+	// TODO: check that it's a handler service client making the API request
+	return true
+}
+
 type invalidReason string
 
 const (
@@ -331,7 +338,9 @@ func getPrompts(c *Command, r *http.Request, user *auth.UserState) Response {
 		return promptingNotRunningError()
 	}
 
-	prompts, err := getInterfaceManager(c).InterfacesRequestsManager().Prompts(userID)
+	clientActivity := isClientActivity(c, r)
+
+	prompts, err := getInterfaceManager(c).InterfacesRequestsManager().Prompts(userID, clientActivity)
 	if err != nil {
 		return promptingError(err)
 	}
@@ -360,7 +369,9 @@ func getPrompt(c *Command, r *http.Request, user *auth.UserState) Response {
 		return promptingNotRunningError()
 	}
 
-	prompt, err := getInterfaceManager(c).InterfacesRequestsManager().PromptWithID(userID, promptID)
+	clientActivity := isClientActivity(c, r)
+
+	prompt, err := getInterfaceManager(c).InterfacesRequestsManager().PromptWithID(userID, promptID, clientActivity)
 	if err != nil {
 		return promptingError(err)
 	}
@@ -392,7 +403,9 @@ func postPrompt(c *Command, r *http.Request, user *auth.UserState) Response {
 		return promptingError(fmt.Errorf("cannot decode request body into prompt reply: %w", err))
 	}
 
-	satisfiedPromptIDs, err := getInterfaceManager(c).InterfacesRequestsManager().HandleReply(userID, promptID, reply.Constraints, reply.Outcome, reply.Lifespan, reply.Duration)
+	clientActivity := isClientActivity(c, r)
+
+	satisfiedPromptIDs, err := getInterfaceManager(c).InterfacesRequestsManager().HandleReply(userID, promptID, reply.Constraints, reply.Outcome, reply.Lifespan, reply.Duration, clientActivity)
 	if err != nil {
 		return promptingError(err)
 	}

diff --git a/daemon/api_prompting_test.go b/daemon/api_prompting_test.go
@@ -52,34 +52,38 @@ type fakeInterfacesRequestsManager struct {
 	err          error
 
 	// Store most recent received values
-	userID      uint32
-	snap        string
-	iface       string
-	id          prompting.IDType // used for prompt ID or rule ID
-	constraints *prompting.Constraints
-	outcome     prompting.OutcomeType
-	lifespan    prompting.LifespanType
-	duration    string
+	userID         uint32
+	snap           string
+	iface          string
+	id             prompting.IDType // used for prompt ID or rule ID
+	constraints    *prompting.Constraints
+	outcome        prompting.OutcomeType
+	lifespan       prompting.LifespanType
+	duration       string
+	clientActivity bool
 }
 
-func (m *fakeInterfacesRequestsManager) Prompts(userID uint32) ([]*requestprompts.Prompt, error) {
+func (m *fakeInterfacesRequestsManager) Prompts(userID uint32, clientActivity bool) ([]*requestprompts.Prompt, error) {
 	m.userID = userID
+	m.clientActivity = clientActivity
 	return m.prompts, m.err
 }
 
-func (m *fakeInterfacesRequestsManager) PromptWithID(userID uint32, promptID prompting.IDType) (*requestprompts.Prompt, error) {
+func (m *fakeInterfacesRequestsManager) PromptWithID(userID uint32, promptID prompting.IDType, clientActivity bool) (*requestprompts.Prompt, error) {
 	m.userID = userID
 	m.id = promptID
+	m.clientActivity = clientActivity
 	return m.prompt, m.err
 }
 
-func (m *fakeInterfacesRequestsManager) HandleReply(userID uint32, promptID prompting.IDType, constraints *prompting.Constraints, outcome prompting.OutcomeType, lifespan prompting.LifespanType, duration string) ([]prompting.IDType, error) {
+func (m *fakeInterfacesRequestsManager) HandleReply(userID uint32, promptID prompting.IDType, constraints *prompting.Constraints, outcome prompting.OutcomeType, lifespan prompting.LifespanType, duration string, clientActivity bool) ([]prompting.IDType, error) {
 	m.userID = userID
 	m.id = promptID
 	m.constraints = constraints
 	m.outcome = outcome
 	m.lifespan = lifespan
 	m.duration = duration
+	m.clientActivity = clientActivity
 	return m.satisfiedIDs, m.err
 }
 
@@ -651,6 +655,7 @@ func (s *promptingSuite) TestGetPromptsHappy(c *C) {
 
 	// Check parameters
 	c.Check(s.manager.userID, Equals, uint32(1000))
+	c.Check(s.manager.clientActivity, Equals, true)
 
 	// Check return value
 	prompts, ok := rsp.Result.([]*requestprompts.Prompt)
@@ -681,6 +686,7 @@ func (s *promptingSuite) TestGetPromptHappy(c *C) {
 	// Check parameters
 	c.Check(s.manager.userID, Equals, uint32(1000))
 	c.Check(s.manager.id, Equals, prompting.IDType(0x0123456789abcdef))
+	c.Check(s.manager.clientActivity, Equals, true)
 
 	// Check return value
 	prompt, ok := rsp.Result.(*requestprompts.Prompt)
@@ -722,6 +728,7 @@ func (s *promptingSuite) TestPostPromptHappy(c *C) {
 	c.Check(s.manager.outcome, Equals, contents.Outcome)
 	c.Check(s.manager.lifespan, Equals, contents.Lifespan)
 	c.Check(s.manager.duration, Equals, contents.Duration)
+	c.Check(s.manager.clientActivity, Equals, true)
 
 	// Check return value
 	satisfiedIDs, ok := rsp.Result.([]prompting.IDType)

diff --git a/interfaces/prompting/requestprompts/export_test.go b/interfaces/prompting/requestprompts/export_test.go
@@ -23,9 +23,15 @@ import (
 	"time"
 
 	"github.com/snapcore/snapd/interfaces/prompting"
+	"github.com/snapcore/snapd/testutil"
+	"github.com/snapcore/snapd/timeutil"
 )
 
-const MaxOutstandingPromptsPerUser = maxOutstandingPromptsPerUser
+const (
+	InitialTimeout               = initialTimeout
+	ActivityTimeout              = activityTimeout
+	MaxOutstandingPromptsPerUser = maxOutstandingPromptsPerUser
+)
 
 func NewPrompt(id prompting.IDType, timestamp time.Time, snap string, iface string, path string, remainingPermissions []string, availablePermissions []string, originalPermissions []string) *Prompt {
 	constraints := &promptConstraints{
@@ -51,3 +57,7 @@ func (pdb *PromptDB) PerUser() map[uint32]*userPromptDB {
 func (pdb *PromptDB) NextID() (prompting.IDType, error) {
 	return pdb.maxIDMmap.NextID()
 }
+
+func MockTimeAfterFunc(f func(d time.Duration, callback func()) timeutil.Timer) (restore func()) {
+	return testutil.Mock(&timeAfterFunc, f)
+}