WIP POP improvements#2070
Conversation
Exposing ICrypto provider to allow users to pass in their own
| /// </summary> | ||
| /// <param name="uri">Protected resource URL.</param> | ||
| /// <returns></returns> | ||
| public T WithProofOfPosessionUri(Uri uri) |
There was a problem hiding this comment.
I don't like this strategy of piling on options on the AcquireToken* method because it "polutes" . There will be at least 4 params on SHR -> 4 methods on each AcqurieToken* entry point.
Instead, you can add a simple config object, similar to
So we could have a single method:
WithPoP(PoPOptions options = null)
There was a problem hiding this comment.
The main question here is - what should the default be here? I would argue for a secure default, where POP takes into account the method (GET / POST) and the url ...
| { | ||
| if (CommonParameters.PopUri == null ) | ||
| { | ||
| throw new MsalClientException(MsalError.PopUriCannotBeNull, "Proof of possesion endpoint is null."); |
There was a problem hiding this comment.
Uri is not mandatory as far as I understand. Please check with Brian / George.
So the user has the ability to say "don't bind the uri to the pop token"
| throw new MsalClientException(MsalError.PopUriCannotBeNull, "Proof of possesion endpoint is null."); | ||
| } | ||
|
|
||
| HttpRequestMessage message = new HttpRequestMessage(CommonParameters.PopMethod != null ? CommonParameters.PopMethod : HttpMethod.Get, |
There was a problem hiding this comment.
No, this isn't how it's supposed to work. The user has the ability to say "don't bind the method to the pop token".
| /// </summary> | ||
| /// <param name="popCryptoProvider"> Proof of posession cryptography provider</param> | ||
| /// <returns></returns> | ||
| public T WithPopCryptoProvider(IPoPCryptoProvider popCryptoProvider) |
There was a problem hiding this comment.
I think you are trying to tackle 2 stories at the same time, which makes it difficult to implement and review:
- Expose the crypto details to the user
- Make the method and uri optional
I don't think you should tackle #2 right now, nobody asked for it and it will complicate things.
| /// </summary> | ||
| /// <param name="popCryptoProvider"> Proof of posession cryptography provider</param> | ||
| /// <returns></returns> | ||
| public T WithPopCryptoProvider(IPoPCryptoProvider popCryptoProvider) |
There was a problem hiding this comment.
I think IPoPCryptoProvider is too low level and a higher level abstraction should be exposed to the user.
To help figure out the best solution, I would recommend you try to create 2 implementations that will help our users:
- An implementation of this interface that uses an X509Certificate2
- An implementation of this interfaces that uses ECD encyrption instead of RSA
Also, I believe that this public API should be discussed with Brian and George as well.
bgavrilMS
left a comment
bgavrilMS
left a comment
There was a problem hiding this comment.
"bring your own encryption" public API needs more work.
2 participants
for #2013