webauthn implementation #7
Conversation
Feature: * Implemented PIWebAuthnSignRequest to be able to recognize when a webauthn token was triggered * A button will be shown in the UI if a SIgnRequest is available * Implemented ValidateCheckWebAuthn to send the SignResponse that is received from the browser to privacyIDEA Internally: * Fixed namespace names * Put try-catch block around usages of dynmaic to prevent crashes * Refactored the usage of HttpClient to use StringContent instead of FormUrlEncoded so that we can decide what to encode. (encoding the SignResponse breaks it for the server) * SendRequest now properly uses the headers if any are passed
nilsbehlen
added
the
Type: Enhancement
| } | ||
| } | ||
| else |
There was a problem hiding this comment.
I would go for
else if (mode == "interactive")
and then finally do
else {
raise Exception("Mode not supported").
}
This might be more clear, and then is allowed to fail with a u2f token.
There was a problem hiding this comment.
This is not the mode that will be sent from privacyIDEA in the future.
It is the internal mode of the plugin, therefore there can be no mode that is not supported.
There was a problem hiding this comment.
Ah, ok. YOu are parsing the mode somewhere else. So at this point the plugin already knows the mode.
Conceptually I decided in the JSON Response to all the default mode "interactive" and not "otp", since it can be anything - like entering a PIN to be changed.
I would appreciate, if you also would call it here in the code "interactive" and not "otp", to have a uniform wording.
(The Glossry is to be done :-)
| if (response.MultiChallenge.Count > 0) | ||
| { | ||
| // TODO | ||
| Log("multichallenge > 0"); |
There was a problem hiding this comment.
What do you mean with "multichallenge" here?
I would like to get rid of the term "multi_challenge" in the long run, so maybe you can already talk of response.challenges.Count
or is the object created automatically from the key names like
multi_challenge -> MultiChallenge
?
There was a problem hiding this comment.
In this case it is the name of the field in the PIResponse class, so it could already be renamed.
There was a problem hiding this comment.
It is up to you how you (or others) will understand the code better.
I made a new issue for that (#8). Since it is not really related to webauthn i would just merge this PR now |
This is a good idea! Thanks. |
|
As for me this could be merged. |
Feature (closes #1):
Internally: