diff --git a/CHANGELOG.md b/CHANGELOG.md index f235243b..6f88c2f0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -20,8 +20,11 @@ NOTE: Add new changes BELOW THIS COMMENT. ### Fixed +- The HTTP API `GET /control/profile` endpoint failing when no users were configured ([#7985]). - Missing warning on the *Encryption Settings* page when using a certificate without an IP address. +[#7985]: https://github.com/AdguardTeam/AdGuardHome/issues/7985 + diff --git a/internal/home/auth.go b/internal/home/auth.go index f0e074c0..f586766b 100644 --- a/internal/home/auth.go +++ b/internal/home/auth.go @@ -54,7 +54,7 @@ type authConfig struct { // rateLimiter manages the rate limiting for login attempts. It must not be // nil. - rateLimiter loginRaateLimiter + rateLimiter loginRateLimiter // trustedProxies is a set of subnets considered as trusted. trustedProxies netutil.SubnetSet @@ -75,12 +75,27 @@ type authConfig struct { // auth stores web user information and handles authentication. type auth struct { - logger *slog.Logger - rateLimiter loginRaateLimiter + // logger is used to log the operation of the auth module. + logger *slog.Logger + + // rateLimiter manages rate limiting for login attempts. + rateLimiter loginRateLimiter + + // trustedProxies is a set of subnets considered trusted. trustedProxies netutil.SubnetSet - sessions aghuser.SessionStorage - users aghuser.DB - isGLiNet bool + + // sessions stores web users' sessions. + sessions aghuser.SessionStorage + + // users stores user credentials. + users aghuser.DB + + // isGLiNet indicates whether GLiNet mode is enabled. + isGLiNet bool + + // isUserless indicates that there are no users defined in the configuration + // file. + isUserless bool } // newAuth returns the new properly initialized *auth. @@ -111,6 +126,7 @@ func newAuth(ctx context.Context, conf *authConfig) (a *auth, err error) { sessions: s, users: userDB, isGLiNet: conf.isGLiNet, + isUserless: len(conf.users) == 0, }, nil } @@ -174,6 +190,8 @@ func (a *auth) addUser(ctx context.Context, u *webUser, password string) (err er panic(err) } + a.isUserless = false + a.logger.DebugContext(ctx, "added user", "login", u.Name) return nil diff --git a/internal/home/authhttp.go b/internal/home/authhttp.go index bc8e7068..e5517933 100644 --- a/internal/home/authhttp.go +++ b/internal/home/authhttp.go @@ -320,7 +320,7 @@ type authMiddlewareDefaultConfig struct { logger *slog.Logger // rateLimiter manages the rate limiting for login attempts. - rateLimiter loginRaateLimiter + rateLimiter loginRateLimiter // trustedProxies is a set of subnets considered as trusted. // @@ -340,7 +340,7 @@ type authMiddlewareDefaultConfig struct { // passes it with the context. type authMiddlewareDefault struct { logger *slog.Logger - rateLimiter loginRaateLimiter + rateLimiter loginRateLimiter trustedProxies netutil.SubnetSet sessions aghuser.SessionStorage users aghuser.DB diff --git a/internal/home/authratelimiter.go b/internal/home/authratelimiter.go index 95e8d571..dee88ff1 100644 --- a/internal/home/authratelimiter.go +++ b/internal/home/authratelimiter.go @@ -9,8 +9,8 @@ import ( // cache. const failedAuthTTL = 1 * time.Minute -// loginRaateLimiter is an interface for rate limiting login attempts. -type loginRaateLimiter interface { +// loginRateLimiter is an interface for rate limiting login attempts. +type loginRateLimiter interface { // check returns the duration of time left until a user is unblocked. // A non-positive result indicates that the user is not blocked. check(usrID string) (left time.Duration) @@ -66,7 +66,7 @@ func newAuthRateLimiter(blockDur time.Duration, maxAttempts uint) (ab *authRateL } // type check -var _ loginRaateLimiter = (*authRateLimiter)(nil) +var _ loginRateLimiter = (*authRateLimiter)(nil) // cleanupLocked checks each blocked users removing ones with expired TTL. For // internal use only. diff --git a/internal/home/config.go b/internal/home/config.go index 65452f8f..49dcf4ca 100644 --- a/internal/home/config.go +++ b/internal/home/config.go @@ -629,8 +629,8 @@ func validateBindHosts(conf *configuration) (err error) { } // parseConfig loads configuration from the YAML file, upgrading it if -// necessary. -func parseConfig() (err error) { +// necessary. l must not be nil. +func parseConfig(ctx context.Context, l *slog.Logger) (err error) { // Do the upgrade if necessary. config.fileData, err = readConfigFile() if err != nil { @@ -652,7 +652,7 @@ func parseConfig() (err error) { return err } else if upgraded { confPath := configFilePath() - log.Debug("writing config file %q after config upgrade", confPath) + l.DebugContext(ctx, "writing config file after config upgrade", "path", confPath) err = maybe.WriteFile(confPath, config.fileData, aghos.DefaultPermFile) if err != nil { @@ -666,7 +666,7 @@ func parseConfig() (err error) { return err } - err = validateConfig() + err = validateConfig(ctx, l) if err != nil { return err } @@ -679,8 +679,9 @@ func parseConfig() (err error) { return validateTLSCipherIDs(config.TLS.OverrideTLSCiphers) } -// validateConfig returns error if the configuration is invalid. -func validateConfig() (err error) { +// validateConfig returns error if the configuration is invalid. l must not be +// nil. +func validateConfig(ctx context.Context, l *slog.Logger) (err error) { err = validateBindHosts(config) if err != nil { // Don't wrap the error since it's informative enough as is. @@ -716,6 +717,10 @@ func validateConfig() (err error) { config.Filtering.FiltersUpdateIntervalHours = 24 } + if len(config.Users) == 0 { + l.WarnContext(ctx, "no users in the configuration file; authentication is disabled") + } + return nil } diff --git a/internal/home/home.go b/internal/home/home.go index 9536219d..ac180ff7 100644 --- a/internal/home/home.go +++ b/internal/home/home.go @@ -178,7 +178,8 @@ func setupContext(ctx context.Context, baseLogger *slog.Logger, opts options) (e return nil } - err = parseConfig() + // TODO(s.chzhen): Consider adding a key prefix. + err = parseConfig(ctx, baseLogger) if err != nil { log.Error("parsing configuration file: %s", err) @@ -862,7 +863,7 @@ func initUsers( baseLogger *slog.Logger, isGLiNet bool, ) (auth *auth, err error) { - var rateLimiter loginRaateLimiter + var rateLimiter loginRateLimiter if config.AuthAttempts > 0 && config.AuthBlockMin > 0 { blockDur := time.Duration(config.AuthBlockMin) * time.Minute rateLimiter = newAuthRateLimiter(blockDur, config.AuthAttempts) diff --git a/internal/home/profilehttp.go b/internal/home/profilehttp.go index 41d2c18e..74a7b2ed 100644 --- a/internal/home/profilehttp.go +++ b/internal/home/profilehttp.go @@ -47,7 +47,8 @@ type profileJSON struct { // handleGetProfile is the handler for GET /control/profile endpoint. func (web *webAPI) handleGetProfile(w http.ResponseWriter, r *http.Request) { var name string - if !web.auth.isGLiNet { + + if !web.auth.isGLiNet && !web.auth.isUserless { u, ok := webUserFromContext(r.Context()) if !ok { w.WriteHeader(http.StatusUnauthorized) diff --git a/internal/home/profilehttp_internal_test.go b/internal/home/profilehttp_internal_test.go new file mode 100644 index 00000000..56f07d22 --- /dev/null +++ b/internal/home/profilehttp_internal_test.go @@ -0,0 +1,119 @@ +package home + +import ( + "encoding/binary" + "net/http" + "net/http/httptest" + "os" + "path/filepath" + "testing" + "time" + + "github.com/AdguardTeam/AdGuardHome/internal/agh" + "github.com/AdguardTeam/golibs/testutil" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "golang.org/x/crypto/bcrypt" +) + +func TestWeb_HandleGetProfile(t *testing.T) { + storeGlobals(t) + + const ( + testTTL = 60 + + glTokenFileSuffix = "test" + + userName = "name" + userPassword = "password" + + path = "/control/profile" + ) + + passwordHash, err := bcrypt.GenerateFromPassword([]byte(userPassword), bcrypt.DefaultCost) + require.NoError(t, err) + + tempDir := t.TempDir() + glFilePrefix = tempDir + "/gl_token_" + glTokenFile := glFilePrefix + glTokenFileSuffix + + glFileData := make([]byte, 4) + binary.NativeEndian.PutUint32(glFileData, uint32(time.Now().Unix()+testTTL)) + + err = os.WriteFile(glTokenFile, glFileData, 0o644) + require.NoError(t, err) + + sessionsDB := filepath.Join(tempDir, "sessions.db") + + user := &webUser{ + Name: userName, + PasswordHash: string(passwordHash), + } + + auth, err := newAuth(testutil.ContextWithTimeout(t, testTimeout), &authConfig{ + baseLogger: testLogger, + rateLimiter: emptyRateLimiter{}, + trustedProxies: nil, + dbFilename: sessionsDB, + users: nil, + sessionTTL: testTTL * time.Second, + isGLiNet: false, + }) + require.NoError(t, err) + + t.Cleanup(func() { auth.close(testutil.ContextWithTimeout(t, testTimeout)) }) + + globalContext.mux = http.NewServeMux() + + tlsMgr, err := newTLSManager(testutil.ContextWithTimeout(t, testTimeout), &tlsManagerConfig{ + logger: testLogger, + confModifier: agh.EmptyConfigModifier{}, + }) + require.NoError(t, err) + + web, err := initWeb( + testutil.ContextWithTimeout(t, testTimeout), + options{}, + nil, + nil, + testLogger, + tlsMgr, + auth, + agh.EmptyConfigModifier{}, + false, + ) + require.NoError(t, err) + + globalContext.web = web + + mux := auth.middleware().Wrap(globalContext.mux) + + require.True(t, t.Run("userless", func(t *testing.T) { + w := httptest.NewRecorder() + r := httptest.NewRequest(http.MethodGet, path, nil) + + web.handleGetProfile(w, r) + assert.Equal(t, http.StatusOK, w.Code) + })) + + require.True(t, t.Run("add_user", func(t *testing.T) { + ctx := testutil.ContextWithTimeout(t, testTimeout) + err = auth.addUser(ctx, user, userPassword) + require.NoError(t, err) + + w := httptest.NewRecorder() + r := httptest.NewRequest(http.MethodGet, path, nil) + + web.handleGetProfile(w, r) + assert.Equal(t, http.StatusUnauthorized, w.Code) + + w = httptest.NewRecorder() + r = httptest.NewRequest(http.MethodGet, path, nil) + + loginCookie := generateAuthCookie(t, mux, userName, userPassword) + r.AddCookie(loginCookie) + + web.handleGetProfile(w, r) + assert.Equal(t, http.StatusUnauthorized, w.Code) + })) +}