xamarin / Xamarin.Auth

Xamarin.Auth
Apache License 2.0
541 stars 351 forks source link

Potential error in the SecureStorageAccountStore Migration Helper Class #441

Open yanxiaodi opened 4 years ago

yanxiaodi commented 4 years ago

Xamarin.Auth Issue

I checked the code of SecureStorageAccountStore here: https://github.com/xamarin/Xamarin.Auth/wiki/Migrating-from-AccountStore-to-Xamarin.Essentials-SecureStorage

public static async Task MigrateAllAccountsAsync(string serviceId, IEnumerable<Account> accountStoreAccounts)
    {
        var wasMigrated = await SecureStorage.GetAsync("XamarinAuthAccountStoreMigrated");
        if (wasMigrated == "1")
            return;
        await SecureStorage.SetAsync("XamarinAuthAccountStoreMigrated", "1");

I am wondering if it is a bug? It seems like that this helper only supports one serviceId. If there are multiple serviceIds, only the first account can be migrated because XamarinAuthAccountStoreMigrated has been set to 1 when migrating the others.

Version

Expected behaviour

The helper should support multiple accounts.

Actual behaviour

To fix it, we can include the serviceId in the key, such as:

await SecureStorage.SetAsync($"XamarinAuthAccountStore{serviceId}Migrated", "1");
yanxiaodi commented 4 years ago

I just updated the Wiki. Not sure why I have the permission to edit it directly? Please review my change. Thanks. https://github.com/xamarin/Xamarin.Auth/wiki/Migrating-from-AccountStore-to-Xamarin.Essentials-SecureStorage/_compare/3e910f3a065c1c4400a556391876090018fb72ca...44f7cefb4385ddedd8c64dec42ca91922548c122

ghost commented 4 years ago

I believe there's a typo in one of the instances - the $ is inside the string instead of prefixing it.

yanxiaodi commented 4 years ago

@LoadEffectiveAddress Thanks a lot. My bad. Have updated:

https://github.com/xamarin/Xamarin.Auth/wiki/Migrating-from-AccountStore-to-Xamarin.Essentials-SecureStorage/_compare/3e910f3a065c1c4400a556391876090018fb72ca...397ec87a67591f03370d5c5487baffe59acfb87d