From 0029bbe377f141073af5e6ce9282b3f251ba4a36 Mon Sep 17 00:00:00 2001 From: Didier Roche Date: Mon, 25 Sep 2023 18:28:48 +0200 Subject: [PATCH 1/7] Fix missing userinfo metadata when granting access In the selection of userinfo, we missed the marker in the data json struct in the examplebroker. Readding it. --- internal/brokers/examplebroker/broker.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/brokers/examplebroker/broker.go b/internal/brokers/examplebroker/broker.go index 087b04a86..9b107024c 100644 --- a/internal/brokers/examplebroker/broker.go +++ b/internal/brokers/examplebroker/broker.go @@ -608,7 +608,7 @@ func (b *Broker) handleIsAuthenticated(ctx context.Context, sessionInfo sessionI return responses.AuthDenied, `{"message": "user not found"}`, nil } - return responses.AuthGranted, userInfoFromName(user.Name), nil + return responses.AuthGranted, fmt.Sprintf(`{"userinfo": %s}`, userInfoFromName(user.Name)), nil } // EndSession ends the requested session and triggers the necessary clean up steps, if any. From 5a22c216979d7516e45cd406c62af2b0a4e46007 Mon Sep 17 00:00:00 2001 From: denisonbarbosa Date: Tue, 26 Sep 2023 06:45:28 -0400 Subject: [PATCH 2/7] Remove brokers import on examplebroker The examplebroker package is imported by brokers when using the "withexamplebroker" build tag. If we import the brokers pkg in the examplebroker one, we'll get a build error due to cycle imports. --- internal/brokers/examplebroker/dbus.go | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/internal/brokers/examplebroker/dbus.go b/internal/brokers/examplebroker/dbus.go index 220179158..d5fa51ef1 100644 --- a/internal/brokers/examplebroker/dbus.go +++ b/internal/brokers/examplebroker/dbus.go @@ -8,13 +8,14 @@ import ( "github.com/godbus/dbus/v5" "github.com/godbus/dbus/v5/introspect" - "github.com/ubuntu/authd/internal/brokers" "github.com/ubuntu/decorate" ) const ( dbusObjectPath = "/com/ubuntu/authd/ExampleBroker" busName = "com.ubuntu.authd.ExampleBroker" + // we need to redeclare the interface here to avoid include cycles. + dbusInterface = "com.ubuntu.authd.Broker" ) // Bus is the D-Bus object that will answer calls for the broker. @@ -34,7 +35,7 @@ func StartBus(ctx context.Context, cfgPath string) (err error) { b, _, _ := New("ExampleBroker") obj := Bus{broker: b} - err = conn.Export(&obj, dbusObjectPath, brokers.DbusInterface) + err = conn.Export(&obj, dbusObjectPath, dbusInterface) if err != nil { return err } @@ -44,7 +45,7 @@ func StartBus(ctx context.Context, cfgPath string) (err error) { Interfaces: []introspect.Interface{ introspect.IntrospectData, { - Name: brokers.DbusInterface, + Name: dbusInterface, Methods: introspect.Methods(&obj), }, }, From 61543ea0662f0faf16093d2720fad2c457513beb Mon Sep 17 00:00:00 2001 From: denisonbarbosa Date: Tue, 26 Sep 2023 06:48:41 -0400 Subject: [PATCH 3/7] Remove unused code from examplebroker We were not using the exampleUser type for anything (neither its Stringer implementation), so it's better to simplify things. --- internal/brokers/examplebroker/broker.go | 79 +++--------------------- 1 file changed, 8 insertions(+), 71 deletions(-) diff --git a/internal/brokers/examplebroker/broker.go b/internal/brokers/examplebroker/broker.go index 9b107024c..c7f8d0fd1 100644 --- a/internal/brokers/examplebroker/broker.go +++ b/internal/brokers/examplebroker/broker.go @@ -59,76 +59,13 @@ type Broker struct { isAuthenticatedCallsMu sync.Mutex } -type exampleUser struct { - UID string `json:"uid"` - Name string `json:"name"` - Groups map[string]map[string]string `json:"groups"` -} - -func (u exampleUser) String() string { - data, err := json.Marshal(u) - if err != nil { - panic(fmt.Sprintf("Invalid user data: %v", err)) - } - return string(data) -} - var ( - exampleUsers = map[string]exampleUser{ - "user1": { - UID: "4245874", - Name: "My user", - Groups: map[string]map[string]string{ - "group1": { - "name": "Group 1", - "gid": "3884", - }, - "group2": { - "name": "Group 2", - "gid": "4884", - }, - }, - }, - "user2": { - UID: "33333", - Name: "My secondary user", - Groups: map[string]map[string]string{ - "group2": { - "name": "Group 2", - "gid": "4884", - }, - }, - }, - "user-mfa": { - UID: "44444", - Name: "User that needs MFA", - Groups: map[string]map[string]string{ - "group1": { - "name": "Group 1", - "gid": "3884", - }, - }, - }, - "user-needs-reset": { - UID: "55555", - Name: "User that needs passwd reset", - Groups: map[string]map[string]string{ - "group1": { - "name": "Group 1", - "gid": "3884", - }, - }, - }, - "user-can-reset": { - UID: "66666", - Name: "User that can passwd reset", - Groups: map[string]map[string]string{ - "group1": { - "name": "Group 1", - "gid": "3884", - }, - }, - }, + exampleUsers = map[string]string{ + "user1": "My user", + "user2": "My secondary user", + "user-mfa": "User that needs MFA", + "user-needs-reset": "User that needs passwd reset", + "user-can-reset": "User that can passwd reset", } ) @@ -603,12 +540,12 @@ func (b *Broker) handleIsAuthenticated(ctx context.Context, sessionInfo sessionI } } - user, exists := exampleUsers[sessionInfo.username] + name, exists := exampleUsers[sessionInfo.username] if !exists { return responses.AuthDenied, `{"message": "user not found"}`, nil } - return responses.AuthGranted, fmt.Sprintf(`{"userinfo": %s}`, userInfoFromName(user.Name)), nil + return responses.AuthGranted, fmt.Sprintf(`{"userinfo": %s}`, userInfoFromName(name)), nil } // EndSession ends the requested session and triggers the necessary clean up steps, if any. From 0a61c74e254143cf99486d82177f2a7dbc8c2085 Mon Sep 17 00:00:00 2001 From: denisonbarbosa Date: Tue, 26 Sep 2023 06:49:53 -0400 Subject: [PATCH 4/7] No longer return userinfo when access is auth.Next The user information should only be provided when we return auth.Granted --- internal/brokers/examplebroker/broker.go | 1 + 1 file changed, 1 insertion(+) diff --git a/internal/brokers/examplebroker/broker.go b/internal/brokers/examplebroker/broker.go index c7f8d0fd1..9ba804cd7 100644 --- a/internal/brokers/examplebroker/broker.go +++ b/internal/brokers/examplebroker/broker.go @@ -428,6 +428,7 @@ func (b *Broker) IsAuthenticated(ctx context.Context, sessionID, authenticationD if sessionInfo.currentMfaStep < sessionInfo.neededMfa { sessionInfo.currentMfaStep++ access = responses.AuthNext + data = "" } } } else if access == responses.AuthRetry { From 218249920e2b33c49db52f1e3c7e047079af6d34 Mon Sep 17 00:00:00 2001 From: Didier Roche Date: Tue, 26 Sep 2023 15:53:32 +0200 Subject: [PATCH 5/7] Rename skip-button to button MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This is the ui layout used in other parts, let’s align with it. --- internal/brokers/examplebroker/broker.go | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/internal/brokers/examplebroker/broker.go b/internal/brokers/examplebroker/broker.go index 9ba804cd7..72c98d551 100644 --- a/internal/brokers/examplebroker/broker.go +++ b/internal/brokers/examplebroker/broker.go @@ -295,14 +295,14 @@ func getSupportedModes(sessionInfo sessionInfo, supportedUILayouts []map[string] }), } - if layout["skip-button"] != "" { + if layout["button"] != "" { allModes["optionalreset"] = map[string]string{ "selection_label": "Password reset", "ui": mapToJSON(map[string]string{ - "type": "newpassword", - "label": "Enter your new password", - "entry": "chars_password", - "skip-button": "Skip", + "type": "newpassword", + "label": "Enter your new password", + "entry": "chars_password", + "button": "Skip", }), } } From d797afa1199a0ed1523425386870a7af3bd5737b Mon Sep 17 00:00:00 2001 From: Didier Roche Date: Tue, 26 Sep 2023 15:54:58 +0200 Subject: [PATCH 6/7] Set timelapse label on label entry --- internal/brokers/examplebroker/broker.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/internal/brokers/examplebroker/broker.go b/internal/brokers/examplebroker/broker.go index 72c98d551..125cd72a3 100644 --- a/internal/brokers/examplebroker/broker.go +++ b/internal/brokers/examplebroker/broker.go @@ -287,7 +287,7 @@ func getSupportedModes(sessionInfo sessionInfo, supportedUILayouts []map[string] break } allModes["mandatoryreset"] = map[string]string{ - "selection_label": "Password reset (3 days until mandatory)", + "selection_label": "Password reset", "ui": mapToJSON(map[string]string{ "type": "newpassword", "label": "Enter your new password", @@ -300,7 +300,7 @@ func getSupportedModes(sessionInfo sessionInfo, supportedUILayouts []map[string] "selection_label": "Password reset", "ui": mapToJSON(map[string]string{ "type": "newpassword", - "label": "Enter your new password", + "label": "Enter your new password (3 days until mandatory)", "entry": "chars_password", "button": "Skip", }), From f0d6710c659dc62b5ac6cf9c077d622f9302f331 Mon Sep 17 00:00:00 2001 From: Didier Roche Date: Tue, 26 Sep 2023 16:27:17 +0200 Subject: [PATCH 7/7] Off by one error in MFA requests Rename the variables to make more sense to reason with in example broker. Co-authored-by: Denison Barbosa --- internal/brokers/examplebroker/broker.go | 33 ++++++++++-------------- 1 file changed, 13 insertions(+), 20 deletions(-) diff --git a/internal/brokers/examplebroker/broker.go b/internal/brokers/examplebroker/broker.go index 125cd72a3..73c825619 100644 --- a/internal/brokers/examplebroker/broker.go +++ b/internal/brokers/examplebroker/broker.go @@ -40,8 +40,8 @@ type sessionInfo struct { pwdChange passwdReset - neededMfa int - currentMfaStep int + neededAuthSteps int + currentAuthStep int } type isAuthenticatedCtx struct { @@ -92,16 +92,18 @@ func (b *Broker) NewSession(ctx context.Context, username, lang string) (session username: username, lang: lang, pwdChange: noReset, + currentAuthStep: 1, + neededAuthSteps: 1, attemptsPerMode: make(map[string]int), } switch username { case "user-mfa": - info.neededMfa = 3 + info.neededAuthSteps = 3 case "user-needs-reset": - info.neededMfa = 1 + info.neededAuthSteps = 2 info.pwdChange = mustReset case "user-can-reset": - info.neededMfa = 1 + info.neededAuthSteps = 2 info.pwdChange = canReset } @@ -122,11 +124,11 @@ func (b *Broker) GetAuthenticationModes(ctx context.Context, sessionID string, s allModes := getSupportedModes(sessionInfo, supportedUILayouts) // If the user needs mfa, we remove the last used mode from the list of available modes. - if sessionInfo.currentMfaStep > 0 && sessionInfo.currentMfaStep < sessionInfo.neededMfa { + if sessionInfo.currentAuthStep > 1 && sessionInfo.currentAuthStep < sessionInfo.neededAuthSteps { allModes = getMfaModes(sessionInfo, sessionInfo.allModes) } // If the user needs or can reset the password, we only show those authentication modes. - if sessionInfo.currentMfaStep > 0 && sessionInfo.pwdChange != noReset { + if sessionInfo.currentAuthStep > 1 && sessionInfo.pwdChange != noReset { allModes = getPasswdResetModes(sessionInfo, sessionInfo.allModes) } @@ -418,19 +420,10 @@ func (b *Broker) IsAuthenticated(ctx context.Context, sessionID, authenticationD }() access, data, err = b.handleIsAuthenticated(b.isAuthenticatedCalls[sessionID].ctx, sessionInfo, authData) - if access == responses.AuthGranted { - switch sessionInfo.username { - case "user-needs-reset": - fallthrough - case "user-can-reset": - fallthrough - case "user-mfa": - if sessionInfo.currentMfaStep < sessionInfo.neededMfa { - sessionInfo.currentMfaStep++ - access = responses.AuthNext - data = "" - } - } + if access == responses.AuthGranted && sessionInfo.currentAuthStep < sessionInfo.neededAuthSteps { + sessionInfo.currentAuthStep++ + access = responses.AuthNext + data = "" } else if access == responses.AuthRetry { sessionInfo.attemptsPerMode[sessionInfo.selectedMode]++ if sessionInfo.attemptsPerMode[sessionInfo.selectedMode] >= maxAttempts {