seclog: review improvements

Author: ernestl

seclog/nop.go

@@ -25,6 +25,7 @@ type nopLogger struct{}
 // Ensure [nopLogger] implements [SecurityLogger].
 var _ SecurityLogger = (*nopLogger)(nil)
 
+// NewNopLogger returns a [SecurityLogger] that silently discards all events.
 func NewNopLogger() SecurityLogger {
 	return nopLogger{}
 }

seclog/seclog.go

@@ -27,6 +27,9 @@ import (
 	"github.com/snapcore/snapd/logger"
 )
 
+// unknown is the placeholder for empty fields in descriptions.
+const unknown = "<unknown>"
+
 // Level is the importance or severity of a log event.
 // The higher the level, the more severe the event.
 type Level int
@@ -40,9 +43,7 @@ const (
 	LevelCritical Level = 5
 )
 
-// String returns a name for the level. If the level has a name, then that name
-// in uppercase is returned. Otherwise, a string of the form "UKNOWN(<number>)"
-// is returned.
+// String returns a name for the level.
 func (l Level) String() string {
 	switch l {
 	case LevelDebug:
@@ -61,9 +62,6 @@ func (l Level) String() string {
 }
 
 // SnapdUser represents the identity of a user for security log events.
-// The slog output schema is defined by [SnapdUser.LogValue], which
-// renders Expiration as "never" for zero values instead of emitting a
-// zero-value datetime.
 type SnapdUser struct {
 	ID             int64     `json:"snapd-user-id"`
 	StoreUserName  string    `json:"store-user-name"`
@@ -73,10 +71,8 @@ type SnapdUser struct {
 
 // String returns a colon-separated description of the user in the form
 // "<ID>:<StoreUserEmail>:<StoreUserName>". Fields that are unset use
-// "unknown" as a placeholder. A zero ID is treated as unset.
+// "<unknown>" as a placeholder. A zero ID means unset.
 func (u SnapdUser) String() string {
-	const unknown = "unknown"
-
 	id := unknown
 	if u.ID != 0 {
 		id = fmt.Sprintf("%d", u.ID)
@@ -115,8 +111,6 @@ type Reason struct {
 // "<Code>:<Message>". Fields that are unset use "unknown" as a
 // placeholder.
 func (r Reason) String() string {
-	const unknown = "unknown"
-
 	code := unknown
 	if r.Code != "" {
 		code = r.Code
@@ -156,7 +150,8 @@ type SecurityLogger interface {
 
 var (
 	globalLogger SecurityLogger = NewNopLogger()
-	lock         sync.Mutex
+	// lock guards globalLogger reads and writes.
+	lock sync.Mutex
 )
 
 // Setup activates the security logger, replacing any previously
@@ -172,10 +167,11 @@ func Setup(l SecurityLogger) {
 
 // LogLoggerEnabled logs that the security logger has been enabled.
 func LogLoggerEnabled() {
+	logger.Noticef("security logger enabled")
+
 	lock.Lock()
 	defer lock.Unlock()
 
-	logger.Noticef("security logger enabled")
 	globalLogger.LogAny(
 		Event{Category: "SYS", Name: "sys_logging_enabled", Level: LevelInfo},
 		"Security logging enabled",
@@ -184,10 +180,11 @@ func LogLoggerEnabled() {
 
 // LogLoggerDisabled logs that the security logger has been disabled.
 func LogLoggerDisabled() {
+	logger.Noticef("security logger disabled")
+
 	lock.Lock()
 	defer lock.Unlock()
 
-	logger.Noticef("security logger disabled")
 	globalLogger.LogAny(
 		Event{Category: "SYS", Name: "sys_logging_disabled", Level: LevelCritical},
 		"Security logging disabled",

seclog/seclog_test.go

@@ -46,7 +46,7 @@ func (s *SecLogSuite) SetUpTest(c *C) {
 	// No cleanup of the global logger is needed: every suite that
 	// uses seclog calls Setup in its own SetUpTest, replacing any
 	// leftover logger from a previous suite.
-	seclog.Setup(seclogtest.NewMockSecurityLogger(s.buf))
+	seclog.Setup(seclogtest.MockSecurityLogger(s.buf))
 }
 
 func (s *SecLogSuite) TearDownTest(c *C) {
@@ -76,8 +76,6 @@ func (s *SecLogSuite) TestString(c *C) {
 		"UNKNOWN(7)",
 	}
 
-	c.Assert(len(levels), Equals, len(expected))
-
 	obtained := make([]string, 0, len(levels))
 
 	for _, level := range levels {
@@ -93,17 +91,17 @@ func (s *SecLogSuite) TestSnapdUserString(c *C) {
 		ID: 42, StoreUserEmail: "a@b.com", StoreUserName: "jdoe",
 	}.String(), Equals, "42:a@b.com:jdoe")
 
-	// All fields zero/empty — all "unknown".
-	c.Check(seclog.SnapdUser{}.String(), Equals, "unknown:unknown:unknown")
+	// All fields zero/empty — all "<unknown>".
+	c.Check(seclog.SnapdUser{}.String(), Equals, "<unknown>:<unknown>:<unknown>")
 
 	// Only ID set.
-	c.Check(seclog.SnapdUser{ID: 7}.String(), Equals, "7:unknown:unknown")
+	c.Check(seclog.SnapdUser{ID: 7}.String(), Equals, "7:<unknown>:<unknown>")
 
 	// Only email set.
-	c.Check(seclog.SnapdUser{StoreUserEmail: "x@y.z"}.String(), Equals, "unknown:x@y.z:unknown")
+	c.Check(seclog.SnapdUser{StoreUserEmail: "x@y.z"}.String(), Equals, "<unknown>:x@y.z:<unknown>")
 
 	// Only username set.
-	c.Check(seclog.SnapdUser{StoreUserName: "root"}.String(), Equals, "unknown:unknown:root")
+	c.Check(seclog.SnapdUser{StoreUserName: "root"}.String(), Equals, "<unknown>:<unknown>:root")
 }
 
 func (s *SecLogSuite) TestReasonString(c *C) {
@@ -112,14 +110,14 @@ func (s *SecLogSuite) TestReasonString(c *C) {
 		Code: seclog.ReasonInvalidCredentials, Message: "bad password",
 	}.String(), Equals, "invalid-credentials:bad password")
 
-	// Both fields empty — all "unknown".
-	c.Check(seclog.Reason{}.String(), Equals, "unknown:unknown")
+	// Both fields empty — all "<unknown>".
+	c.Check(seclog.Reason{}.String(), Equals, "<unknown>:<unknown>")
 
 	// Only code set.
-	c.Check(seclog.Reason{Code: seclog.ReasonInternal}.String(), Equals, "internal:unknown")
+	c.Check(seclog.Reason{Code: seclog.ReasonInternal}.String(), Equals, "internal:<unknown>")
 
 	// Only message set.
-	c.Check(seclog.Reason{Message: "something broke"}.String(), Equals, "unknown:something broke")
+	c.Check(seclog.Reason{Message: "something broke"}.String(), Equals, "<unknown>:something broke")
 }
 
 func (s *SecLogSuite) TestSetupSuccess(c *C) {
@@ -134,7 +132,7 @@ func (s *SecLogSuite) TestSetupReplacesExistingLogger(c *C) {
 
 	// Replace with a second logger.
 	secondBuf := &bytes.Buffer{}
-	seclog.Setup(seclogtest.NewMockSecurityLogger(secondBuf))
+	seclog.Setup(seclogtest.MockSecurityLogger(secondBuf))
 
 	// New events go to the second logger, not the first.
 	s.buf.Reset()

seclog/seclogtest/seclogtest.go

@@ -29,39 +29,39 @@ import (
 	"github.com/snapcore/snapd/seclog"
 )
 
-// MockSecurityLogger implements seclog.SecurityLogger and writes event
+// mockLogger implements seclog.SecurityLogger and writes event
 // names plus key identifying data to a buffer. This lets tests verify
 // that the right events are emitted without depending on slog or JSON.
-type MockSecurityLogger struct {
+type mockLogger struct {
 	buf *bytes.Buffer
 }
 
-// Ensure MockSecurityLogger implements seclog.SecurityLogger.
-var _ seclog.SecurityLogger = (*MockSecurityLogger)(nil)
+// Ensure mockLogger implements seclog.SecurityLogger.
+var _ seclog.SecurityLogger = (*mockLogger)(nil)
 
-// NewMockSecurityLogger returns a MockSecurityLogger that writes to the
+// MockSecurityLogger returns a [seclog.SecurityLogger] that writes to the
 // given buffer.
-func NewMockSecurityLogger(buf *bytes.Buffer) *MockSecurityLogger {
-	return &MockSecurityLogger{buf: buf}
+func MockSecurityLogger(buf *bytes.Buffer) seclog.SecurityLogger {
+	return &mockLogger{buf: buf}
 }
 
 // LogAny implements [seclog.SecurityLogger.LogAny].
-func (m *MockSecurityLogger) LogAny(event seclog.Event, description string, attrs ...seclog.Attr) {
+func (m *mockLogger) LogAny(event seclog.Event, description string, attrs ...seclog.Attr) {
 	fmt.Fprintf(m.buf, "%s %s", event.Name, description)
 	for _, a := range attrs {
 		fmt.Fprintf(m.buf, " [%s=%#v]", a.Key, a.Value)
 	}
 	fmt.Fprintln(m.buf)
 }
 
-// NewMockSlogLogger returns a buffer and a constructor function matching the
+// MockSlogLogger returns a buffer and a constructor function matching the
 // seclog.NewSlogLogger signature. The constructor ignores its arguments and
-// returns a MockSecurityLogger backed by the buffer. This is intended for
+// returns a mockLogger backed by the buffer. This is intended for
 // mocking the newSlogLogger variable in tests.
-func NewMockSlogLogger() (*bytes.Buffer, func(io.Writer, string, seclog.Level) seclog.SecurityLogger) {
+func MockSlogLogger() (*bytes.Buffer, func(io.Writer, string, seclog.Level) seclog.SecurityLogger) {
 	buf := &bytes.Buffer{}
-	fn := func(_ io.Writer, _ string, _ seclog.Level) seclog.SecurityLogger {
-		return NewMockSecurityLogger(buf)
+	fn := func(io.Writer, string, seclog.Level) seclog.SecurityLogger {
+		return MockSecurityLogger(buf)
 	}
 	return buf, fn
 }