Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[netXX-desktop] PasswordVault is throwing "NotImplementedException" #17372

Open
TopperDEL opened this issue Jul 3, 2024 · 12 comments
Open

[netXX-desktop] PasswordVault is throwing "NotImplementedException" #17372

TopperDEL opened this issue Jul 3, 2024 · 12 comments
Labels
area/skia ✏️ Categorizes an issue or PR as relevant to Skia difficulty/medium 🤔 Categorizes an issue for which the difficulty level is reachable with a good understanding of WinUI kind/enhancement New feature or request

Comments

@TopperDEL
Copy link
Contributor

Current behavior

I want to use the PasswordVault in MacOS-Desktop - but during the Constructor of PasswordVault it raises a "NotImplementedException".

var vault = new Windows.Security.Credentials.PasswordVault();

Expected behavior

The PasswordVault works there, too.

How to reproduce it (as minimally and precisely as possible)

No response

Workaround

Don't use it or temporarily save credentials to a local file.

Works on UWP/WinUI

Yes

Environment

Uno.WinUI / Uno.WinUI.WebAssembly / Uno.WinUI.Skia

NuGet package version(s)

5.2

Affected platforms

Skia (macOS)

IDE

Visual Studio Code

IDE version

latest

Relevant plugins

No response

Anything else we need to know?

No response

@TopperDEL TopperDEL added difficulty/tbd Categorizes an issue for which the difficulty level needs to be defined. kind/bug Something isn't working triage/untriaged Indicates an issue requires triaging or verification labels Jul 3, 2024
@Youssef1313 Youssef1313 added kind/enhancement New feature or request and removed kind/bug Something isn't working labels Jul 3, 2024
@spouliot spouliot changed the title [Mac-Desktop] PasswordVault is throwing "NotImplementedException" [netXX-desktop] PasswordVault is throwing "NotImplementedException" Nov 4, 2024
@spouliot
Copy link
Contributor

spouliot commented Nov 4, 2024

This happens for all netXX-desktop (not only on macOS) since there's no code for this when __SKIA__ is defined (nor any extension mechanism to add it to specific Skia-based platforms). c.c. @MartinZikmund

@TopperDEL
Copy link
Contributor Author

I just happened to try the Mac Version again. I had nö Debugger attached yet (tried the build from my Pipeline) but it still crashes on startup (where the vault is used). So I assume this error is still my Show Stopper.

Is this hard to implement? Then I would wait. Otherwise I would go for a temporary work around for my app.

@TopperDEL
Copy link
Contributor Author

I would love to tackle this, but I nearly have any spare time currently. But if someone could guide me I would love to contribute again.

@spouliot
Copy link
Contributor

spouliot commented Nov 4, 2024

@TopperDEL do you only need it for the Mac ? or for all netXX-desktop platforms ?

We normally (not quite for this right now) have some generalized/shared code for Skia so only the platform specific bits needs to be written.

However the new macOS backend does not use [Xamarin.Mac|Microsoft.macOS].dll so the platform code needs to be rewritten in (native) ObjC.

@TopperDEL
Copy link
Contributor Author

I only need it for Mac currently. Is it feasable to do?

@spouliot
Copy link
Contributor

spouliot commented Nov 5, 2024

Is it feasable to do?

Of course!

Have a look at the Uno.UI.Runtime.Skia.MacOS code to see how the new Mac backend works. Basically it calls to an ObjC library that expose C API (IOW it hides everything that is ObjC specific).

You then need to translate the existing C# code (for iOS/macOS) into ObjC and expose them as C API.

@MartinZikmund MartinZikmund added area/skia ✏️ Categorizes an issue or PR as relevant to Skia difficulty/medium 🤔 Categorizes an issue for which the difficulty level is reachable with a good understanding of WinUI and removed triage/untriaged Indicates an issue requires triaging or verification difficulty/tbd Categorizes an issue for which the difficulty level needs to be defined. labels Nov 6, 2024
@TopperDEL
Copy link
Contributor Author

So basically I get the idea. But I'm lacking many information. I don't know ObjC and I don't know the specifics of the Keychain on MacOS (both from a user and a developer perspective).

There exists a project, though, that we might use here. It also has a native wrapper for the Keychain using this. Might it be a good way to build upon this? Or do you like to be independent and roll your own implementation?

@spouliot
Copy link
Contributor

spouliot commented Nov 7, 2024

Might it be a good way to build upon this?

Adding a dependency is always a tradeoff. It can be more work than actually rolling your own. Think of the build changes required to add that library (to Uno and to apps).

In our case we need an ObjC version of this code, which is rather small

https://github.com/unoplatform/uno/blob/master/src/Uno.UWP/Security/Credentials/PasswordVault.iOSmacOS.cs

Or do you like to be independent and roll your own implementation?

Another advantage of translating our actual code is that existing applications (using the old macOS backend) would remain compatible once migrated to the new backend.

@TopperDEL
Copy link
Contributor Author

TopperDEL commented Nov 11, 2024

Thanks for guiding me, @spouliot. I was at least able to compile the dylib on my mac but I still don't really know how to do it exactly. I tried with the help of github copilot, but it failed. The error I get from xcode is not helpful for me as I do not know ObjC.

What confuses me:

https://github.com/unoplatform/uno/blob/master/src/Uno.UWP/Security/Credentials/PasswordVault.iOSmacOS.cs

This code has to be brought to ObjC? Why this one? If I understand it correctly it persists credentials in a binary format. Using the same technique on the skia-backend would make it compatible with the "old" mac-backend, right?

But that means that I would need to bring the SecKeyChain.QueryAsRecord() and SecKeyChain.Update() methods to ObjC, not the whole KeyChainPersister.

@spouliot
Copy link
Contributor

This code has to be brought to ObjC?

Mostly... now I think most of the keyvault APIs are actually C API (not ObjC), but it does need to end up inside the UnoNativeMac native library.

Also general code can (and should) remain in C# so it can be shared with others Skia platforms (in the future).

Why this one?

It's the implementation of the old macOS host backend. It works and, by based your code on it, it can be backward compatible for existing apps.

But that means that I would need to bring the SecKeyChain.QueryAsRecord() and SecKeyChain.Update() methods to ObjC, not the whole KeyChainPersister.

Yes. It's best to keep, as much as possible, the code in C# since it can be shared with other Skia-based platforms. Only what's specific to macOS should end up in the native library.

Also you can look at Xamarin's source code for SecKeyChain since they are mostly p/invoke (to C functions) so moving this to the Xcode project should not be too hard.

But that means that I would need to bring the SecKeyChain.QueryAsRecord() and SecKeyChain.Update() methods to ObjC, not the whole KeyChainPersister.

It needs a complete Skia implementation, the current one simply throws.

So in the end, to be mergeable, there needs to be some additional Skia general code, i.e. code that will run on all Skia platforms.

Then each platform backend can extend it with platform specific code. For macOS that would use the keyvault API. Other platforms won't have (for now) an implementation so that should throw a NotImplementedException.

See the clipboard implementation for an example of how extensions are registered and instantiated.

@TopperDEL
Copy link
Contributor Author

I just found out that you were one of the authors of the Xamarin-implementation for the SecKeyChain. :)

So, I just tried to understand what needs to be done. I really would love to tackle this - but: I still do not really get where to start. When I loook at the implementation of the SecKeyChain here I was wondering why we do not simply use that class from the Xamarin-macios-repository as a whole? Basically I would need to add the same stuff - why not use the code as the licence permits the use?

Otherwise I would start with this class as it seems to be the main entry into the native c-stuff.

But again: I do not know enough about MacOs, the KeyVault, Xcode and all so it seems this thing is too big for me, unfortunately. I would need much more understanding about marshalling stuff, how the KeyVault on mac works etc pp.

@spouliot
Copy link
Contributor

I just found out that you were one of the authors of the Xamarin-implementation for the SecKeyChain. :)

heh, I did a lot of things in previous self incarnations ;-)

why we do not simply use that class from the Xamarin-macios-repository as a whole? Basically I would need to add the same stuff - why not use the code as the licence permits the use?

"As-is" the code has a lot of dependencies on netXX-macos SDK, either managed or inside its native runtime and even on its tooling. You won't be able to copy/paste the file into Uno - at least without copying a lot of other things as well.

However since it's C API you could copy the p/invokes (but not most of the helpers and ObjC based classes) and use C# exclusively to implement the feature. It's not gonna be less code but since you're familiar in C# (and not C) it might be easier.

I do not know enough about MacOs, the KeyVault, Xcode and all so it seems this thing is too big for me, unfortunately.

Do you feel ok with the other C# parts ? like the extension registration, a general Skia implementation. If so then we could collaborate in a PR where I would take care of the native bits.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/skia ✏️ Categorizes an issue or PR as relevant to Skia difficulty/medium 🤔 Categorizes an issue for which the difficulty level is reachable with a good understanding of WinUI kind/enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

4 participants