Conversation
rayluo
left a comment
rayluo
left a comment
There was a problem hiding this comment.
Thank you @abhidnya13 for your tireless effort on setting up a real test environment for this! That gave us high confidence on this implementation. Documenting below are our off-line conversations on how to polish this PR. Almost there. :-)
|
Oh this is an extra requirement beyond the original task scope: we will probably want to investigate how to automatically compute the thumbprint from the public cert. That would save app dev the trouble of updating thumbprint when they rotate their certs. |
…nd certificate string
rayluo
left a comment
rayluo
left a comment
There was a problem hiding this comment.
Thanks for your effort! I provide some more comments below. Let me know if you have more questions. :-)
On a side note, later we might also want to make the thumbprint input optional, ideally on the service-side, or we might still need to do it on client-side. So be prepared for such upcoming change.
|
Thanks for the review Ray ! This is really good learning for me :) I have made the changes. Let me know if it looks good now. Feel free to suggest anything else that we can do to make it better before we merge it in. |
rayluo
left a comment
rayluo
left a comment
There was a problem hiding this comment.
The rest of this PR looks great! Now let's make a final tuning on the regex part.
PS: The thumbprint enhancement will be an extra topic. I'll send an email to service team about it.
rayluo
left a comment
rayluo
left a comment
There was a problem hiding this comment.
Beautiful work! Ship it!
We can merge this PR now. The auto-compute-thumbprint topic can be addressed later in a different PR.
2 participants
This type of authentication expects authority url to be mentioned with the actual tenant id at the moment. Using organizations will fail the request