Feature/refactoring #80
Conversation
| public static IAuthProvider GetInstance(IntPtr keePassWindowHandle, AuthCacheType authCacheType) | ||
| public static IAuthProvider Create(IntPtr keePassWindowHandle) | ||
| { | ||
| var authCacheType = Settings.Instance.GetAuthCacheType(); |
There was a problem hiding this comment.
Не очень нравится что тут зависимость на настройки. Всё-таки кажется что раз метод статический, то хочется чтобы он был чистым.
Но, с другой стороны, тут так же есть UAC и мы жили с этим без напрягов
| 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:
| { | ||
| SetCompositeKey(keyPromptForm, compositeKey); | ||
| CloseFormWithResult(keyPromptForm, DialogResult.OK); | ||
| } |
| { | ||
| if (_keyCipher.AuthProvider.CurrentCacheType != Settings.Instance.GetAuthCacheType()) | ||
| { | ||
| // todo: something unexpected, messagebox |
There was a problem hiding this comment.
Наверное тут такая же ситуация, как и если у нас исключение "ключ был удалён" и надо то же сообщение показывать
| } | ||
| 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: