Support API versioning in broker API and authd by adombeck · Pull Request #1446 · canonical/authd

adombeck · 2026-04-14T19:55:25Z

Adds a com.ubuntu.authd.Broker2 D-Bus interface in accordance with https://dbus.freedesktop.org/doc/dbus-api-design.html#api-versioning

UDENG-9793

adombeck · 2026-04-14T20:02:28Z

@denisonbarbosa do you want take over from here?

denisonbarbosa · 2026-04-14T20:05:01Z

@denisonbarbosa do you want take over from here?

I'll take a look at the current status and we can talk more about this (maybe in a HO?)

adombeck · 2026-04-14T20:05:46Z

I'll take a look at the current status and we can talk more about this (maybe in a HO?)

sounds good

codecov · 2026-04-14T20:08:33Z

Codecov Report

❌ Patch coverage is 85.18519% with 8 lines in your changes missing coverage. Please review.
✅ Project coverage is 86.11%. Comparing base (e9e7bfd) to head (c393a64).
⚠️ Report is 5 commits behind head on main.

Files with missing lines	Patch %	Lines
...d-oidc-brokers/internal/dbusservice/dbusservice.go	65.00%	7 Missing ⚠️
internal/brokers/dbusbroker.go	96.87%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1446      +/-   ##
==========================================
- Coverage   87.03%   86.11%   -0.93%     
==========================================
  Files          93      113      +20     
  Lines        6379     7489    +1110     
  Branches      111      111              
==========================================
+ Hits         5552     6449     +897     
- Misses        771      984     +213     
  Partials       56       56

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copilot

Pull request overview

This PR introduces D-Bus API versioning support for brokers by adding a versioned interface (com.ubuntu.authd.Broker2) and updating both the daemon-side broker client and the OIDC broker D-Bus service to negotiate/use the appropriate interface version.

Changes:

Add daemon-side broker interface discovery via D-Bus introspection and select the “latest” available broker interface when calling methods.
Export multiple D-Bus interfaces (Broker and Broker2) from the OIDC broker service, and thread an API version into the broker implementation to gate behavior changes.
Update/adjust tests and golden outputs to reflect new behavior (e.g., brokers not available on D-Bus are ignored).

Reviewed changes

Copilot reviewed 12 out of 12 changed files in this pull request and generated 5 comments.

Show a summary per file

File	Description
internal/brokers/dbusbroker.go	Introspects broker object to detect supported interfaces and uses latest interface for calls.
internal/brokers/dbusbroker_test.go	Adds unit tests for interface detection/sorting and latest-interface selection.
internal/brokers/manager_test.go	Updates manager tests and sets up D-Bus broker mocks in `TestMain`.
internal/brokers/testdata/golden/TestNewManager/Ignores_brokers_not_available_on_dbus	New golden output for ignoring non-exported brokers.
internal/brokers/testdata/golden/TestNewManager/Creates_manager_even_if_broker_is_not_exported_on_dbus	Removes/updates prior golden output for behavior that changed.
authd-oidc-brokers/internal/dbusservice/dbusservice.go	Exports multiple broker interfaces and constructs per-interface broker instances.
authd-oidc-brokers/internal/dbusservice/methods.go	Moves exported D-Bus methods onto the per-interface exported object.
authd-oidc-brokers/internal/dbusservice/localbus.go	Adjusts local-bus implementation for the dbusservice package (with build tag).
authd-oidc-brokers/internal/broker/broker.go	Adds API version field and gates new response value behind version >= 2.
authd-oidc-brokers/internal/broker/helper_test.go	Updates test helper to pass latest API version into broker constructor.
authd-oidc-brokers/internal/broker/broker_test.go	Updates broker tests to pass latest API version into broker constructor.
authd-oidc-brokers/cmd/authd-oidc/daemon/daemon.go	Updates daemon to let dbusservice construct versioned broker instances from config.

Comments suppressed due to low confidence (1)

authd-oidc-brokers/internal/dbusservice/localbus.go:31

localbus.go now defines getBus on *Interface, but getBus is called on *Service (and the disconnect field also lives on Service). This won’t compile under the withlocalbus build tag, and the file also references context/log without importing them. The receiver should remain *Service (matching systembus.go), and the disconnect cleanup should be set on Service.

// getBus creates the local bus and returns a connection to the bus.
// It attaches a disconnect handler to stop the local bus subprocess.
func (s *Interface) getBus() (*dbus.Conn, error) {
	cleanup, err := testutils.StartSystemBusMock()
	if err != nil {
		return nil, err
	}
	log.Infof(context.Background(), "Using local bus address: %s", os.Getenv("DBUS_SYSTEM_BUS_ADDRESS"))
	conn, err := dbus.ConnectSystemBus()
	if err != nil {
		return nil, err
	}

	s.disconnect = func() {
		conn.Close()
		cleanup()
	}
	return conn, err

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot

Pull request overview

Copilot reviewed 12 out of 12 changed files in this pull request and generated 2 comments.

Comments suppressed due to low confidence (1)

authd-oidc-brokers/internal/dbusservice/localbus.go:31

localbus.go (build tag withlocalbus) no longer compiles: getBus is now a method on *Interface, but it assigns s.disconnect (field exists on Service, not Interface) and dbusservice.New() calls service.getBus() (receiver mismatch). This file also uses context.Background() and log.Infof without importing context/log. getBus should likely remain a *Service method (matching systembus.go) and set service.disconnect.

// getBus creates the local bus and returns a connection to the bus.
// It attaches a disconnect handler to stop the local bus subprocess.
func (s *Interface) getBus() (*dbus.Conn, error) {
	cleanup, err := testutils.StartSystemBusMock()
	if err != nil {
		return nil, err
	}
	log.Infof(context.Background(), "Using local bus address: %s", os.Getenv("DBUS_SYSTEM_BUS_ADDRESS"))
	conn, err := dbus.ConnectSystemBus()
	if err != nil {
		return nil, err
	}

	s.disconnect = func() {
		conn.Close()
		cleanup()
	}
	return conn, err

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot

Pull request overview

Copilot reviewed 12 out of 12 changed files in this pull request and generated 3 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot

Pull request overview

Copilot reviewed 12 out of 12 changed files in this pull request and generated 1 comment.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-04-23T15:15:40Z

+	slices.SortFunc(supportedInterfaces, func(a, b string) int {
+		versionA, err := strconv.Atoi(strings.TrimPrefix(a, DbusBaseInterface))
+		if err != nil {
+			// If the interface doesn't match the expected format, we consider it as less than any valid versioned interface.
+			return -1
+		}
+		versionB, err := strconv.Atoi(strings.TrimPrefix(b, DbusBaseInterface))
+		if err != nil {
+			// If the interface doesn't match the expected format, we consider it as less than any valid versioned interface.
+			return 1
+		}
+		if versionA < versionB {
+			return -1
+		} else if versionA > versionB {
+			return 1
+		}
+		return 0
+	})


The slices.SortFunc comparator violates strict weak ordering for interfaces that parse as non-numeric suffixes (including the unversioned base interface). For example, comparing base-vs-base returns -1 instead of 0, which can lead to undefined/unstable sorting and potentially selecting the wrong “latest” interface. Consider normalizing interface names to a numeric version first (treat com.ubuntu.authd.Broker as version 1) and ensure the comparator returns 0 when versions are equal/unknown in the same way for both sides.

seems valid to me. what do you think, @denisonbarbosa?

I'd like to see how copilot fixes it. @copilot, please push a fix.

The last commit copilot pushed fixes it and it LGTM.

Either the diff looks weird to me, or Copilot only reverted the change you did to the slice declaration and didn't touch the SortFunc at all?

Nvm, I was looking at the wrong commit 😅

Adds a com.ubuntu.authd.Broker2 D-Bus interface in accordance with https://dbus.freedesktop.org/doc/dbus-api-design.html#api-versioning Also authd now properly considers the possibility of multiple interfaces and, for now, choose the latest one available.

Agent-Logs-Url: https://github.com/canonical/authd/sessions/9ae9064a-3eaf-4982-a7bf-d0d1b95133a6 Co-authored-by: adombeck <18482300+adombeck@users.noreply.github.com>

adombeck · 2026-04-24T11:45:36Z

LGTM now

Agent-Logs-Url: https://github.com/canonical/authd/sessions/2156af1c-51fa-4e85-8e59-050702012524 Co-authored-by: adombeck <18482300+adombeck@users.noreply.github.com>

adombeck mentioned this pull request Apr 14, 2026

Return PAM_MAXTRIES on too many auth failures #1425

Open

adombeck requested a review from Copilot April 16, 2026 15:52

Copilot started reviewing on behalf of adombeck April 16, 2026 15:52 View session

Copilot AI reviewed Apr 16, 2026

View reviewed changes

denisonbarbosa requested a review from Copilot April 17, 2026 12:06

Copilot started reviewing on behalf of denisonbarbosa April 17, 2026 12:06 View session

Copilot AI reviewed Apr 17, 2026

View reviewed changes

Comment thread internal/brokers/dbusbroker_test.go Outdated

Comment thread authd-oidc-brokers/internal/broker/broker.go

denisonbarbosa force-pushed the broker-api-versioning branch from 5f04457 to e4872c6 Compare April 17, 2026 12:28

adombeck changed the base branch from exit-on-max-tries to main April 17, 2026 12:43

denisonbarbosa force-pushed the broker-api-versioning branch from e4872c6 to beab51a Compare April 23, 2026 11:12

denisonbarbosa requested a review from Copilot April 23, 2026 11:14

Copilot started reviewing on behalf of denisonbarbosa April 23, 2026 11:14 View session

Copilot AI reviewed Apr 23, 2026

View reviewed changes

Comment thread authd-oidc-brokers/internal/dbusservice/localbus.go

Comment thread authd-oidc-brokers/internal/dbusservice/dbusservice.go

Comment thread internal/brokers/dbusbroker.go

denisonbarbosa force-pushed the broker-api-versioning branch from beab51a to c393a64 Compare April 23, 2026 14:58

denisonbarbosa requested a review from Copilot April 23, 2026 14:59

Copilot started reviewing on behalf of denisonbarbosa April 23, 2026 15:03 View session

Copilot AI reviewed Apr 23, 2026

View reviewed changes

denisonbarbosa changed the title ~~WIP: Support API versioning in broker API~~ Support API versioning in broker API and authd Apr 24, 2026

denisonbarbosa marked this pull request as ready for review April 24, 2026 11:09

adombeck force-pushed the broker-api-versioning branch from c393a64 to 1130087 Compare April 24, 2026 11:31

adombeck commented Apr 24, 2026

View reviewed changes

Comment thread internal/brokers/dbusbroker.go Outdated

Copilot started work on behalf of adombeck April 24, 2026 11:33 View session

Fix sort function to not call strconv.Atoi on known-failing empty suffix

d73971e

Agent-Logs-Url: https://github.com/canonical/authd/sessions/9ae9064a-3eaf-4982-a7bf-d0d1b95133a6 Co-authored-by: adombeck <18482300+adombeck@users.noreply.github.com>

Copilot finished work on behalf of adombeck April 24, 2026 11:43

adombeck requested a review from denisonbarbosa April 24, 2026 11:45

Copilot started work on behalf of adombeck April 24, 2026 11:53 View session

Revert empty slice literal; update test expectation to nil

62773d2

Agent-Logs-Url: https://github.com/canonical/authd/sessions/2156af1c-51fa-4e85-8e59-050702012524 Co-authored-by: adombeck <18482300+adombeck@users.noreply.github.com>

Copilot finished work on behalf of adombeck April 24, 2026 11:59

Conversation

adombeck commented Apr 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

adombeck commented Apr 14, 2026

Uh oh!

denisonbarbosa commented Apr 14, 2026

Uh oh!

adombeck commented Apr 14, 2026

Uh oh!

codecov Bot commented Apr 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Apr 23, 2026

Choose a reason for hiding this comment

Uh oh!

adombeck Apr 24, 2026

Choose a reason for hiding this comment

Uh oh!

adombeck Apr 24, 2026

Choose a reason for hiding this comment

Uh oh!

adombeck Apr 24, 2026

Choose a reason for hiding this comment

Uh oh!

denisonbarbosa Apr 24, 2026

Choose a reason for hiding this comment

Uh oh!

denisonbarbosa Apr 24, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

adombeck commented Apr 24, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

adombeck commented Apr 14, 2026 •

edited

Loading

codecov Bot commented Apr 14, 2026 •

edited

Loading