goaaats / KeySharp

Cross-platform C#/.NET keyring access
MIT License
15 stars 2 forks source link

Error code bug in C++ when deleting an item #2

Open Kanellaman opened 8 months ago

Kanellaman commented 8 months ago

After deleting successfully an item from keychain an error code is raised resulting in exception in C# code.

I run on this problem while testing the C# code on Windows.

Program.cs

using System;
namespace KeySharp
{
    internal class Program
    {
        static void Main(string[] args)
        {
            Keyring.SetPassword("Lalala", "Test", "thisuser", "Neone");
            try
            {
                var password = Keyring.GetPassword("Lalala", "Test", "thisuser");
                Console.WriteLine(password);
            }
            catch (KeyringException ex)
            {
                Console.WriteLine(ex.Message);
            }
            Keyring.DeletePassword("Lalala", "Test", "thisuser");
        }
    }
}

Output:

Neone

Unhandled Exception: KeySharp.KeyringException:  (NoError)
   at KeySharp.Keyring.ThrowLastError() in C:\Users\user\Source\Repos\KeySharp\KeySharp\Keyring.cs:line 114
   at KeySharp.Keyring.DeletePassword(String package, String service, String username) in C:\Users\user\Source\Repos\KeySharp\KeySharp\Keyring.cs:line 84
   at KeySharp.Program.Main(String[] args) in C:\Users\user\Source\Repos\KeySharp\KeySharp\Program.cs:line 18

The fix I am suggesting is changing the 50th line of main.cpp to if (error->type != keychain::ErrorType::NoError) similar to other functions. I have not actually tested the change I am suggesting but I am pretty sure this is where the error occurs(I'll try to re-compile the C++ code to test it). It would be awesome if you could verify.

OmarResponsival commented 2 months ago

It would be awesome if this got fixed, I currently have to do a try catch and check the error type which it works but is throwing my code coverage on testing a little off. I rather not fork since I really wouldn't be adding any functionality to it, the library works great except for this small thing.

goaaats commented 2 months ago

I'd be very happy to accept a pull request with this fix and rebuild the natives/release a new version! I sadly don't have time to look into it myself at the moment though.

Kanellaman commented 2 months ago

I'd be very happy to accept a pull request with this fix and rebuild the natives/release a new version! I sadly don't have time to look into it myself at the moment though.

Next week I will look into it and have a pull request made. No worries.

OmarForever commented 2 months ago

I'd be very happy to accept a pull request with this fix and rebuild the natives/release a new version! I sadly don't have time to look into it myself at the moment though.

Next week I will look into it and have a pull request made. No worries.

I opened a pull request with your fix, I tested locally and works properly.

Kanellaman commented 2 months ago

Mind if you accept the pull request and close the issue as resolved? @goaaats

goaaats commented 2 months ago

I merged the pull request, thanks Omar! I probably won't have time to make releases for a little while, so I'll leave the ticket open till then and update it when I get around to it.