Skip to content

Fix missing Authorization header for servers added in the settings dialog - #1246

Merged
HenriWahl merged 2 commits into
HenriWahl:Fix-missing-Authorization-headerfrom
discostur:fix-missing-authorization-header
Sep 7, 2026
Merged

HenriWahl merged 2 commits into
HenriWahl:Fix-missing-Authorization-headerfrom
discostur:fix-missing-authorization-header

Conversation

@discostur

Copy link
Copy Markdown
Contributor

create_server() sets refresh_authentication for every enabled server that does not save its password, which is the default for a newly added server:

# if password is not to be saved ask for it at startup
if (server.enabled is True and server.save_password is False and
        server.use_autologin is False):
    new_server.refresh_authentication = True

While that flag is set, fetch_url() deliberately sends the request through a temporary session without any credentials:

if no_auth is False and \
   not self.refresh_authentication or \
   no_auth is False and self.authentication == 'web':
    ...  # session with credentials
else:
    # send request without authentication data
    temporary_session = requests.Session()

So no Authorization header goes out at all - regardless of whether Basic Auth or a Bearer token is configured, which is what the issue describes.

At startup this stays invisible, because the loop that creates the servers resets the flag again a few lines later:

for server in conf.servers.values():
    created_server = create_server(server)
    if created_server is not None:
        servers[server.name] = created_server
        # for the next time no auth needed
        servers[server.name].refresh_authentication = False

The settings dialog does not do that. qui/dialogs/server.py calls create_server() when a server is added or edited and puts the result straight into servers, so a server configured in the dialog keeps its credentials suppressed for the rest of the session. That is exactly the reproduction path in the issue - add a server, configure credentials, watch the requests arrive without an Authorization header - and it also explains why it looks erratic: after a restart the same server works, because then the startup loop clears the flag.

Since the assignment and the reset cancel each other out at startup and only do damage in the dialog path, both are removed.

Verified with the little HTTP listener from the issue:

before after
server from the startup loop Authorization: Basic c29tZXVzZXI6... unchanged
server from the settings dialog Authorization: None Authorization: Basic c29tZXVzZXI6...

Bearer tokens behave the same way and arrive as Authorization: Bearer <token> now.

What the removed comment intended - asking for the password at startup when it is not saved - still happens, just through the regular path: the request goes out with the empty password, the monitor answers 401, and get_status() sets refresh_authentication so the GUI asks for credentials. Checked against a listener answering 401: the flag is set as before and get_status() returns 401, so the authentication dialog still comes up. Nothing in fetch_url() or in the authentication flow itself is touched.

A test covers the flag and the credentials ending up on the session; it fails without the change.

Fixes #1212

discostur and others added 2 commits September 1, 2026 15:56
create_server() set refresh_authentication for every enabled server which
does not save its password - which is the default for a new server. While
that flag is set, fetch_url() deliberately sends the request through a
temporary session without any credentials, so no Authorization header
goes out at all.

At startup this had no effect, because the loop creating the servers
immediately reset the flag again two lines later. The settings dialog
does not, so a server which was just added or edited keeps running with
its credentials suppressed for the rest of the session. That is exactly
the reproduction path in the issue: add a server, configure Basic Auth or
a Bearer token, and watch the requests arrive without an Authorization
header.

Since the assignment and the reset cancelled each other out at startup
and only did damage in the dialog, both are removed.

What the removed comment intended - asking for the password at startup
when it is not saved - still happens: the request goes out with the empty
password, the monitor answers 401, and get_status() sets
refresh_authentication so the GUI asks for credentials. That path is
untouched.

Fixes HenriWahl#1212

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
ZabbixServer.__init__() forced refresh_authentication to True. Until now the
loop which creates the servers at startup reset the flag right afterwards, so
the assignment only ever took effect for servers created by the settings
dialog. With that unconditional reset gone, every enabled Zabbix server now
starts up with the flag set.

That has no influence on the authentication itself - init_http() runs
check_authentication() on every poll and assigns the flag in each of its
branches before login() is consulted, and the session which super().init_http()
skips creating is created two lines later anyway. It does however show up in
the GUI: show_window() treats a set flag as "not ok", so the status window pops
open with only its header, the authenticate button appears and the treeview
reports an authentication problem - for a fully configured server which is
perfectly fine, until the first get_status() completes.

Since the flag is determined on every poll regardless of its initial value,
the constructor has no business presetting it.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Approval recommended

The changes directly address the described root cause with minimal surface area and add targeted tests that fail without the fix.

Pull request overview

This PR fixes missing Authorization headers for servers created/updated via the settings dialog by removing the refresh_authentication preset/reset behavior from create_server() and the startup creation loop, so newly created server instances don’t suppress credentials for the rest of the session.

Changes:

  • Remove create_server() logic that set refresh_authentication=True for enabled servers without saved passwords.
  • Remove the unconditional refresh_authentication=False reset in the startup server-creation loop.
  • Add unit tests asserting refresh_authentication is not preset and that sessions carry Basic/Bearer credentials.
File summaries
File Description
Nagstamon/servers/__init__.py Removes refresh_authentication preset/reset so dialog-created servers don’t drop auth headers.
Nagstamon/servers/Zabbix.py Stops presetting refresh_authentication=True in the constructor to avoid suppressing credentials before polling logic runs.
tests/test_servers.py Adds coverage for create_server() auth-flag behavior and session credential population (Basic/Bearer).
Review details
  • Files reviewed: 3/3 changed files
  • Comments generated: 0
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

@HenriWahl
HenriWahl changed the base branch from master to Fix-missing-Authorization-header September 7, 2026 21:04
@HenriWahl
HenriWahl merged commit 84cf9ce into HenriWahl:Fix-missing-Authorization-header Sep 7, 2026
HenriWahl pushed a commit that referenced this pull request Sep 16, 2026
…alog (#1246)

* fix: send the configured credentials for servers added in the settings

create_server() set refresh_authentication for every enabled server which
does not save its password - which is the default for a new server. While
that flag is set, fetch_url() deliberately sends the request through a
temporary session without any credentials, so no Authorization header
goes out at all.

At startup this had no effect, because the loop creating the servers
immediately reset the flag again two lines later. The settings dialog
does not, so a server which was just added or edited keeps running with
its credentials suppressed for the rest of the session. That is exactly
the reproduction path in the issue: add a server, configure Basic Auth or
a Bearer token, and watch the requests arrive without an Authorization
header.

Since the assignment and the reset cancelled each other out at startup
and only did damage in the dialog, both are removed.

What the removed comment intended - asking for the password at startup
when it is not saved - still happens: the request goes out with the empty
password, the monitor answers 401, and get_status() sets
refresh_authentication so the GUI asks for credentials. That path is
untouched.

Fixes #1212

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* fix(zabbix): do not preset refresh_authentication in the constructor

ZabbixServer.__init__() forced refresh_authentication to True. Until now the
loop which creates the servers at startup reset the flag right afterwards, so
the assignment only ever took effect for servers created by the settings
dialog. With that unconditional reset gone, every enabled Zabbix server now
starts up with the flag set.

That has no influence on the authentication itself - init_http() runs
check_authentication() on every poll and assigns the flag in each of its
branches before login() is consulted, and the session which super().init_http()
skips creating is created two lines later anyway. It does however show up in
the GUI: show_window() treats a set flag as "not ok", so the status window pops
open with only its header, the authenticate button appears and the treeview
reports an authentication problem - for a fully configured server which is
perfectly fine, until the first get_status() completes.

Since the flag is determined on every poll regardless of its initial value,
the constructor has no business presetting it.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Alertmanager server type does not send Authorization headers

3 participants