brokers: use providerID for user identification#1546
Conversation
There was a problem hiding this comment.
Pull request overview
This PR migrates remote user identification from unstable attributes (email / preferred_username) to a stable provider-assigned identifier (sub, or oid for MS Entra ID), enabling reliable matching across username/email changes and supporting upcoming username-shortening features.
Changes:
- Persist
subin the authd users DB (schema v3) with a partial unique index, and expose lookup bysub. - Update authd user update logic to resolve existing users by
suband handle sub-matched renames (including local group membership cleanup). - Bump broker API to v3, passing
subinNewSession/DeleteUser, and migrate OIDC-broker cache directories from username-keyed to sub-keyed storage.
Reviewed changes
Copilot reviewed 159 out of 203 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/users/types/types.go | Add Sub field to broker-returned UserInfo for stable identity matching. |
| internal/users/manager.go | Use sub for user lookup/rename handling; persist sub into DB; add SubForUser. |
| internal/users/defs.go | Populate types.UserInfo.Sub from DB rows. |
| internal/users/db/users.go | Add sub column support to queries/inserts/updates; add UserBySub. |
| internal/users/db/update.go | Allow UID/name change when sub matches; include sub in user scans. |
| internal/users/db/sql/create_schema.sql | Extend schema with sub column and partial unique index. |
| internal/users/db/migration.go | Add migration to introduce sub column + partial unique index (schema v3). |
| internal/testutils/broker.go | Update broker mock API to v3 (sub parameter) and userinfo JSON to sub. |
| internal/services/user/user.go | Pass stored sub to broker DeleteUser for sub-keyed cache cleanup. |
| internal/services/pam/pam.go | Look up sub and pass it into broker session creation (API v3). |
| internal/brokers/manager.go | Extend broker manager NewSession signature to include sub. |
| internal/brokers/manager_test.go | Update tests for new NewSession(..., sub) signature. |
| internal/brokers/localbroker.go | Update local broker interface implementation to accept sub. |
| internal/brokers/dbusbroker.go | Implement v2/v3 dispatch for NewSession/DeleteUser based on interface version. |
| internal/brokers/dbusbroker_test.go | Expect v3 interface selection and adjust version-related test cases. |
| internal/brokers/broker.go | Thread sub through broker interface and document v3 behavior. |
| internal/brokers/broker_test.go | Update tests for DeleteUser(..., sub) signature. |
| internal/brokers/testdata/golden/TestUserPreCheck/Successfully_pre-check_user | Update golden userinfo JSON key from uuid to sub. |
| examplebroker/dbus.go | Adapt example broker to new internal NewSession/DeleteUser signatures (passing empty sub). |
| examplebroker/broker.go | Accept sub in example broker methods (unused) to satisfy new interface. |
| authd-oidc-brokers/internal/token/token_test.go | Rename cached user identifier field from UUID to Sub in tests. |
| authd-oidc-brokers/internal/providers/msentraid/msentraid.go | Use Entra oid as stable identifier; error if missing. |
| authd-oidc-brokers/internal/providers/info/info.go | Rename provider user identifier field to Sub; update constructor. |
| authd-oidc-brokers/internal/providers/info/info_test.go | Update tests for Sub field/parameter naming. |
| authd-oidc-brokers/internal/dbusservice/methods.go | Add v3 DBus methods accepting sub (InterfaceV3 wrapper). |
| authd-oidc-brokers/internal/dbusservice/dbusservice.go | Export com.ubuntu.authd.Broker3 interface and v3 introspection XML. |
| authd-oidc-brokers/internal/broker/broker.go | Implement sub-keyed cache lookup/migration; accept sub in NewSession/DeleteUser. |
| authd-oidc-brokers/internal/broker/broker_test.go | Update broker tests for new signatures and cache migration side-effects. |
| authd-oidc-brokers/internal/broker/helper_test.go | Update helpers to call NewSession(..., sub) and cached user info field to Sub. |
| internal/users/testdata/golden/TestUpdateUser/User_private_group_GID_preserved_across_logins | Bump golden DB schema version to v3. |
| internal/users/testdata/golden/TestUpdateUser/UID_does_not_change_if_user_already_exists | Bump golden DB schema version to v3. |
| internal/users/testdata/golden/TestUpdateUser/Successfully_update_user_updating_local_groups_with_changes | Bump golden DB schema version to v3. |
| internal/users/testdata/golden/TestUpdateUser/Successfully_update_user_updating_local_groups | Bump golden DB schema version to v3. |
| internal/users/testdata/golden/TestUpdateUser/Successfully_update_user | Bump golden DB schema version to v3. |
| internal/users/testdata/golden/TestUpdateUser/Removing_last_user_from_a_group_keeps_the_group_record | Bump golden DB schema version to v3. |
| internal/users/testdata/golden/TestUpdateUser/GID_does_not_change_if_group_with_same_UGID_exists | Bump golden DB schema version to v3. |
| internal/users/testdata/golden/TestUpdateUser/GID_does_not_change_if_group_with_same_name_and_empty_UGID_exists | Bump golden DB schema version to v3. |
| internal/users/testdata/golden/TestUpdateUser/Allow_login_with_existing_group_on_system | Bump golden DB schema version to v3. |
| internal/users/testdata/golden/TestUpdateBrokerForUser/Successfully_update_broker_for_user | Bump golden DB schema version to v3. |
| internal/users/testdata/golden/TestUnlockUser/Successfully_enable_user | Bump golden DB schema version to v3. |
| internal/users/testdata/golden/TestNewManager/Warns_creating_manager_with_partially_invalid_UID_ranges | Bump golden DB schema version to v3. |
| internal/users/testdata/golden/TestNewManager/Warns_creating_manager_with_partially_invalid_GID_ranges | Bump golden DB schema version to v3. |
| internal/users/testdata/golden/TestNewManager/Successfully_create_manager_with_UID_range_next_to_systemd_dynamic_users | Bump golden DB schema version to v3. |
| internal/users/testdata/golden/TestNewManager/Successfully_create_manager_with_GID_range_next_to_systemd_dynamic_groups | Bump golden DB schema version to v3. |
| internal/users/testdata/golden/TestNewManager/Successfully_create_manager_with_default_config | Bump golden DB schema version to v3. |
| internal/users/testdata/golden/TestNewManager/Successfully_create_manager_with_custom_config | Bump golden DB schema version to v3. |
| internal/users/testdata/golden/TestLockUser/Successfully_lock_user | Bump golden DB schema version to v3. |
| internal/users/testdata/golden/TestDeleteUser/Successfully_delete_user_removes_them_from_local_groups | Bump golden DB schema version to v3. |
| internal/users/testdata/golden/TestDeleteUser/Successfully_delete_user_keeps_primary_group_if_other_users_still_use_it | Bump golden DB schema version to v3. |
| internal/users/testdata/golden/TestDeleteUser/Successfully_delete_user_keeps_other_users_in_shared_group | Bump golden DB schema version to v3. |
| internal/users/testdata/golden/TestDeleteUser/Successfully_delete_user_and_remove_home | Bump golden DB schema version to v3. |
| internal/users/testdata/golden/TestDeleteUser/Successfully_delete_user | Bump golden DB schema version to v3. |
| internal/users/testdata/golden/TestDeleteGroup/Successfully_delete_shared_group_leaves_other_groups_intact | Bump golden DB schema version to v3. |
| internal/users/testdata/golden/TestDeleteGroup/Successfully_delete_group_keeps_its_members_in_the_db | Bump golden DB schema version to v3. |
| internal/users/testdata/golden/TestCompareNewUserInfoWithDB/Compare_all_valid_users/userwithoutbroker-from-getOldUserInfoFromDB | Add sub: \"\" to golden userinfo output. |
| internal/users/testdata/golden/TestCompareNewUserInfoWithDB/Compare_all_valid_users/user3-from-getOldUserInfoFromDB | Add sub: \"\" to golden userinfo output. |
| internal/users/testdata/golden/TestCompareNewUserInfoWithDB/Compare_all_valid_users/user2-from-getOldUserInfoFromDB | Add sub: \"\" to golden userinfo output. |
| internal/users/testdata/golden/TestCompareNewUserInfoWithDB/Compare_all_valid_users/user1-from-getOldUserInfoFromDB | Add sub: \"\" to golden userinfo output. |
| internal/users/testdata/golden/TestCompareNewUserInfoWithDB/Compare_all_not_matching_users/userwithoutbroker-from-getOldUserInfoFromDB | Add sub: \"\" to golden userinfo output. |
| internal/users/testdata/golden/TestCompareNewUserInfoWithDB/Compare_all_not_matching_users/user3-from-getOldUserInfoFromDB | Add sub: \"\" to golden userinfo output. |
| internal/users/testdata/golden/TestCompareNewUserInfoWithDB/Compare_all_not_matching_users/user2-from-getOldUserInfoFromDB | Add sub: \"\" to golden userinfo output. |
| internal/users/testdata/golden/TestCompareNewUserInfoWithDB/Compare_all_not_matching_users/user1-from-getOldUserInfoFromDB | Add sub: \"\" to golden userinfo output. |
| internal/users/db/testdata/golden/TestUpdateUserEntry/Update_user_does_not_change_shell_if_it_exists | Bump golden DB schema version to v3. |
| internal/users/db/testdata/golden/TestUpdateUserEntry/Update_user_does_not_change_homedir_if_it_exists | Bump golden DB schema version to v3. |
| internal/users/db/testdata/golden/TestUpdateUserEntry/Update_user_by_renaming_a_group | Bump golden DB schema version to v3. |
| internal/users/db/testdata/golden/TestUpdateUserEntry/Update_user_by_removing_optional_gecos_field_if_not_set | Bump golden DB schema version to v3. |
| internal/users/db/testdata/golden/TestUpdateUserEntry/Update_user_by_changing_attributes | Bump golden DB schema version to v3. |
| internal/users/db/testdata/golden/TestUpdateUserEntry/Update_user_by_adding_a_new_local_group | Bump golden DB schema version to v3. |
| internal/users/db/testdata/golden/TestUpdateUserEntry/Update_user_by_adding_a_new_group | Bump golden DB schema version to v3. |
| internal/users/db/testdata/golden/TestUpdateUserEntry/Update_user_by_adding_a_new_default_group | Bump golden DB schema version to v3. |
| internal/users/db/testdata/golden/TestUpdateUserEntry/Update_only_user_even_if_we_have_multiple_of_them | Bump golden DB schema version to v3. |
| internal/users/db/testdata/golden/TestUpdateUserEntry/Remove_user_from_a_group_still_part_from_another_user | Bump golden DB schema version to v3. |
| internal/users/db/testdata/golden/TestUpdateUserEntry/Remove_group_from_user | Bump golden DB schema version to v3. |
| internal/users/db/testdata/golden/TestUpdateUserEntry/Insert_new_user_without_optional_gecos_field | Bump golden DB schema version to v3. |
| internal/users/db/testdata/golden/TestUpdateUserEntry/Insert_new_user | Bump golden DB schema version to v3. |
| internal/users/db/testdata/golden/TestUpdateUserEntry/Add_user_to_group_from_another_user | Bump golden DB schema version to v3. |
| internal/users/db/testdata/golden/TestSetUserID/Set_user_id_for_existing_user | Bump golden DB schema version to v3. |
| internal/users/db/testdata/golden/TestSetGroupID/Set_group_id_for_existing_group | Bump golden DB schema version to v3. |
| internal/users/db/testdata/golden/TestNew/New_without_any_initialized_database | Bump golden DB schema version to v3. |
| internal/users/db/testdata/golden/TestNew/New_with_already_existing_database | Bump golden DB schema version to v3. |
| internal/users/db/testdata/golden/TestMigrationToLowercaseUserAndGroupNamesWithSymlinkedPreviousBackup/db | Bump golden DB schema version to v3. |
| internal/users/db/testdata/golden/TestMigrationToLowercaseUserAndGroupNamesWithSymlinkedGroupFile/db | Bump golden DB schema version to v3. |
| internal/users/db/testdata/golden/TestMigrationToLowercaseUserAndGroupNamesWithPreviousBackup/db | Bump golden DB schema version to v3. |
| internal/users/db/testdata/golden/TestMigrationToLowercaseUserAndGroupNamesWithBackupFailure/db | Bump golden DB schema version to v3. |
| internal/users/db/testdata/golden/TestMigrationToLowercaseUserAndGroupNamesEmptyDB/db | Bump golden DB schema version to v3. |
| internal/users/db/testdata/golden/TestMigrationToLowercaseUserAndGroupNamesAlreadyUpdated/db | Bump golden DB schema version to v3. |
| internal/users/db/testdata/golden/TestMigrationToLowercaseUserAndGroupNames/db | Bump golden DB schema version to v3. |
| internal/users/db/testdata/golden/TestMigrationAddLockedColumnToUsersTable | Bump golden DB schema version to v3. |
| internal/users/db/testdata/golden/TestDeleteUser/Deleting_last_user_from_a_group_keeps_the_group_record | Bump golden DB schema version to v3. |
| internal/users/db/testdata/golden/TestDeleteUser/Deleting_existing_user_keeps_other_group_members_intact | Bump golden DB schema version to v3. |
| internal/users/db/testdata/golden/TestDeleteGroup/Deleting_sole_group_of_a_user_removes_group_and_memberships_but_keeps_user | Bump golden DB schema version to v3. |
| internal/users/db/testdata/golden/TestDeleteGroup/Deleting_shared_group_removes_only_that_group_and_its_memberships | Bump golden DB schema version to v3. |
| internal/services/user/testdata/golden/TestUnlockUser/Successfully_unlock_user_with_uppercase | Bump golden DB schema version to v3. |
| internal/services/user/testdata/golden/TestUnlockUser/Successfully_unlock_user | Bump golden DB schema version to v3. |
| internal/services/user/testdata/golden/TestLockUser/Successfully_lock_user_with_uppercase | Bump golden DB schema version to v3. |
| internal/services/user/testdata/golden/TestLockUser/Successfully_lock_user | Bump golden DB schema version to v3. |
| internal/services/user/testdata/golden/TestDeleteUser/Warning_when_broker_not_found/database | Bump golden DB schema version to v3. |
| internal/services/user/testdata/golden/TestDeleteUser/Warning_when_broker_fails_to_delete/database | Bump golden DB schema version to v3. |
| internal/services/user/testdata/golden/TestDeleteUser/Successfully_delete_user/database | Bump golden DB schema version to v3. |
| internal/services/user/testdata/golden/TestDeleteUser/Successfully_delete_user_with_uppercase/database | Bump golden DB schema version to v3. |
| internal/services/user/testdata/golden/TestDeleteGroup/Successfully_delete_group_with_uppercase | Bump golden DB schema version to v3. |
| internal/services/user/testdata/golden/TestDeleteGroup/Successfully_delete_group | Bump golden DB schema version to v3. |
| internal/services/pam/testdata/golden/TestSetDefaultBrokerForUser/Username_is_case_insensitive/cache.db | Bump golden DB schema version to v3. |
| internal/services/pam/testdata/golden/TestSetDefaultBrokerForUser/Update_default_broker_for_existing_user_with_a_broker/cache.db | Bump golden DB schema version to v3. |
| internal/services/pam/testdata/golden/TestSetDefaultBrokerForUser/Set_default_broker_for_existing_user_with_no_broker/cache.db | Bump golden DB schema version to v3. |
| internal/services/pam/testdata/golden/TestIsAuthenticated/Update_local_groups/cache.db | Bump golden DB schema version to v3. |
| internal/services/pam/testdata/golden/TestIsAuthenticated/Update_existing_DB_on_success/cache.db | Bump golden DB schema version to v3. |
| internal/services/pam/testdata/golden/TestIsAuthenticated/Successfully_authenticate/cache.db | Bump golden DB schema version to v3. |
| internal/services/pam/testdata/golden/TestIsAuthenticated/Successfully_authenticate_with_groups_with_uppercase/cache.db | Bump golden DB schema version to v3. |
| internal/services/pam/testdata/golden/TestIsAuthenticated/Successfully_authenticate_user_with_uppercase/cache.db | Bump golden DB schema version to v3. |
| internal/services/pam/testdata/golden/TestIsAuthenticated/Successfully_authenticate_if_first_call_is_canceled/cache.db | Bump golden DB schema version to v3. |
| internal/services/pam/testdata/golden/TestIsAuthenticated/Successfully_authenticate_after_calling_second_time_without_cancelling/cache.db | Bump golden DB schema version to v3. |
| internal/services/pam/testdata/golden/TestIsAuthenticated/Error_when_user_is_locked/cache.db | Bump golden DB schema version to v3. |
| internal/services/pam/testdata/golden/TestIsAuthenticated/Error_when_there_is_no_broker/cache.db | Bump golden DB schema version to v3. |
| internal/services/pam/testdata/golden/TestIsAuthenticated/Error_when_sessionID_is_empty/cache.db | Bump golden DB schema version to v3. |
| internal/services/pam/testdata/golden/TestIsAuthenticated/Error_when_not_root/cache.db | Bump golden DB schema version to v3. |
| internal/services/pam/testdata/golden/TestIsAuthenticated/Error_when_broker_returns_invalid_userinfo/cache.db | Bump golden DB schema version to v3. |
| internal/services/pam/testdata/golden/TestIsAuthenticated/Error_when_broker_returns_invalid_data/cache.db | Bump golden DB schema version to v3. |
| internal/services/pam/testdata/golden/TestIsAuthenticated/Error_when_broker_returns_invalid_access/cache.db | Bump golden DB schema version to v3. |
| internal/services/pam/testdata/golden/TestIsAuthenticated/Error_when_authenticating/cache.db | Bump golden DB schema version to v3. |
| internal/services/pam/testdata/golden/TestIsAuthenticated/Error_on_updating_local_groups_with_unexisting_file/cache.db | Bump golden DB schema version to v3. |
| internal/services/pam/testdata/golden/TestIsAuthenticated/Error_on_empty_data_even_if_granted/cache.db | Bump golden DB schema version to v3. |
| internal/services/pam/testdata/golden/TestIsAuthenticated/Denies_authentication_when_broker_times_out/cache.db | Bump golden DB schema version to v3. |
| internal/services/pam/testdata/golden/TestIDGeneration/Generate_ID/cache.db | Bump golden DB schema version to v3. |
| authd-oidc-brokers/internal/broker/testdata/golden/TestUserPreCheck/Successfully_allow_username_with_matching_allowed_suffix | Update golden userinfo JSON key from uuid to sub. |
| authd-oidc-brokers/internal/broker/testdata/golden/TestUserPreCheck/Successfully_allow_username_that_matches_at_least_one_allowed_suffix | Update golden userinfo JSON key from uuid to sub. |
| authd-oidc-brokers/internal/broker/testdata/golden/TestUserPreCheck/Successfully_allow_username_ignoring_empty_string_in_config | Update golden userinfo JSON key from uuid to sub. |
| authd-oidc-brokers/internal/broker/testdata/golden/TestUserPreCheck/Successfully_allow_username_if_suffix_is_allow_all | Update golden userinfo JSON key from uuid to sub. |
| authd-oidc-brokers/internal/broker/testdata/golden/TestUserPreCheck/Successfully_allow_username_if_suffix_has_asterisk | Update golden userinfo JSON key from uuid to sub. |
| authd-oidc-brokers/internal/broker/testdata/golden/TestUserPreCheck/Return_userinfo_with_correct_homedir_after_precheck | Update golden userinfo JSON key from uuid to sub. |
| authd-oidc-brokers/internal/broker/testdata/golden/TestIsAuthenticated/Successfully_authenticate_user_with_password/first_call | Update golden auth response JSON key from uuid to sub. |
| authd-oidc-brokers/internal/broker/testdata/golden/TestIsAuthenticated/Successfully_authenticate_user_with_password/data/provider_url/test-user-id/token.json | Add/update golden cached token file content. |
| authd-oidc-brokers/internal/broker/testdata/golden/TestIsAuthenticated/Successfully_authenticate_user_with_password/data/provider_url/test-user-id/password | Add/update golden cached password file content. |
| authd-oidc-brokers/internal/broker/testdata/golden/TestIsAuthenticated/Successfully_authenticate_with_device_auth_when_provider_uses_thin_id_token/second_call | Update golden auth response JSON key from uuid to sub. |
| authd-oidc-brokers/internal/broker/testdata/golden/TestIsAuthenticated/Successfully_authenticate_with_device_auth_when_provider_uses_thin_id_token/data/provider_url/test-user-id/token.json | Add/update golden cached token file content. |
| authd-oidc-brokers/internal/broker/testdata/golden/TestIsAuthenticated/Successfully_authenticate_with_device_auth_when_provider_uses_thin_id_token/data/provider_url/test-user-id/password | Add/update golden cached password file content. |
| authd-oidc-brokers/internal/broker/testdata/golden/TestIsAuthenticated/Successfully_authenticate_user_with_device_auth_and_newpassword/second_call | Update golden auth response JSON key from uuid to sub. |
| authd-oidc-brokers/internal/broker/testdata/golden/TestIsAuthenticated/Successfully_authenticate_user_with_device_auth_and_newpassword/data/provider_url/test-user-id/token.json | Add/update golden cached token file content. |
| authd-oidc-brokers/internal/broker/testdata/golden/TestIsAuthenticated/Successfully_authenticate_user_with_device_auth_and_newpassword/data/provider_url/test-user-id/password | Add/update golden cached password file content. |
| authd-oidc-brokers/internal/broker/testdata/golden/TestIsAuthenticated/Owner_extra_groups_configured/first_call | Update golden auth response JSON key from uuid to sub. |
| authd-oidc-brokers/internal/broker/testdata/golden/TestIsAuthenticated/Owner_extra_groups_configured/data/provider_url/test-user-id/token.json | Add/update golden cached token file content. |
| authd-oidc-brokers/internal/broker/testdata/golden/TestIsAuthenticated/Owner_extra_groups_configured/data/provider_url/test-user-id/password | Add/update golden cached password file content. |
| authd-oidc-brokers/internal/broker/testdata/golden/TestIsAuthenticated/Owner_extra_groups_configured_but_user_does_not_become_owner/first_call | Update golden auth response JSON key from uuid to sub. |
| authd-oidc-brokers/internal/broker/testdata/golden/TestIsAuthenticated/Owner_extra_groups_configured_but_user_does_not_become_owner/data/provider_url/test-user-id/token.json | Add/update golden cached token file content. |
| authd-oidc-brokers/internal/broker/testdata/golden/TestIsAuthenticated/Owner_extra_groups_configured_but_user_does_not_become_owner/data/provider_url/test-user-id/password | Add/update golden cached password file content. |
| authd-oidc-brokers/internal/broker/testdata/golden/TestIsAuthenticated/Extra_groups_configured/first_call | Update golden auth response JSON key from uuid to sub. |
| authd-oidc-brokers/internal/broker/testdata/golden/TestIsAuthenticated/Extra_groups_configured/data/provider_url/test-user-id/token.json | Add/update golden cached token file content. |
| authd-oidc-brokers/internal/broker/testdata/golden/TestIsAuthenticated/Extra_groups_configured/data/provider_url/test-user-id/password | Add/update golden cached password file content. |
| authd-oidc-brokers/internal/broker/testdata/golden/TestIsAuthenticated/Extra_and_owner_extra_groups_configured_with_existing_extra_group_in_cached_user_info/first_call | Update golden auth response JSON key from uuid to sub. |
| authd-oidc-brokers/internal/broker/testdata/golden/TestIsAuthenticated/Extra_and_owner_extra_groups_configured_but_already_in_cached_user_info/first_call | Update golden auth response JSON key from uuid to sub. |
| authd-oidc-brokers/internal/broker/testdata/golden/TestIsAuthenticated/Authenticating_with_qrcode_reacquires_token/second_call | Update golden auth response JSON key from uuid to sub. |
| authd-oidc-brokers/internal/broker/testdata/golden/TestIsAuthenticated/Authenticating_with_qrcode_reacquires_token/data/provider_url/test-user-id/token.json | Add/update golden cached token file content. |
| authd-oidc-brokers/internal/broker/testdata/golden/TestIsAuthenticated/Authenticating_with_qrcode_reacquires_token/data/provider_url/test-user-id/password | Add/update golden cached password file content. |
| authd-oidc-brokers/internal/broker/testdata/golden/TestIsAuthenticated/Authenticating_with_password_when_provider_supports_device_registration/first_call | Update golden auth response JSON key from uuid to sub. |
| authd-oidc-brokers/internal/broker/testdata/golden/TestIsAuthenticated/Authenticating_with_password_when_provider_supports_device_registration/data/provider_url/test-user-id/token.json | Add/update golden cached token file content. |
| authd-oidc-brokers/internal/broker/testdata/golden/TestIsAuthenticated/Authenticating_with_password_when_provider_supports_device_registration/data/provider_url/test-user-id/password | Add/update golden cached password file content. |
| authd-oidc-brokers/internal/broker/testdata/golden/TestIsAuthenticated/Authenticating_with_password_when_provider_authentication_is_forced/first_call | Update golden auth response JSON key from uuid to sub. |
| authd-oidc-brokers/internal/broker/testdata/golden/TestIsAuthenticated/Authenticating_with_password_when_provider_authentication_is_forced/data/provider_url/test-user-id/token.json | Add/update golden cached token file content. |
| authd-oidc-brokers/internal/broker/testdata/golden/TestIsAuthenticated/Authenticating_with_password_when_provider_authentication_is_forced/data/provider_url/test-user-id/password | Add/update golden cached password file content. |
| authd-oidc-brokers/internal/broker/testdata/golden/TestIsAuthenticated/Authenticating_with_password_still_allowed_if_token_is_expired_and_server_is_unreachable/first_call | Update golden auth response JSON key from uuid to sub. |
| authd-oidc-brokers/internal/broker/testdata/golden/TestIsAuthenticated/Authenticating_with_password_still_allowed_if_server_is_unreachable/first_call | Update golden auth response JSON key from uuid to sub. |
| authd-oidc-brokers/internal/broker/testdata/golden/TestIsAuthenticated/Authenticating_with_password_still_allowed_if_no_refresh_token_and_server_is_unreachable/first_call | Update golden auth response JSON key from uuid to sub. |
| authd-oidc-brokers/internal/broker/testdata/golden/TestIsAuthenticated/Authenticating_with_password_skips_token_refresh_network_error/first_call | Update golden auth response JSON key from uuid to sub. |
| authd-oidc-brokers/internal/broker/testdata/golden/TestIsAuthenticated/Authenticating_with_password_refreshes_groups/first_call | Update golden auth response JSON key from uuid to sub. |
| authd-oidc-brokers/internal/broker/testdata/golden/TestIsAuthenticated/Authenticating_with_password_refreshes_groups/data/provider_url/test-user-id/token.json | Add/update golden cached token file content. |
| authd-oidc-brokers/internal/broker/testdata/golden/TestIsAuthenticated/Authenticating_with_password_refreshes_groups/data/provider_url/test-user-id/password | Add/update golden cached password file content. |
| authd-oidc-brokers/internal/broker/testdata/golden/TestIsAuthenticated/Authenticating_with_password_refreshes_expired_token/first_call | Update golden auth response JSON key from uuid to sub. |
| authd-oidc-brokers/internal/broker/testdata/golden/TestIsAuthenticated/Authenticating_with_password_refreshes_expired_token/data/provider_url/test-user-id/token.json | Add/update golden cached token file content. |
| authd-oidc-brokers/internal/broker/testdata/golden/TestIsAuthenticated/Authenticating_with_password_refreshes_expired_token/data/provider_url/test-user-id/password | Add/update golden cached password file content. |
| authd-oidc-brokers/internal/broker/testdata/golden/TestIsAuthenticated/Authenticating_with_password_keeps_old_groups_if_session_is_offline/first_call | Update golden auth response JSON key from uuid to sub. |
| authd-oidc-brokers/internal/broker/testdata/golden/TestIsAuthenticated/Authenticating_with_password_keeps_old_groups_if_fetching_groups_fails/first_call | Update golden auth response JSON key from uuid to sub. |
| authd-oidc-brokers/internal/broker/testdata/golden/TestIsAuthenticated/Authenticating_with_password_keeps_old_groups_if_fetching_groups_fails/data/provider_url/test-user-id/token.json | Add/update golden cached token file content. |
| authd-oidc-brokers/internal/broker/testdata/golden/TestIsAuthenticated/Authenticating_with_password_keeps_old_groups_if_fetching_groups_fails/data/provider_url/test-user-id/password | Add/update golden cached password file content. |
| authd-oidc-brokers/internal/broker/testdata/golden/TestIsAuthenticated/Authenticating_with_device_auth_when_provider_supports_device_registration/second_call | Update golden auth response JSON key from uuid to sub. |
| authd-oidc-brokers/internal/broker/testdata/golden/TestIsAuthenticated/Authenticating_with_device_auth_when_provider_supports_device_registration/data/provider_url/test-user-id/token.json | Add/update golden cached token file content. |
| authd-oidc-brokers/internal/broker/testdata/golden/TestIsAuthenticated/Authenticating_with_device_auth_when_provider_supports_device_registration/data/provider_url/test-user-id/password | Add/update golden cached password file content. |
| authd-oidc-brokers/internal/broker/testdata/golden/TestIsAuthenticated/Authenticating_when_the_auth_data_secret_field_uses_the_old_name/first_call | Update golden auth response JSON key from uuid to sub. |
| authd-oidc-brokers/internal/broker/testdata/golden/TestIsAuthenticated/Authenticating_when_the_auth_data_secret_field_uses_the_old_name/data/provider_url/test-user-id/token.json | Add/update golden cached token file content. |
| authd-oidc-brokers/internal/broker/testdata/golden/TestIsAuthenticated/Authenticating_when_the_auth_data_secret_field_uses_the_old_name/data/provider_url/test-user-id/password | Add/update golden cached password file content. |
| authd-oidc-brokers/internal/broker/testdata/golden/TestIsAuthenticated/Authenticating_still_allowed_if_token_is_missing_scopes/second_call | Update golden auth response JSON key from uuid to sub. |
| authd-oidc-brokers/internal/broker/testdata/golden/TestIsAuthenticated/Authenticating_still_allowed_if_token_is_missing_scopes/data/provider_url/test-user-id/token.json | Add/update golden cached token file content. |
| authd-oidc-brokers/internal/broker/testdata/golden/TestIsAuthenticated/Authenticating_still_allowed_if_token_is_missing_scopes/data/provider_url/test-user-id/password | Add/update golden cached password file content. |
| authd-oidc-brokers/internal/broker/testdata/golden/TestConcurrentIsAuthenticated/First_auth_starts_first_then_second_starts_and_first_finishes/first_auth | Update golden auth response JSON key from uuid to sub. |
| authd-oidc-brokers/internal/broker/testdata/golden/TestConcurrentIsAuthenticated/First_auth_starts_first_then_second_starts_and_first_finishes/second_auth | Update golden auth response JSON key from uuid to sub. |
| authd-oidc-brokers/internal/broker/testdata/golden/TestConcurrentIsAuthenticated/First_auth_starts_first_then_second_starts_and_first_finishes/data/provider_url/user1/token.json | Add/update golden cached token file content. |
| authd-oidc-brokers/internal/broker/testdata/golden/TestConcurrentIsAuthenticated/First_auth_starts_first_then_second_starts_and_first_finishes/data/provider_url/user1/password | Add/update golden cached password file content. |
| authd-oidc-brokers/internal/broker/testdata/golden/TestConcurrentIsAuthenticated/First_auth_starts_first_then_second_starts_and_first_finishes/data/provider_url/user2/token.json | Add/update golden cached token file content. |
| authd-oidc-brokers/internal/broker/testdata/golden/TestConcurrentIsAuthenticated/First_auth_starts_first_then_second_starts_and_first_finishes/data/provider_url/user2/password | Add/update golden cached password file content. |
| authd-oidc-brokers/internal/broker/testdata/golden/TestConcurrentIsAuthenticated/First_auth_starts_first_but_second_finishes_first/first_auth | Update golden auth response JSON key from uuid to sub. |
| authd-oidc-brokers/internal/broker/testdata/golden/TestConcurrentIsAuthenticated/First_auth_starts_first_but_second_finishes_first/second_auth | Update golden auth response JSON key from uuid to sub. |
| authd-oidc-brokers/internal/broker/testdata/golden/TestConcurrentIsAuthenticated/First_auth_starts_first_but_second_finishes_first/data/provider_url/user1/token.json | Add/update golden cached token file content. |
| authd-oidc-brokers/internal/broker/testdata/golden/TestConcurrentIsAuthenticated/First_auth_starts_first_but_second_finishes_first/data/provider_url/user1/password | Add/update golden cached password file content. |
| authd-oidc-brokers/internal/broker/testdata/golden/TestConcurrentIsAuthenticated/First_auth_starts_first_but_second_finishes_first/data/provider_url/user2/token.json | Add/update golden cached token file content. |
| authd-oidc-brokers/internal/broker/testdata/golden/TestConcurrentIsAuthenticated/First_auth_starts_first_but_second_finishes_first/data/provider_url/user2/password | Add/update golden cached password file content. |
| authd-oidc-brokers/internal/broker/testdata/golden/TestConcurrentIsAuthenticated/First_auth_starts_first_but_second_finishes_first_and_is_registered_as_the_owner/second_auth | Update golden auth response JSON key from uuid to sub. |
| authd-oidc-brokers/internal/broker/testdata/golden/TestConcurrentIsAuthenticated/First_auth_starts_first_but_second_finishes_first_and_is_registered_as_the_owner/data/provider_url/user2/token.json | Add/update golden cached token file content. |
| authd-oidc-brokers/internal/broker/testdata/golden/TestConcurrentIsAuthenticated/First_auth_starts_first_but_second_finishes_first_and_is_registered_as_the_owner/data/provider_url/user2/password | Add/update golden cached password file content. |
| authd-oidc-brokers/internal/broker/testdata/golden/TestConcurrentIsAuthenticated/First_auth_starts_and_finishes_before_second/first_auth | Update golden auth response JSON key from uuid to sub. |
| authd-oidc-brokers/internal/broker/testdata/golden/TestConcurrentIsAuthenticated/First_auth_starts_and_finishes_before_second/second_auth | Update golden auth response JSON key from uuid to sub. |
| authd-oidc-brokers/internal/broker/testdata/golden/TestConcurrentIsAuthenticated/First_auth_starts_and_finishes_before_second/data/provider_url/user1/token.json | Add/update golden cached token file content. |
| authd-oidc-brokers/internal/broker/testdata/golden/TestConcurrentIsAuthenticated/First_auth_starts_and_finishes_before_second/data/provider_url/user1/password | Add/update golden cached password file content. |
| authd-oidc-brokers/internal/broker/testdata/golden/TestConcurrentIsAuthenticated/First_auth_starts_and_finishes_before_second/data/provider_url/user2/token.json | Add/update golden cached token file content. |
| authd-oidc-brokers/internal/broker/testdata/golden/TestConcurrentIsAuthenticated/First_auth_starts_and_finishes_before_second/data/provider_url/user2/password | Add/update golden cached password file content. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@copilot resolve the merge conflicts in this pull request |
Resolved and pushed in |
denisonbarbosa
force-pushed
the
use-sub-for-identification
branch
2 times, most recently
from
fa8c983 to
9a10353
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1546 +/- ##
==========================================
- Coverage 88.09% 87.14% -0.96%
==========================================
Files 98 119 +21
Lines 6709 8033 +1324
Branches 111 111
==========================================
+ Hits 5910 7000 +1090
- Misses 743 977 +234
Partials 56 56 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
denisonbarbosa
force-pushed
the
use-sub-for-identification
branch
from
d352cfa to
4ef784c
Compare
denisonbarbosa
force-pushed
the
use-sub-for-identification
branch
3 times, most recently
from
3851b8f to
ca9c64c
Compare
nooreldeenmansour
left a comment
nooreldeenmansour
left a comment
There was a problem hiding this comment.
Thanks for addressing the earlier findings :)
Two more critical findings which I validated locally with tests, I formatted the local diffs (tests/proposed fixes) in the review comments below, so you can take a further look.
| @@ -26,17 +26,23 @@ type UserRow struct { | |||
| BrokerID string `yaml:"broker_id,omitempty"` | |||
There was a problem hiding this comment.
I'm not sure if we ever marshal this struct into JSON/YAML, but the omitempty implies that the broker ID is an optional field, which it is not anymore.
| BrokerID string `yaml:"broker_id,omitempty"` | |
| BrokerID string `yaml:"broker_id"` |
There was a problem hiding this comment.
I think this is mainly for tests. Every marshalling that we do for actual communication is JSON, not YAML. In order to avoid adding too much diff to the already quite big PR, I think this change can be avoided. Wdyt?
There was a problem hiding this comment.
would it require changes in the testdata? or only this single line? if it's only the single line, I would prefer doing the change to make it clear that this is not optional now.
Regardless, I agree that the PR is quite big. We could move the commits which make the broker ID required to a separate PR and base this one on it - if that doesn't cause too many merge conflicts. I think conceptually it would make sense.
There was a problem hiding this comment.
It will cause changes in golden files. I still think that the change is not necessary because this only controls what is serialized in the test files, but we can investigate more about it or add a comment to the struct field mentioning that this is only for tests or that it is a required value (although I think the rest of the code already safeguards it).
The identifier returned by providers is the OIDC "sub" claim (or an equivalent stable per-tenant identifier), not a UUID. Rename the info.User field and its JSON tag to ProviderID/"provider_id" so the meaning is explicit and matches the wire format exchanged with authd. This is a pure rename with no behavior change.
Entra ID's "sub" is pairwise per application and changes if the app registration changes, whereas "oid" is stable across all apps within a tenant. Use the "oid" claim as the provider ID and fail authentication when it is missing.
denisonbarbosa
force-pushed
the
use-sub-for-identification
branch
3 times, most recently
from
9c2b9f8 to
12c436e
Compare
nooreldeenmansour
left a comment
nooreldeenmansour
left a comment
There was a problem hiding this comment.
Two small suggestions below, also, don't forget to fix the commit messages to ensure they aren't stale
…ider ID Bump the broker API to v3, adding a "provider_id" argument to NewSession and DeleteUser so authd can pass the stable provider identifier. When a provider ID is supplied, the broker stores and looks up the on-disk cache under a provider-ID keyed directory instead of the username. Existing username-based caches are migrated on first use and a compatibility symlink is left behind, so a user's cache survives an email/username change at the identity provider. v2 callers keep the previous username-based behavior. A dangling compatibility symlink, left when its provider-ID keyed target is removed (for example by a partial DeleteUser), would otherwise make every later cache write resolve through the broken link and fail, locking the user out with no way to recover. Detect that on session start and drop the stale link so the username path can be rebuilt as a fresh cache directory.
Add a "provider_id" column to the users table to hold the stable provider identifier, with uniqueness scoped per broker via a partial index, plus the schema migration and the UserRow/query plumbing. Add UserByProviderID to look up a user by its broker-scoped provider ID. No caller populates the column yet.
Propagate the provider ID through UserInfo and the user manager. On UpdateUser, resolve the user by its broker-scoped provider ID so that a username/email change at the identity provider is handled as a rename of the existing user (preserving its UID and cleaning up stale local group entries) instead of failing with a UID conflict. The provider-ID match authorises the rename and bypasses the "UID already in use" guard, so an IdP-side email change landing on a username already owned by a different user would otherwise hit the raw UNIQUE(name) constraint and surface an opaque SQLite error. Reject that collision explicitly with a clear message, leaving both users intact. Expose ProviderIDForUser for callers that need a user's stable identifier.
Derive the broker ID from its D-Bus name so it stays stable regardless of the broker's display name, and detect and call the v3 broker interface when available, passing the user's stable provider ID to NewSession and DeleteUser. The PAM and user services look the provider ID up from the database so brokers can resolve the provider-ID keyed cache directly. The example broker and test doubles are updated to the v3 signatures.
…to GetBroker Now that the brokerID is a mandatory and unchangeable data for each user, updating the API for cleaner names makes sense.
The previous code was accidentally ignoring the error that could be returned by committing the transaction. Adding a named return to the function and joining with the previous error value should allow us to also consider any error that happen when committing the transaction.
denisonbarbosa
force-pushed
the
use-sub-for-identification
branch
from
12c436e to
e2125c3
Compare
This comment was marked as resolved.
This comment was marked as resolved.
nooreldeenmansour
left a comment
nooreldeenmansour
left a comment
There was a problem hiding this comment.
Thanks for addressing the suggestions and the great work on this PR!
I've added a tiny suggestion to consider
Also, ensure that commit messages are accurate, for example, 2651c92 has a stale message, we dropped the change regarding brokerId derivation from D-Bus names
| defer func() { | ||
| err = commitOrRollBackTransaction(err, tx) | ||
| // We don't want to lose the original error if commit/rollback also fails, so we join the errors together. | ||
| err = errors.Join(err, commitOrRollBackTransaction(err, tx)) |
There was a problem hiding this comment.
| err = errors.Join(err, commitOrRollBackTransaction(err, tx)) | |
| err = commitOrRollBackTransaction(err, tx) |
Is the Join here necessary?
There was a problem hiding this comment.
The join is to keep the original error message as well, otherwise it might be replaced by the result of the commitOrRollbackTransaction function.
There was a problem hiding this comment.
https://github.com/canonical/authd/blob/use-sub-for-identification/internal/users/db/db.go#L220-L227
It seems that it already preserves the error message, and doesn't replace it
There was a problem hiding this comment.
Hmm, that's true. Weird pattern though... I wonder if it's better to update the function rather than remove the join calls.
There was a problem hiding this comment.
I think it would be simplest to just amend the last commit and remove the join calls it introduced, to avoid altering the commitOrRollbackTransaction function
adombeck
left a comment
adombeck
left a comment
There was a problem hiding this comment.
Sorry, I'm still not done with the full review, but as discussed, I'm still submitting it.
There are quite some complex cases to think through here...
| // apps within a tenant. | ||
| stableID := userClaims.Oid | ||
| if stableID == "" { | ||
| return info.User{}, fmt.Errorf("oid claim is missing from the ID token") |
There was a problem hiding this comment.
This should be covered by a test in TestGetUserInfo
| // compatible change is made to the API. | ||
| // Note: Remember to also bump the LatestAPIVersion in internal/brokers/dbusbroker.go. | ||
| LatestAPIVersion uint = 2 | ||
| LatestAPIVersion uint = 3 |
There was a problem hiding this comment.
from the commit message of "add D-Bus API v3 and key the cache by provider ID":
Existing username-based caches are migrated on first use and a
compatibility symlink is left behind, so a user's cache survives an
email/username change at the identity provider
I don't understand the part about the cache surviving a email/username change at the identity provider. What happens if the email/username is changed at the identity provider? How does the symlink come into play in that scenario?
There was a problem hiding this comment.
Great question, which I don't have an answer for 😂 I'm pretty sure I mentioned that using the sub would have this username-change resilience (didn't say anything about the cache though) and that might have confused the agent. I'll update the commit message once I start rebasing the fixes.
| func (b *Broker) ensureProviderIDCacheDir(s *session, providerID string) { | ||
| providerIDDir, err := b.userDataDir(providerID) | ||
| if err != nil || providerIDDir == s.userDataDir { | ||
| return |
There was a problem hiding this comment.
We don't even log the error message?
There was a problem hiding this comment.
Yeah, I don't fully understand why Claude did that. Probably thought that there was no way for that call to error out, but then why check the error in the first place? I'll update this to consider the error.
| } | ||
| // NOTE: s.providerID is set only after successfully switching cache directories. | ||
|
|
||
| if _, statErr := os.Stat(providerIDDir); statErr == nil { |
There was a problem hiding this comment.
| if _, statErr := os.Stat(providerIDDir); statErr == nil { | |
| exists, err := fileutils.FileExists(providerIDDir) | |
| if err != nil { | |
| // Handle unexpected error | |
| } | |
| if exists { |
| if authInfo.UserInfo.ProviderID != "" && session.providerID == "" { | ||
| b.ensureProviderIDCacheDir(session, authInfo.UserInfo.ProviderID) | ||
| } |
There was a problem hiding this comment.
authInfo.UserInfo.ProviderID should never be empty here, right?
There was a problem hiding this comment.
I think it can be, no? If the user authenticates without refreshing the token, we load the cached AuthInfo, then we rely directly on the unmarshalled authInfo.UserInfo which, before the update, did not have the ProviderID field and thus will be empty. Am I missing something?
There was a problem hiding this comment.
True. Let's mention that in the comment then.
|
|
||
| // On first successful online authentication, migrate the cache directory to | ||
| // provider ID-based storage so that the cache survives future email/username changes. | ||
| if authInfo.UserInfo.ProviderID != "" && session.providerID == "" { |
There was a problem hiding this comment.
This is again referencing email/username changes which are not the reason why we're renaming the data directory from username to provider ID. The reason we're doing that is that we're using the provider ID for identifying user now.
Also, this code is not only run on first successful auth, so I would also omit that.
| // On first successful online authentication, migrate the cache directory to | |
| // provider ID-based storage so that the cache survives future email/username changes. | |
| if authInfo.UserInfo.ProviderID != "" && session.providerID == "" { | |
| if session.providerID == "" { | |
| // The cache directory doesn't use the provider ID as its name yet, but the username. Migrate it. |
There was a problem hiding this comment.
Also, this code is not only run on first successful auth, so I would also omit that.
That was not what the comment meant, but I don't mind updating it.
| // Provider ID-based directory already exists. | ||
| log.Infof(context.Background(), "Redirecting cache for user %q to existing provider ID-based directory %q", s.username, providerIDDir) | ||
| if linkErr := os.Symlink(providerIDDir, s.userDataDir); linkErr != nil && !os.IsExist(linkErr) { | ||
| log.Warningf(context.Background(), "Could not create cache compatibility symlink %q: %v", s.userDataDir, linkErr) |
There was a problem hiding this comment.
I think if we reach this case, we have a real problem. If we didn't get a provider ID from authd in the NewSession request, it means either:
- This is the first login after updating authd and the broker, or
- Only the broker was updated and authd doesn't store the provider ID yet.
If it's the second case, it means that authd will again only send a username in the NewSession request the next time the user tries to authenticate. Then, without the symlink, we don't have any way to get the user's data directory and the authentication will fail - even when they update authd (because authd still doesn't have the provider ID stored).
We could avoid that by reverting the renaming of the data directory here.
There was a problem hiding this comment.
This was a complicated one to think about.
we don't have any way to get the user's data directory and the authentication will fai
Is this correct? I think the broker will still be able to resolve the data directory (even without the symlink), but, since it won't find anything cached there, it will treat the user as a "new" one and reask for device authentication. Am I missing something?
We could avoid that by reverting the renaming of the data directory here.
If we fail to create a symlink, it's very likely (if not certain) that trying to rename the directory will also fail, no? So it's probably safer to leave the structure untouched and request that an admin take a look at it.
There was a problem hiding this comment.
Is this correct? I think the broker will still be able to resolve the data directory (even without the symlink), but, since it won't find anything cached there, it will treat the user as a "new" one and reask for device authentication. Am I missing something?
I meant there is no way to get the actual user data directory, i.e. the one we renamed to the provider ID. But you're right, authentication will not fail in that case, the user will be treated as a new user. That's also not great:
- The user can't use the local password for authentication during this authentication attempt
- They are asked to set a local password again (or the Entra password will be used if they use that flow). If that leads to a different local password being set, the keyring is still encrypted with the old one and can't be unlocked automatically anymore.
- If device registration is enabled, it will lead to a duplicate device registration.
So I still think it would make sense to avoid that if possible, by trying to undo the rename here.
Note that "only the broker was updated and authd was not" it not an uncommon edge case. The broker snaps are update automatically by default, the Debian packages are not.
| // Provider ID-based directory already exists. | ||
| log.Infof(context.Background(), "Redirecting cache for user %q to existing provider ID-based directory %q", s.username, providerIDDir) | ||
| if linkErr := os.Symlink(providerIDDir, s.userDataDir); linkErr != nil && !os.IsExist(linkErr) { | ||
| log.Warningf(context.Background(), "Could not create cache compatibility symlink %q: %v", s.userDataDir, linkErr) |
There was a problem hiding this comment.
Also, a test covering this case would be good.
| // NOTE: s.providerID is set only after successfully switching cache directories. | ||
|
|
||
| if _, statErr := os.Stat(providerIDDir); statErr == nil { | ||
| // Provider ID-based directory already exists. |
There was a problem hiding this comment.
This can only happen if authd wasn't updated yet and the email was changed in the IdP, right? I don't think we handled that case on the authd side before. Before this PR, if the email was changed, it was treated as a different user both by the broker and by authd, which means a new user records with a different UID was created by authd.
So to reach this case, these steps must have been taken:
- Broker was updated, authd was not
- User logs in, data directory is renamed to provider ID, compatibility symlink is created
- User changes email in IdP
- User logs in again
- Broker creates a new data directory named after the new email address
Now we try to create a symlink pointing from the old data directory (named after the provider ID) to the new one (named after the new email) - which will fail, because the directory already exists.
What happens next? As long as authd isn't updated, the data directory named after the new email will be used during authentication - meaning that the local password stored in there is used. Once authd is updated, the old data directory named after the provider ID will be used (except for the first time they log in after updating, because then authd still doesn't know the provider ID).
I got to go now, so I can't finish the analysis on how we should handle this.
There was a problem hiding this comment.
I feel like these edge cases were not well thought through during implementation. @denisonbarbosa please take a look at this comment and #1546 (comment) and think about how these cases should be handled. Thanks!
There was a problem hiding this comment.
I think the reasoning is mistaken here. If the broker was updated, it means it knows about providerIDs already, and once the user authenticates, their directory will be migrated to the new providerID format. If the user then changes their email to a new one, the broker can still resolve the providerID after the user authenticates, so we can silently switch to the providerID cache format (not implemented, AFAIK, but we can add this).
Also, I think we should remove the symlink as soon as authd sends the providerID for a user in NewSession, wdyt? This would mean that authd was already updated and that the user already has a providerID assigned to him, so no reason to keep the symlink available.
Another approach would be to drop the migration altogether and ask the users to redo device authentication after the update. The issue is that the broker would always request device authentication until authd is updated, because we would never get the providerID in NewSession.
There was a problem hiding this comment.
I think the reasoning is mistaken here. If the broker was updated, it means it knows about providerIDs already, and once the user authenticates, their directory will be migrated to the new providerID format. If the user then changes their email to a new one, the broker can still resolve the providerID after the user authenticates
How? In this scenario:
- authd wasn't updated yet, so it only sends the username, not the provider ID
- the user changes their email and uses that new email as the username
I still think that in this case, the broker will create a new data directory named after the new email address.
There was a problem hiding this comment.
afterwards, the broker tries to rename the newly created user directory to the provider ID, but that directory already exists, which brings us to this line I commented on.
There was a problem hiding this comment.
then it creates a symlink from new-username@example.com to sub-example-user - but sub-example-user is the old user data directory.
We used to rely on emails or
preferred_usernameto identify remote users. These are not guaranteed to be stable and can create difficulties when trying to identify users who have changed their email/username. In order to better identify the users and properly match them to their respective local accounts, we need to switch to using thesub(oidfor EntraID) claim, which is a unique ID assigned by the provider to its users, to identify the remote users. These will be generically named providerID in the code, since we use different values for the brokers.This also helps us to implement new features, such as allowing login with shortened usernames.
UDENG-9399