Feature/refactoring #80
Conversation
| { | ||
| var authCacheType = Settings.Instance.GetAuthCacheType(); |
There was a problem hiding this comment.
Не очень нравится что тут зависимость на настройки. Всё-таки кажется что раз метод статический, то хочется чтобы он был чистым.
Но, с другой стороны, тут так же есть UAC и мы жили с этим без напрягов
| @@ -13,22 +13,23 @@ public interface IAuthProvider | |||
| byte[] Encrypt(byte[] data); | |||
| byte[] PromptToDecrypt(byte[] data); | |||
| void ClaimCurrentCacheType(AuthCacheType authCacheType); | |||
| AuthCacheType CurrentCacheType { get; } | |||
| AuthCacheType CurrentCacheType { get; set; } | |||
There was a problem hiding this comment.
Наличие сеттера вводит в заблужение о цели существования ClaimCurrentCacheType
There was a problem hiding this comment.
Наверное, просто Claim... не должен быть частью AuthProviderа
There was a problem hiding this comment.
Ещё надо подумать – можно ли как-то обойтись от зависимости на тип кэша у провайдера совсем. Например, этот же энам у нас используется и при создании storage'а, но там мы на разные реализации всё разнесли и свитч только в фабрике присутствует. Может и тут нам также поступить и декомпозировать ответственность за шифровку/дешивровку в отдельный класс, а за выбор ключа сделать ответственным кого-нибудь другого? Можно передавать провайдер ключа как зависимость AuthProvider'у, например. Что думаешь?
| { | ||
| CreatePersistentKey(false).Dispose(); | ||
| } | ||
| CreatePersistentKey(false).Dispose(); |
There was a problem hiding this comment.
Раз уж тронули, добавь плиз overwriteExisting:
| @@ -161,16 +194,17 @@ private void Unlock(KeyPromptForm keyPromptForm, string dbPath) | |||
| CloseFormWithResult(keyPromptForm, DialogResult.OK); | |||
| } | |||
| { | ||
| if (_keyCipher.AuthProvider.CurrentCacheType != Settings.Instance.GetAuthCacheType()) | ||
| { | ||
| // todo: something unexpected, messagebox |
There was a problem hiding this comment.
Наверное тут такая же ситуация, как и если у нас исключение "ключ был удалён" и надо то же сообщение показывать
| ErrorHandler.ShowError(ex); | ||
| } | ||
| } | ||
| } | ||
| else if (SystemInformation.TerminalServerSession) // RDP | ||
| { | ||
| MessageBox.Show(AuthProviderUIContext.Current, |
There was a problem hiding this comment.
А для этого не требуется AuthProviderUIContext? Там выше у нас зачем-то юзинг перед сообщениями
| { | ||
| ErrorHandler.ShowError(ex); | ||
| } | ||
| _keyManager.RevokeAll(); |
There was a problem hiding this comment.
Раз мы везде убрали проверки внутри, то надо сделать проверку на null в конструкторе
Main changes: