Consider: Changing the API from callbacks to interface for storage interactions between library and app

[aseigler]

I have a second sample credential store (on prem active directory) nearly ready to test. Currently it is fairly messy to switch the code in the controller out from one credential store to another. Therefore, I would think we should:

• Define a list of required methods and parameters for a credential storage database
• Provide a simple, configuration based way to switch between storage databases, using the in memory database as the default.

[abergs]

Define a list of required methods and parameters for a credential storage database

I can extract the Func signatures and add some /// docs if that would suffice?

Provide a simple, configuration based way to switch between storage databases, using the in memory database as the default.

For demo/test-purposes, right?
Since the library isn't really concerned with storage of credentials and the interface is Func's (which allows you do to whatever you want without implementing some type of IFidoCredentialsStore interface)

[aseigler]

I think the minimum number of properties for an assertion validation, per spec, is 3: credential ID, public key, and counter. I was thinking maybe create an interface with just those 3, and let credential store implementers extend from that? Just trying to come up with a way we can cleanly demonstrate different storage options without having to change a bunch of code to switch.

[abergs]

Do you still think this should be reworked?

[aseigler]

If folks are going to want to plug their own database into this, and not really dig in much deeper than that, it seems like a good idea. Based on what I had to do in the "ActiveDirectory" branch to switch from using the in memory database to on prem AD database, I think it would be easier if doing it again knowing exactly what was required as minimum to meet spec.

[abergs]

Alright, I hear you. I'm just battling a bit to not use the the lambda because the simplicity. Maybe some kind of wrapper could do the trick? Need to sit down and write some code to get the feeling. Will probably have time this weekend 👍

[abergs]

I have an idea,
what if we change the library class to use a private constructor and instead exposes two Create-functions.

Create(IConfiguration)

This will return an interface similar to the existing API.

CreateUsingStore(IConfiguration, ICredentialStore)

This will take a store Interface as an argument return a new interface, without the callback methods.

What do you think?

[StillLearnin]

I'm sorting through this issue right now. We have an aspnet core web app using aspnetcore identity and researching the best way to integrate this auth flow and store the credentials using the identity user manager. Has any more work or thought been put into this?

[StillLearnin]

Maybe a place to start would be to move from concrete User and StoredCredential classes to IUser and ICredential interfaces. Is this correct, and would you be open to a PR for this?

[aseigler]

As far as I am concerned, that looks like the direction I wanted this to go in. PR's are absolutely welcomed!

[StillLearnin]

Ok, I probably can't do it today anymore but I'll work on one.

[CodeTherapist]

Not sure if my thoughts are valid. I would prefer that the credential store is not part of the Fido2 core library because this concern is something that is very specific to an application. The Fido2 specification doesn't cover where and how the data should be stored. Therefore I would add a new project like Fido2.Identity (just a first idea) as an abstraction and provide specific implementation for specific App-Models e.g. AspNetCore.

What do you think?

[StillLearnin]

Yes, those are my exact thoughts.
The steps (I think) are these:

1. Accept TUser and TCredential parameters in this library's main constructor so as not to depend on a specific User or StoredCredential type.
2. Extract all storage related code into another project with appropriate interfaces etc.
3. Provide packages for AspNetCore.Identity etc. that implement the storage interfaces.

[abergs]

@StillLearnin I agree maybe with point1, allowing the user to use a Generic.

However: there is no storage in this library. The library currently uses callbacks to allow the consumer (app) to retrieve or store information.

See the DemoController for an example of the callbaks:
https://github.com/abergs/fido2-net-lib/blob/master/Fido2Demo/Controller.cs#L197

The dicussion earlier in this issue is whether a different api (e.g. forcing the consumer to implement a interface defined by us and then calling that interface) would be more practical than callbacks.

Again: There is no storage done by the library. We rely on the consumer app to retrieve and store any user or key related information.

edit: I do not think using a generic in the constructor is helpful. The only "User" we define is the one used to transmit WebAuthn/Fido2 spec information about a User: https://github.com/abergs/fido2-net-lib/blob/94057aa13609b1baa15a45e3c2e40913fd0a4858/fido2-net-lib/CredentialCreateOptions.cs#L231 as described by the spec

I am still open to and considering if a different API than callbacks would be better, but the case that Alex describes is maybe not the way most consumers will encounter (changing different storage mechanisms for demo-purposes).

[abergs]

@aseigler one way to accomplish what you originally wanted is perhaps to hide the actual implementation of the callbacks in your own interface? :)

Psuedo code:

public class Controller {
     private IMyCredentialStorage credentials = new ADCredentialsStorage();
     //private IMyCredentialStorage credentials = new MemoryCredentialsStorage();
     //private IMyCredentialStorage credentials = new AspNetCredentialsStorage();
     //private IMyCredentialStorage credentials = new MySqlCredentialsStorage();
     //private IMyCredentialStorage credentials = new WhateverCredentialsStorage();


    [HttpPost]
        [Route("/makeCredential")]
        public async Task<JsonResult> MakeCredential([FromBody] AuthenticatorAttestationRawResponse attestationResponse)
        {
            try
            {
                // 1. get the options we sent the client
                var jsonOptions = HttpContext.Session.GetString("fido2.attestationOptions");
                var options = CredentialCreateOptions.FromJson(jsonOptions);

                // 2. Create callback so that lib can verify credential id is unique to this user
                IsCredentialIdUniqueToUserAsyncDelegate callback = async (IsCredentialIdUniqueToUserParams args) =>
                {
// USE YOUR METHOD implementation: 
                    var users = await credentials.GetUsersByCredentialIdAsync(args.CredentialId);
                    if (users.Count > 0) return false;

                    return true;
                };

// OR EVEN BETTER: 
IsCredentialIdUniqueToUserAsyncDelegate callback = credentials.CheckUniqueToUser;
                // 2. Verify and make the credentials
                var success = await _lib.MakeNewCredentialAsync(attestationResponse, options, callback);

                // 3. Store the credentials in db
               credentials.AddCredentialToUser(options.User, new YOURStoredCredential
                {
                    Descriptor = new PublicKeyCredentialDescriptor(success.Result.CredentialId),
                    PublicKey = success.Result.PublicKey,
                    UserHandle = success.Result.User.Id,
                    SignatureCounter = success.Result.Counter,
                    CredType = success.Result.CredType,
                    RegDate = DateTime.Now,
                    AaGuid = success.Result.Aaguid
                });

                // 4. return "ok" to the client
                return Json(success);
            }
            catch (Exception e)
            {
                return Json(new CredentialMakeResult { Status = "error", ErrorMessage = FormatException(e) });
            }
        }

}

[abergs]

@aseigler The IMyCredentialStorage is something the app developer would define and implement. It would sit between the app (webbapp/nativeapp/whatever) and storage mechanism (database/memory/whatever) to allow easily switching the implementations of storage without touching the interactions (callbacks) with Fido2-lib that might be spread around in different functions or files.

edit: This would solve your problem, changing from in-memory to AD without touching the callbacks.
Question still to be decided is if we should define this interface and force the consumer to implement it, or if the callbacks are easier. Maybe that should be a different issue so not to confuse people :)

[abergs]

Just for the record: I'm still leaning towards callback because of simplicity and putting less work on the developer that consumes this library.

If they want the flexibility of an interface, they are free to define and use one, but we won't force it upon them.

[CodeTherapist]

Again: There is not storage done by the library. We rely on the consumer app to retrieve and store any user or key related information.

@abergs I agree on that - I think that this is the right way to do it.
Regarding the callback vs interface. The callback is simple to use, flexible and should be enough to start with.

[StillLearnin]

Sorry, I wasn't very clear in what I was trying to say. You are obviously correct,

there is no storage in this library

What I was trying to suggest is that the DevelopmentMemoryStore and its associated StoredCredential class be moved to the Demo (or some other) project. This would keep the main library free from all hints of storage implementations. For me at least, it was confusing when looking into replacing the DemoStorage with PersistentStorage, but it's quite possible it won't be confusing to others.

On the generics issue:

@abergs

I do not think using a generic in the constructor is helpful. The only "User" we define is the one used to transmit WebAuthn/Fido2 spec information ... as described by the spec

From the spec

The PublicKeyCredentialUserEntity dictionary is used to supply additional user account attributes when creating a new credential.

Problem

Given the current implementation, how can I extend the user class to include optional fields or add custom "additional user account attributes"? With generics, this would be easy.

Suggestion

Provide a constructor that accepts a generic TUser with a default of type User. TUser must inherit User.

[abergs]

Thank you for a good follow up.
Maybe it's not optimal to have DevelopmentMemoryStore in the library dll. It does however make it simple to test things during development for consumers. It's currently placed in the namespace Fido2NetLib.Development so hopefully its usage is limited and its intentions clear.

Regarding TUser
I'm not really sure when you would need to have it extended. When calling in to the library, I believe the consumer of the library would normally be required to "map" their App.User to a Fido2NetLib.User which is probably a requirement anyway since few applications represent the Id field as a byte[].

E.g:
var fidoUser = myUser.ToFidoUser()
or
var fidoUser = new Fido2NetLib.User() { Id = ConvertToBytes(myUser.Id) ...};

What's the scenario where you need to access data from the user object where a custom type would be helpful? The only case I can come up with is in the public delegate Task<bool> IsCredentialIdUniqueToUserAsyncDelegate(IsCredentialIdUniqueToUserParams credentialIdUserParams); callback, where it might be helpful to have more information regarding the user. However, it would come with the cost of forcing the consumer to represent their App.User have their Id-prop as a byte[], no?

edit:

but it's quite possible it won't be confusing to others.

If one person is confused by it, I think we should expect others to be confused by it. Maybe documentation or comments would be enough to clarify?

[abergs]

Let's move the discussion regarding TUser to #88