SecureStorage.ios.cs throws improper Exceptions.

[robv8r]

Bug report best practices: Submitting Issues

Description

The source code for SecureStorage.ios.cs throws System.Exception instead of a more appropriate exception.

The first is in SetValueForKey. The second is in RemoveRecord

This is not a proper use of Exceptions. A developer should neither throw nor catch a System.Exception per Microsoft's Best Practices.

Expected Behavior

A more appropriate exception that can be handled at runtime. For example, a System.ArgumentOutOfRangeException if the value could not be removed because the key didn't exist.

Actual Behavior

If a value cannot be set or removed, a generic System.Exception is thrown.

Basic Information

• Version with issue: 0.9.1
• Last known good version: N/A
• IDE: Visual Studio 2017 Enterprise
• Platform Target Frameworks:
  • iOS: All versions

VS bug #735656

[jamesmontemagno]

Will assign to @Redth to cleanup this exception.

I would need to see which of these are correct or if it should be a custom exception.

[Mrnikbobjeff]

No exception in the best standard docs fits our scenario. We should use ExternalException as it fits our use case exactly. External calls from our application which returns an error code.

[robv8r]

In this case, always throwing an ExternalException may not be descriptive enough for developers.

May I recommend using the same Exception types as IDictionary<TKey,TValue>? For example, if the result of an operation is SecStatusCode.ItemNotFound, a KeyNotFoundException may be appropriate.

---Update---

Using exceptions similar to IDictionary may also be appropriate for other SecureStorage platform implementations.

[Mrnikbobjeff]

All monotouch error codes and their suggested corresponding exception:
Allocate | OutOfMemoryException
AuthFailed | UnauthorziedAccessException?
BadReq | InvalidOperationException / Custom?
Decode | FormatException
DuplicateItem | InvalidOperationException / Custom?
DuplicateKeyChain | InvalidOperationException / Custom?
InteractionNotAllowed |UnauthorizedAccessException
InternalComponent | ExternalException
InvalidKeyChain | InvalidOperationException
IO | InvalidDataException
ItemNotFound | KeyNotFoundException
NoSuchKeyChain | InvalidOperationException
NotAvailable | UnauthorizedAccessException
OpWr | FileLoadException
Param | ArgumentException?
ReadOnly | Custom Exception || UnauthorizedAccessException
Success | None
Unimplemented | NotImplementedException
UserCanceled | TaskCancelledException
VerifyFailed | Could not find more info when this occurs, probably Custom

As you see if we were to honor the most specific exception usage, we would force the user to either catch Exception or 10 different exception types. I would be in favor of a class which also exposes the ErrorCode and just throw this uniform Exception. This is less of a burden on dev and maintainer, as nobody benefits from a InvalidOperationException. Without the error code the exception which we throw is irrelevant. I especially dislike the implication that we may throw over 10 different Exceptions on iOS, but on the other platforms it is vastly reduced and virtually exception free.

[robv8r]

@Mrnikbobjeff

Would it be more appropriate to use exceptions similar to those thrown by IDictionary operations and add the SecStatusCode to the Exception.Data collection?

[Mrnikbobjeff]

@robv8r
I do see the benefit of using known exception type schemes like in dictionary. The problem here is, if I were to throw a KeyNotFoundException with the data element set to something unrelated we would just confuse the developer. I do not think the exception surface for any Collection type is sufficient to expose all the platform exceptions which could occur here for a myriad of reasons. To keep things as unambiguous as possible I would still suggest using a custom exception type which possibly inherits from External exception to Expose the error code. Again, confusion is minimal and implementers don't have to worry about different exception types. If the developer desires to filter by a error code he/she can still do so by using Exception filtering with properties

[jamesmontemagno]

I think the best thing to do here is have the following:

public class SecureStorageException : Exception
{
    public SecureStorageErrorCode ErrorCode {get;}
}

public enum SecureStorageErrorCode
    {
        AllocateMemory,
        AuthenticationFailed,
        BadRequest,
        Decode,
        DuplicateItem,
        DuplicateKeyChain,
        FileAlreadyOpen,
        InteractionNotAllowed,
        InternalComponent,
        InvalidKeyChain,
        InvalidParameters,
        IO,
        ItemNotFound,
        NoSuchKeyChain,
        NotAvailable,
        Other,
        ReadOnly,
        Unimplemented,
        UserCanceled,
        VerificationFailed
    }

Then i will map the iOS StatusCodes to these.

[Mrnikbobjeff]

After looking over the link @mattleibow provided I believe a combined approach would be advisable - most libraries offer a generic Exception type as it is not recommended to either throw or catch System.Exception. Following suit we should probably create a universal Exception type used for cases such as this where platform exception behaviour surfaces and is needed to properly debug issues. Then, as suggested, we could use the InnerException property to contain specific Exception information such as the Exception type you provided. This way we can preserve everything and still conform to best practices. Also, this would allow having a universal catch clause for exceptions generated by Essentials, something which would be nice to have. I saw you closed your PR @jamesmontemagno so I thought this might be the place to discuss Excption behaviour.

[mattleibow]

Just a link to my comment for a future FYI: #500 (comment)