-
Notifications
You must be signed in to change notification settings - Fork 601
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
i/prompting: add package for prompting common types/functions #13849
Changes from 11 commits
dbddf6b
6a71217
74cc30b
a428025
29920e0
0f9b8d2
05030d3
617e192
a9031f6
01d2a87
4adc766
4fe163d
ccca57a
6a8609a
9875909
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
@@ -0,0 +1,140 @@ | ||||||
// -*- Mode: Go; indent-tabs-mode: t -*- | ||||||
|
||||||
/* | ||||||
* Copyright (C) 2024 Canonical Ltd | ||||||
* | ||||||
* This program is free software: you can redistribute it and/or modify | ||||||
* it under the terms of the GNU General Public License version 3 as | ||||||
* published by the Free Software Foundation. | ||||||
* | ||||||
* This program is distributed in the hope that it will be useful, | ||||||
* but WITHOUT ANY WARRANTY; without even the implied warranty of | ||||||
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the | ||||||
* GNU General Public License for more details. | ||||||
* | ||||||
* You should have received a copy of the GNU General Public License | ||||||
* along with this program. If not, see <http://www.gnu.org/licenses/>. | ||||||
* | ||||||
*/ | ||||||
|
||||||
package prompting | ||||||
|
||||||
import ( | ||||||
"encoding/base32" | ||||||
"encoding/binary" | ||||||
"fmt" | ||||||
"time" | ||||||
) | ||||||
|
||||||
type OutcomeType string | ||||||
|
||||||
const ( | ||||||
OutcomeUnset OutcomeType = "" | ||||||
OutcomeAllow OutcomeType = "allow" | ||||||
OutcomeDeny OutcomeType = "deny" | ||||||
) | ||||||
|
||||||
// IsAllow returns true if the outcome is OutcomeAllow, false if the outcome is | ||||||
// OutcomeDeny, or an error if it cannot be parsed. | ||||||
func (outcome OutcomeType) IsAllow() (bool, error) { | ||||||
switch outcome { | ||||||
case OutcomeAllow: | ||||||
return true, nil | ||||||
case OutcomeDeny: | ||||||
return false, nil | ||||||
default: | ||||||
return false, fmt.Errorf(`outcome must be %q or %q: %q`, OutcomeAllow, OutcomeDeny, outcome) | ||||||
} | ||||||
} | ||||||
|
||||||
// ValidateOutcome returns nil if the given outcome is valid, otherwise an error. | ||||||
func ValidateOutcome(outcome OutcomeType) error { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this is a bit strange, shouldn't it be a parse function taking a string and returning the OutcomeType or an error? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. or maybe what you need is MarshalJSON UnmarshalJSON methods? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hm if we don't care about defining new values of OutcomeType manually, then I think a trivial UnmarshalJSON would be preferred. You do not need MarshalJSON in such case. |
||||||
switch outcome { | ||||||
case OutcomeAllow, OutcomeDeny: | ||||||
return nil | ||||||
default: | ||||||
return fmt.Errorf(`outcome must be %q or %q: %q`, OutcomeAllow, OutcomeDeny, outcome) | ||||||
} | ||||||
} | ||||||
|
||||||
type LifespanType string | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. same |
||||||
|
||||||
const ( | ||||||
LifespanUnset LifespanType = "" | ||||||
LifespanForever LifespanType = "forever" | ||||||
LifespanSingle LifespanType = "single" | ||||||
LifespanTimespan LifespanType = "timespan" | ||||||
bboozzoo marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
// TODO: add LifespanSession which expires after the user logs out | ||||||
// LifespanSession LifespanType = "session" | ||||||
) | ||||||
|
||||||
// ValidateLifespanExpiration checks that the given lifespan is valid and that | ||||||
// the given expiration is valid for that lifespan. | ||||||
// | ||||||
// If the lifespan is LifespanTimespan LifespanTimespan, then expiration must | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
// be non-zero and be after the given currTime. Otherwise, it must be zero. | ||||||
// Returns an error if any of the above are invalid. | ||||||
func ValidateLifespanExpiration(lifespan LifespanType, expiration time.Time, currTime time.Time) error { | ||||||
switch lifespan { | ||||||
case LifespanForever, LifespanSingle: | ||||||
if !expiration.IsZero() { | ||||||
return fmt.Errorf(`expiration must be omitted when lifespan is %q, but received non-zero expiration: %q`, lifespan, expiration) | ||||||
} | ||||||
case LifespanTimespan: | ||||||
if expiration.IsZero() { | ||||||
return fmt.Errorf(`expiration must be non-zero when lifespan is %q, but received empty expiration`, lifespan) | ||||||
} | ||||||
if currTime.After(expiration) { | ||||||
return fmt.Errorf("expiration time has already passed: %q", expiration) | ||||||
} | ||||||
default: | ||||||
return fmt.Errorf(`invalid lifespan: %q`, lifespan) | ||||||
} | ||||||
return nil | ||||||
} | ||||||
|
||||||
// ValidateLifespanParseDuration checks that the given lifespan is valid and | ||||||
// that the given duration is valid for that lifespan. | ||||||
// | ||||||
// If the lifespan is LifespanTimespan, then duration must be a string parsable | ||||||
// by time.ParseDuration(), representing the duration of time for which the rule | ||||||
// should be valid. Otherwise, it must be empty. Returns an error if any of the | ||||||
// above are invalid, otherwise computes the expiration time of the rule based | ||||||
// on the current time and the given duration and returns it. | ||||||
func ValidateLifespanParseDuration(lifespan LifespanType, duration string) (time.Time, error) { | ||||||
var expiration time.Time | ||||||
switch lifespan { | ||||||
case LifespanForever, LifespanSingle: | ||||||
if duration != "" { | ||||||
return expiration, fmt.Errorf(`duration must be empty when lifespan is %q, but received non-empty duration: %q`, lifespan, duration) | ||||||
} | ||||||
case LifespanTimespan: | ||||||
if duration == "" { | ||||||
return expiration, fmt.Errorf(`duration must be non-empty when lifespan is %q, but received empty expiration`, lifespan) | ||||||
} | ||||||
parsedDuration, err := time.ParseDuration(duration) | ||||||
if err != nil { | ||||||
return expiration, fmt.Errorf(`error parsing duration string: %q`, duration) | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. FWIW, errors usually follow the pattern of
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. that's correct, errors should try to be of the form "cannot ..." whenever possible |
||||||
} | ||||||
if parsedDuration <= 0 { | ||||||
return expiration, fmt.Errorf(`duration must be greater than zero: %q`, duration) | ||||||
} | ||||||
expiration = time.Now().Add(parsedDuration) | ||||||
default: | ||||||
return expiration, fmt.Errorf(`invalid lifespan: %q`, lifespan) | ||||||
} | ||||||
return expiration, nil | ||||||
} | ||||||
|
||||||
// NewIDAndTimestamp returns a new unique ID and corresponding timestamp. | ||||||
// | ||||||
// The ID is the current unix time in nanoseconds encoded as a string in base32. | ||||||
// The timestamp is the same time, encoded as a string in time.RFC3339Nano. | ||||||
func NewIDAndTimestamp() (id string, timestamp time.Time) { | ||||||
now := time.Now() | ||||||
nowUnix := uint64(now.UnixNano()) | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What happens when the time reports the same unixnano? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just use a counter. Store with rules storage? And use with request prompts as well. Or combine boot ID with monotonic boot time. (look for helper, reads from
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. According to the time package documentation (https://pkg.go.dev/time#hdr-Monotonic_Clocks),
Thus, the monotonic time is only stripped when rules/prompts are read from disk. This occurs when snapd is restarted, including on system start. Unfortunately, this means There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. where and what are this IDs used for ? in general snapd prefers to use sequential increasing IDs? at least for norification from the kernel don't we get them in a single goroutine? in that context using sequential IDs would be cheap, no? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. AFAIU the IDs are stored within the prompting namespace in notices and we need them to be unique across reboots too (@olivercalder please correct me if I'm wrong), so to keep them sequential we'd either need to store some state. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. well, we could restart counting from the max of the pending ones when we restart There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yes, that's a good point There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We could start from the max of the pending ones, though that would allow for the potential that IDs are reused for different rules, if a rule is removed, then snapd restarted. This isn't necessarily a problem, as we don't guarantee that IDs are never reused, but I'd prefer to keep them unique if possible. I think storing a count as part of the rule storage would solve this problem, and it can be atomically incremented whenever a new rule is added. For prompt requests, which are not written to disk (because of listener/notify socket behavior, but that can be discussed in a later PR), we can just keep a count in memory and atomically increment it. So, my opinion is that we should remove this function for now and add a |
||||||
nowBytes := make([]byte, 8) | ||||||
binary.BigEndian.PutUint64(nowBytes, nowUnix) | ||||||
id = base32.StdEncoding.EncodeToString(nowBytes) | ||||||
return id, now | ||||||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,153 @@ | ||
// -*- Mode: Go; indent-tabs-mode: t -*- | ||
|
||
/* | ||
* Copyright (C) 2024 Canonical Ltd | ||
* | ||
* This program is free software: you can redistribute it and/or modify | ||
* it under the terms of the GNU General Public License version 3 as | ||
* published by the Free Software Foundation. | ||
* | ||
* This program is distributed in the hope that it will be useful, | ||
* but WITHOUT ANY WARRANTY; without even the implied warranty of | ||
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the | ||
* GNU General Public License for more details. | ||
* | ||
* You should have received a copy of the GNU General Public License | ||
* along with this program. If not, see <http://www.gnu.org/licenses/>. | ||
* | ||
*/ | ||
|
||
package prompting_test | ||
|
||
import ( | ||
"encoding/base32" | ||
"encoding/binary" | ||
"testing" | ||
"time" | ||
|
||
. "gopkg.in/check.v1" | ||
|
||
"github.com/snapcore/snapd/dirs" | ||
"github.com/snapcore/snapd/interfaces/prompting" | ||
) | ||
|
||
func Test(t *testing.T) { TestingT(t) } | ||
|
||
type promptingSuite struct { | ||
tmpdir string | ||
} | ||
|
||
var _ = Suite(&promptingSuite{}) | ||
|
||
func (s *promptingSuite) SetUpTest(c *C) { | ||
s.tmpdir = c.MkDir() | ||
dirs.SetRootDir(s.tmpdir) | ||
} | ||
|
||
func (s *promptingSuite) TestOutcomeIsAllow(c *C) { | ||
result, err := prompting.OutcomeAllow.IsAllow() | ||
c.Check(err, IsNil) | ||
c.Check(result, Equals, true) | ||
result, err = prompting.OutcomeDeny.IsAllow() | ||
c.Check(err, IsNil) | ||
c.Check(result, Equals, false) | ||
_, err = prompting.OutcomeUnset.IsAllow() | ||
c.Check(err, ErrorMatches, `outcome must be.*`) | ||
_, err = prompting.OutcomeType("foo").IsAllow() | ||
c.Check(err, ErrorMatches, `outcome must be.*`) | ||
} | ||
|
||
func (s *promptingSuite) TestValidateOutcome(c *C) { | ||
c.Assert(prompting.ValidateOutcome(prompting.OutcomeAllow), Equals, nil) | ||
c.Assert(prompting.ValidateOutcome(prompting.OutcomeDeny), Equals, nil) | ||
c.Assert(prompting.ValidateOutcome(prompting.OutcomeUnset), ErrorMatches, `outcome must be.*`) | ||
c.Assert(prompting.ValidateOutcome(prompting.OutcomeType("foo")), ErrorMatches, `outcome must be.*`) | ||
} | ||
|
||
func (s *promptingSuite) TestValidateLifespanExpiration(c *C) { | ||
var unsetExpiration time.Time | ||
currTime := time.Now() | ||
negativeExpiration := currTime.Add(-5 * time.Second) | ||
validExpiration := currTime.Add(10 * time.Minute) | ||
|
||
for _, lifespan := range []prompting.LifespanType{ | ||
prompting.LifespanForever, | ||
prompting.LifespanSingle, | ||
} { | ||
err := prompting.ValidateLifespanExpiration(lifespan, unsetExpiration, currTime) | ||
c.Check(err, IsNil) | ||
for _, exp := range []time.Time{negativeExpiration, validExpiration} { | ||
err = prompting.ValidateLifespanExpiration(lifespan, exp, currTime) | ||
c.Check(err, ErrorMatches, `expiration must be omitted.*`) | ||
} | ||
} | ||
|
||
err := prompting.ValidateLifespanExpiration(prompting.LifespanTimespan, unsetExpiration, currTime) | ||
c.Check(err, ErrorMatches, `expiration must be non-zero.*`) | ||
|
||
err = prompting.ValidateLifespanExpiration(prompting.LifespanTimespan, negativeExpiration, currTime) | ||
c.Check(err, ErrorMatches, `expiration time has already passed.*`) | ||
|
||
err = prompting.ValidateLifespanExpiration(prompting.LifespanTimespan, validExpiration, currTime) | ||
c.Check(err, IsNil) | ||
} | ||
|
||
func (s *promptingSuite) TestValidateLifespanParseDuration(c *C) { | ||
unsetDuration := "" | ||
invalidDuration := "foo" | ||
negativeDuration := "-5s" | ||
validDuration := "10m" | ||
parsedValidDuration, err := time.ParseDuration(validDuration) | ||
c.Assert(err, IsNil) | ||
|
||
for _, lifespan := range []prompting.LifespanType{ | ||
prompting.LifespanForever, | ||
prompting.LifespanSingle, | ||
} { | ||
expiration, err := prompting.ValidateLifespanParseDuration(lifespan, unsetDuration) | ||
c.Check(expiration.IsZero(), Equals, true) | ||
c.Check(err, IsNil) | ||
for _, dur := range []string{invalidDuration, negativeDuration, validDuration} { | ||
expiration, err = prompting.ValidateLifespanParseDuration(lifespan, dur) | ||
c.Check(expiration.IsZero(), Equals, true) | ||
c.Check(err, ErrorMatches, `duration must be empty.*`) | ||
} | ||
} | ||
|
||
expiration, err := prompting.ValidateLifespanParseDuration(prompting.LifespanTimespan, unsetDuration) | ||
c.Check(expiration.IsZero(), Equals, true) | ||
c.Check(err, ErrorMatches, `duration must be non-empty.*`) | ||
|
||
expiration, err = prompting.ValidateLifespanParseDuration(prompting.LifespanTimespan, invalidDuration) | ||
c.Check(expiration.IsZero(), Equals, true) | ||
c.Check(err, ErrorMatches, `error parsing duration string.*`) | ||
|
||
expiration, err = prompting.ValidateLifespanParseDuration(prompting.LifespanTimespan, negativeDuration) | ||
c.Check(expiration.IsZero(), Equals, true) | ||
c.Check(err, ErrorMatches, `duration must be greater than zero.*`) | ||
|
||
expiration, err = prompting.ValidateLifespanParseDuration(prompting.LifespanTimespan, validDuration) | ||
c.Check(err, IsNil) | ||
c.Check(expiration.After(time.Now()), Equals, true) | ||
c.Check(expiration.Before(time.Now().Add(parsedValidDuration)), Equals, true) | ||
} | ||
|
||
func (s *promptingSuite) TestNewIDAndTimestamp(c *C) { | ||
before := time.Now() | ||
id, _ := prompting.NewIDAndTimestamp() | ||
idPaired, timestampPaired := prompting.NewIDAndTimestamp() | ||
after := time.Now() | ||
data1, err := base32.StdEncoding.DecodeString(id) | ||
c.Assert(err, IsNil) | ||
data2, err := base32.StdEncoding.DecodeString(idPaired) | ||
c.Assert(err, IsNil) | ||
parsedNs := int64(binary.BigEndian.Uint64(data1)) | ||
parsedNsPaired := int64(binary.BigEndian.Uint64(data2)) | ||
parsedTime := time.Unix(parsedNs/1000000000, parsedNs%1000000000) | ||
parsedTimePaired := time.Unix(parsedNsPaired/1000000000, parsedNsPaired%1000000000) | ||
c.Assert(parsedTime.After(before), Equals, true) | ||
c.Assert(parsedTime.Before(after), Equals, true) | ||
c.Assert(parsedTimePaired.After(before), Equals, true) | ||
c.Assert(parsedTimePaired.Before(after), Equals, true) | ||
c.Assert(parsedTimePaired.Equal(timestampPaired), Equals, true) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this needs a doc string