From 30a948c873fd816220ce62300616229ebff743fe Mon Sep 17 00:00:00 2001 From: Adrian Dombeck Date: Fri, 17 Jan 2025 14:46:56 +0100 Subject: [PATCH 1/4] Extract session mode constants For improved maintainability and code navigation. --- internal/broker/broker.go | 5 +++-- internal/broker/broker_test.go | 25 +++++++++++---------- internal/broker/helper_test.go | 3 ++- internal/broker/sessionmode/consts.go | 9 ++++++++ internal/providers/msentraid/msentraid.go | 3 ++- internal/providers/noprovider/noprovider.go | 3 ++- 6 files changed, 31 insertions(+), 17 deletions(-) create mode 100644 internal/broker/sessionmode/consts.go diff --git a/internal/broker/broker.go b/internal/broker/broker.go index 63c35fa8..f498a8b6 100644 --- a/internal/broker/broker.go +++ b/internal/broker/broker.go @@ -19,6 +19,7 @@ import ( "github.com/coreos/go-oidc/v3/oidc" "github.com/google/uuid" "github.com/ubuntu/authd-oidc-brokers/internal/broker/authmodes" + "github.com/ubuntu/authd-oidc-brokers/internal/broker/sessionmode" "github.com/ubuntu/authd-oidc-brokers/internal/consts" "github.com/ubuntu/authd-oidc-brokers/internal/fileutils" "github.com/ubuntu/authd-oidc-brokers/internal/password" @@ -399,7 +400,7 @@ func (b *Broker) generateUILayout(session *session, authModeID string) (map[stri case authmodes.NewPassword: label := "Create a local password" - if session.mode == "passwd" { + if session.mode == sessionmode.ChangePassword { label = "Update your local password" } @@ -596,7 +597,7 @@ func (b *Broker) handleIsAuthenticated(ctx context.Context, session *session, au authInfo.UserInfo = userInfo } - if session.mode == "passwd" { + if session.mode == sessionmode.ChangePassword { session.authInfo["auth_info"] = authInfo return AuthNext, nil } diff --git a/internal/broker/broker_test.go b/internal/broker/broker_test.go index 12e5ba8f..44dc8a7e 100644 --- a/internal/broker/broker_test.go +++ b/internal/broker/broker_test.go @@ -14,6 +14,7 @@ import ( "github.com/stretchr/testify/require" "github.com/ubuntu/authd-oidc-brokers/internal/broker" "github.com/ubuntu/authd-oidc-brokers/internal/broker/authmodes" + "github.com/ubuntu/authd-oidc-brokers/internal/broker/sessionmode" "github.com/ubuntu/authd-oidc-brokers/internal/password" "github.com/ubuntu/authd-oidc-brokers/internal/providers/info" "github.com/ubuntu/authd-oidc-brokers/internal/testutils" @@ -108,7 +109,7 @@ func TestNewSession(t *testing.T) { customHandlers: tc.customHandlers, }) - id, _, err := b.NewSession("test-user", "lang", "auth") + id, _, err := b.NewSession("test-user", "lang", sessionmode.Login) require.NoError(t, err, "NewSession should not have returned an error") gotOffline, err := b.IsOffline(id) @@ -170,7 +171,7 @@ func TestGetAuthenticationModes(t *testing.T) { wantErr bool }{ - // Auth Session + // Authentication session "Get_device_auth_qr_if_there_is_no_token": {}, "Get_newpassword_if_already_authenticated_with_device_auth_qr": {secondAuthStep: true}, "Get_password_and_device_auth_qr_if_token_exists": {tokenExists: true}, @@ -178,9 +179,9 @@ func TestGetAuthenticationModes(t *testing.T) { "Get_only_password_if_token_exists_and_provider_is_not_available": {tokenExists: true, providerAddress: "127.0.0.1:31310", unavailableProvider: true}, "Get_only_password_if_token_exists_and_provider_does_not_support_device_auth_qr": {tokenExists: true, providerAddress: "127.0.0.1:31311", deviceAuthUnsupported: true}, - // Passwd Session - "Get_only_password_if_token_exists_and_session_is_passwd": {sessionMode: "passwd", tokenExists: true}, - "Get_newpassword_if_already_authenticated_with_password_and_session_is_passwd": {sessionMode: "passwd", tokenExists: true, secondAuthStep: true}, + // Change password session + "Get_only_password_if_token_exists_and_session_is_passwd": {sessionMode: sessionmode.ChangePassword, tokenExists: true}, + "Get_newpassword_if_already_authenticated_with_password_and_session_is_passwd": {sessionMode: sessionmode.ChangePassword, tokenExists: true, secondAuthStep: true}, "Error_if_there_is_no_session": {sessionID: "-", wantErr: true}, @@ -191,15 +192,15 @@ func TestGetAuthenticationModes(t *testing.T) { "Error_if_expecting_newpassword_but_not_supported": {supportedLayouts: []string{"newpassword-without-entry"}, wantErr: true}, "Error_if_expecting_password_but_not_supported": {supportedLayouts: []string{"form-without-entry"}, wantErr: true}, - // Passwd session errors - "Error_if_session_is_passwd_but_token_does_not_exist": {sessionMode: "passwd", wantErr: true}, + // Change password session errors + "Error_if_session_is_passwd_but_token_does_not_exist": {sessionMode: sessionmode.ChangePassword, wantErr: true}, } for name, tc := range tests { t.Run(name, func(t *testing.T) { t.Parallel() if tc.sessionMode == "" { - tc.sessionMode = "auth" + tc.sessionMode = sessionmode.Login } cfg := &brokerForTestConfig{} @@ -331,9 +332,9 @@ func TestSelectAuthenticationMode(t *testing.T) { } b := newBrokerForTests(t, cfg) - sessionType := "auth" + sessionType := sessionmode.Login if tc.passwdSession { - sessionType = "passwd" + sessionType = sessionmode.ChangePassword } sessionID, _ := newSessionForTests(t, b, "", sessionType) @@ -522,7 +523,7 @@ func TestIsAuthenticated(t *testing.T) { t.Parallel() if tc.sessionMode == "" { - tc.sessionMode = "auth" + tc.sessionMode = sessionmode.Login } if tc.sessionOffline { @@ -1031,7 +1032,7 @@ func TestFetchUserInfo(t *testing.T) { } tc.token.issuer = defaultIssuerURL - sessionID, _, err := b.NewSession(tc.username, "lang", "auth") + sessionID, _, err := b.NewSession(tc.username, "lang", sessionmode.Login) require.NoError(t, err, "Setup: Failed to create session for the tests") cachedInfo := generateCachedInfo(t, tc.token) diff --git a/internal/broker/helper_test.go b/internal/broker/helper_test.go index f605814f..adc06da3 100644 --- a/internal/broker/helper_test.go +++ b/internal/broker/helper_test.go @@ -14,6 +14,7 @@ import ( "github.com/golang-jwt/jwt/v5" "github.com/stretchr/testify/require" "github.com/ubuntu/authd-oidc-brokers/internal/broker" + "github.com/ubuntu/authd-oidc-brokers/internal/broker/sessionmode" "github.com/ubuntu/authd-oidc-brokers/internal/providers" "github.com/ubuntu/authd-oidc-brokers/internal/providers/info" "github.com/ubuntu/authd-oidc-brokers/internal/testutils" @@ -118,7 +119,7 @@ func newSessionForTests(t *testing.T, b *broker.Broker, username, mode string) ( username = "test-user@email.com" } if mode == "" { - mode = "auth" + mode = sessionmode.Login } id, key, err := b.NewSession(username, "some lang", mode) diff --git a/internal/broker/sessionmode/consts.go b/internal/broker/sessionmode/consts.go new file mode 100644 index 00000000..69a64d6a --- /dev/null +++ b/internal/broker/sessionmode/consts.go @@ -0,0 +1,9 @@ +// Package sessionmode defines the session modes supported by the broker. +package sessionmode + +const ( + // Login is used when the session is for user login. + Login = "auth" + // ChangePassword is used when the session is for changing the user password. + ChangePassword = "passwd" +) diff --git a/internal/providers/msentraid/msentraid.go b/internal/providers/msentraid/msentraid.go index f7bf8f33..5aab30c2 100644 --- a/internal/providers/msentraid/msentraid.go +++ b/internal/providers/msentraid/msentraid.go @@ -17,6 +17,7 @@ import ( msgraphauth "github.com/microsoftgraph/msgraph-sdk-go-core/authentication" msgraphmodels "github.com/microsoftgraph/msgraph-sdk-go/models" "github.com/ubuntu/authd-oidc-brokers/internal/broker/authmodes" + "github.com/ubuntu/authd-oidc-brokers/internal/broker/sessionmode" "github.com/ubuntu/authd-oidc-brokers/internal/consts" providerErrors "github.com/ubuntu/authd-oidc-brokers/internal/providers/errors" "github.com/ubuntu/authd-oidc-brokers/internal/providers/info" @@ -286,7 +287,7 @@ func (p Provider) CurrentAuthenticationModesOffered( log.Debugf(context.Background(), "In CurrentAuthenticationModesOffered: sessionMode=%q, supportedAuthModes=%q, tokenExists=%t, providerReachable=%t, endpoints=%q, currentAuthStep=%d\n", sessionMode, supportedAuthModes, tokenExists, providerReachable, endpoints, currentAuthStep) var offeredModes []string switch sessionMode { - case "passwd": + case sessionmode.ChangePassword: if !tokenExists { return nil, errors.New("user has no cached token") } diff --git a/internal/providers/noprovider/noprovider.go b/internal/providers/noprovider/noprovider.go index 47ccf130..156c375f 100644 --- a/internal/providers/noprovider/noprovider.go +++ b/internal/providers/noprovider/noprovider.go @@ -8,6 +8,7 @@ import ( "github.com/coreos/go-oidc/v3/oidc" "github.com/ubuntu/authd-oidc-brokers/internal/broker/authmodes" + "github.com/ubuntu/authd-oidc-brokers/internal/broker/sessionmode" "github.com/ubuntu/authd-oidc-brokers/internal/providers/info" "golang.org/x/oauth2" ) @@ -47,7 +48,7 @@ func (p NoProvider) CurrentAuthenticationModesOffered( ) ([]string, error) { var offeredModes []string switch sessionMode { - case "passwd": + case sessionmode.ChangePassword: if !tokenExists { return nil, errors.New("user has no cached token") } From a6d3e7e3df3629ae9a9f356a31ea63c2ec489af0 Mon Sep 17 00:00:00 2001 From: Adrian Dombeck Date: Tue, 21 Jan 2025 12:24:33 +0100 Subject: [PATCH 2/4] Support new session mode names --- internal/broker/broker.go | 4 ++-- internal/broker/sessionmode/consts.go | 10 ++++++++-- internal/providers/msentraid/msentraid.go | 2 +- internal/providers/noprovider/noprovider.go | 2 +- 4 files changed, 12 insertions(+), 6 deletions(-) diff --git a/internal/broker/broker.go b/internal/broker/broker.go index f498a8b6..240f509c 100644 --- a/internal/broker/broker.go +++ b/internal/broker/broker.go @@ -400,7 +400,7 @@ func (b *Broker) generateUILayout(session *session, authModeID string) (map[stri case authmodes.NewPassword: label := "Create a local password" - if session.mode == sessionmode.ChangePassword { + if session.mode == sessionmode.ChangePassword || session.mode == sessionmode.ChangePasswordOld { label = "Update your local password" } @@ -597,7 +597,7 @@ func (b *Broker) handleIsAuthenticated(ctx context.Context, session *session, au authInfo.UserInfo = userInfo } - if session.mode == sessionmode.ChangePassword { + if session.mode == sessionmode.ChangePassword || session.mode == sessionmode.ChangePasswordOld { session.authInfo["auth_info"] = authInfo return AuthNext, nil } diff --git a/internal/broker/sessionmode/consts.go b/internal/broker/sessionmode/consts.go index 69a64d6a..2472e655 100644 --- a/internal/broker/sessionmode/consts.go +++ b/internal/broker/sessionmode/consts.go @@ -3,7 +3,13 @@ package sessionmode const ( // Login is used when the session is for user login. - Login = "auth" + Login = "login" + // LoginOld is the old name for the login session, which is now deprecated but still used by authd until all broker + // installations are updated. + LoginOld = "auth" // ChangePassword is used when the session is for changing the user password. - ChangePassword = "passwd" + ChangePassword = "change-password" + // ChangePasswordOld is the old name for the change-password session, which is now deprecated but still used by authd + // until all broker installations are updated. + ChangePasswordOld = "passwd" ) diff --git a/internal/providers/msentraid/msentraid.go b/internal/providers/msentraid/msentraid.go index 5aab30c2..9ecfef28 100644 --- a/internal/providers/msentraid/msentraid.go +++ b/internal/providers/msentraid/msentraid.go @@ -287,7 +287,7 @@ func (p Provider) CurrentAuthenticationModesOffered( log.Debugf(context.Background(), "In CurrentAuthenticationModesOffered: sessionMode=%q, supportedAuthModes=%q, tokenExists=%t, providerReachable=%t, endpoints=%q, currentAuthStep=%d\n", sessionMode, supportedAuthModes, tokenExists, providerReachable, endpoints, currentAuthStep) var offeredModes []string switch sessionMode { - case sessionmode.ChangePassword: + case sessionmode.ChangePassword, sessionmode.ChangePasswordOld: if !tokenExists { return nil, errors.New("user has no cached token") } diff --git a/internal/providers/noprovider/noprovider.go b/internal/providers/noprovider/noprovider.go index 156c375f..ac3bb429 100644 --- a/internal/providers/noprovider/noprovider.go +++ b/internal/providers/noprovider/noprovider.go @@ -48,7 +48,7 @@ func (p NoProvider) CurrentAuthenticationModesOffered( ) ([]string, error) { var offeredModes []string switch sessionMode { - case sessionmode.ChangePassword: + case sessionmode.ChangePassword, sessionmode.ChangePasswordOld: if !tokenExists { return nil, errors.New("user has no cached token") } From c9aeeebd3bcbbb238a2a5e08b22d2a524cda7fde Mon Sep 17 00:00:00 2001 From: Adrian Dombeck Date: Tue, 21 Jan 2025 14:54:22 +0100 Subject: [PATCH 3/4] Rename tests --- internal/broker/broker_test.go | 6 +++--- ...ated_with_password_and_session_is_for_changing_password} | 0 ...rd_if_token_exists_and_session_is_for_changing_password} | 0 3 files changed, 3 insertions(+), 3 deletions(-) rename internal/broker/testdata/golden/TestGetAuthenticationModes/{Get_newpassword_if_already_authenticated_with_password_and_session_is_passwd => Get_newpassword_if_already_authenticated_with_password_and_session_is_for_changing_password} (100%) rename internal/broker/testdata/golden/TestGetAuthenticationModes/{Get_only_password_if_token_exists_and_session_is_passwd => Get_only_password_if_token_exists_and_session_is_for_changing_password} (100%) diff --git a/internal/broker/broker_test.go b/internal/broker/broker_test.go index 44dc8a7e..78bd3b66 100644 --- a/internal/broker/broker_test.go +++ b/internal/broker/broker_test.go @@ -180,8 +180,8 @@ func TestGetAuthenticationModes(t *testing.T) { "Get_only_password_if_token_exists_and_provider_does_not_support_device_auth_qr": {tokenExists: true, providerAddress: "127.0.0.1:31311", deviceAuthUnsupported: true}, // Change password session - "Get_only_password_if_token_exists_and_session_is_passwd": {sessionMode: sessionmode.ChangePassword, tokenExists: true}, - "Get_newpassword_if_already_authenticated_with_password_and_session_is_passwd": {sessionMode: sessionmode.ChangePassword, tokenExists: true, secondAuthStep: true}, + "Get_only_password_if_token_exists_and_session_is_for_changing_password": {sessionMode: sessionmode.ChangePassword, tokenExists: true}, + "Get_newpassword_if_already_authenticated_with_password_and_session_is_for_changing_password": {sessionMode: sessionmode.ChangePassword, tokenExists: true, secondAuthStep: true}, "Error_if_there_is_no_session": {sessionID: "-", wantErr: true}, @@ -193,7 +193,7 @@ func TestGetAuthenticationModes(t *testing.T) { "Error_if_expecting_password_but_not_supported": {supportedLayouts: []string{"form-without-entry"}, wantErr: true}, // Change password session errors - "Error_if_session_is_passwd_but_token_does_not_exist": {sessionMode: sessionmode.ChangePassword, wantErr: true}, + "Error_if_session_is_for_changing_password_but_token_does_not_exist": {sessionMode: sessionmode.ChangePassword, wantErr: true}, } for name, tc := range tests { t.Run(name, func(t *testing.T) { diff --git a/internal/broker/testdata/golden/TestGetAuthenticationModes/Get_newpassword_if_already_authenticated_with_password_and_session_is_passwd b/internal/broker/testdata/golden/TestGetAuthenticationModes/Get_newpassword_if_already_authenticated_with_password_and_session_is_for_changing_password similarity index 100% rename from internal/broker/testdata/golden/TestGetAuthenticationModes/Get_newpassword_if_already_authenticated_with_password_and_session_is_passwd rename to internal/broker/testdata/golden/TestGetAuthenticationModes/Get_newpassword_if_already_authenticated_with_password_and_session_is_for_changing_password diff --git a/internal/broker/testdata/golden/TestGetAuthenticationModes/Get_only_password_if_token_exists_and_session_is_passwd b/internal/broker/testdata/golden/TestGetAuthenticationModes/Get_only_password_if_token_exists_and_session_is_for_changing_password similarity index 100% rename from internal/broker/testdata/golden/TestGetAuthenticationModes/Get_only_password_if_token_exists_and_session_is_passwd rename to internal/broker/testdata/golden/TestGetAuthenticationModes/Get_only_password_if_token_exists_and_session_is_for_changing_password From 84842438d4028d5e3e524cfca399ed09a844c862 Mon Sep 17 00:00:00 2001 From: Adrian Dombeck Date: Tue, 21 Jan 2025 14:55:40 +0100 Subject: [PATCH 4/4] Add test for old session mode value --- internal/broker/broker_test.go | 1 + ...ord_if_token_exists_and_session_mode_is_the_old_passwd_value | 2 ++ 2 files changed, 3 insertions(+) create mode 100644 internal/broker/testdata/golden/TestGetAuthenticationModes/Get_only_password_if_token_exists_and_session_mode_is_the_old_passwd_value diff --git a/internal/broker/broker_test.go b/internal/broker/broker_test.go index 78bd3b66..ce8578d9 100644 --- a/internal/broker/broker_test.go +++ b/internal/broker/broker_test.go @@ -182,6 +182,7 @@ func TestGetAuthenticationModes(t *testing.T) { // Change password session "Get_only_password_if_token_exists_and_session_is_for_changing_password": {sessionMode: sessionmode.ChangePassword, tokenExists: true}, "Get_newpassword_if_already_authenticated_with_password_and_session_is_for_changing_password": {sessionMode: sessionmode.ChangePassword, tokenExists: true, secondAuthStep: true}, + "Get_only_password_if_token_exists_and_session_mode_is_the_old_passwd_value": {sessionMode: sessionmode.ChangePasswordOld, tokenExists: true}, "Error_if_there_is_no_session": {sessionID: "-", wantErr: true}, diff --git a/internal/broker/testdata/golden/TestGetAuthenticationModes/Get_only_password_if_token_exists_and_session_mode_is_the_old_passwd_value b/internal/broker/testdata/golden/TestGetAuthenticationModes/Get_only_password_if_token_exists_and_session_mode_is_the_old_passwd_value new file mode 100644 index 00000000..e4e7cb1d --- /dev/null +++ b/internal/broker/testdata/golden/TestGetAuthenticationModes/Get_only_password_if_token_exists_and_session_mode_is_the_old_passwd_value @@ -0,0 +1,2 @@ +- id: password + label: Local Password Authentication