Added PKCE, end session functionality and tested

[ChinthakaSenanayaka]

PKCE functionality is added as of Issue request #28 and tested.
End session functionality is added as of Issue request #52 and tested.

Please merge this PR and this is tested code. We depend on this library for our implementations. Since this has some issues, I fixed them and open for code review and merge. Any code review comments will be incorporated for this PR.

[tikurahul]

Hi. Thanks for your PR. I am going to take a look at in more detail.

I was wondering if PKCE is supported in the case where the client is a web browser ? I see you are pulling in node crypto which is unavailable in browsers. One of the reasons why I delayed PKCE is that I also wanted to provide a WebCrypto implementation.

[ChinthakaSenanayaka]

Q1. Why we need PKCE for web browser clients?
Reason1: We need to remove id_token from the URL. Even with SSL, id_token in URL cannot be considered as a secured approach. That means, in some cases (some users), we need more security than implicit flow.

Reason2: We cannot deviate from the Oauth spec or OIDC spec. If so, then we would have to maintain the custom implementation. Thus, the best approach is to stay within the standard and give PKCE flow to the browser clients.

Furthermore, this will be used with our identity server middleware where our users will request mission critical security access with even public browsers. Some are not interested with implicit flow with their public browser client apps, nevertheless, explaining the circumstances we can recommend the PKCE flow for them. Some may can go with implicit flow based on their requirement.

I think, this explains the requirement in the industry to go for at least PKCE for browser clients for some use cases.

Q2. From Crypto usage to move to WebCrypto
Reason1: WebCryto is still in progress and we cannot rely on that still. Once they release that we will move to that.

Reason2: Used Crypto with Chrome and it worked. I will test on other major browsers as well.

Furthermore, we need a solution now, thus we have to move with existing alternative solution or implement from scratch or contribute to such a in progress library. Anyway, if Crypto is not working on major browsers, we will move with alternatives, such as crypto-browsify, currently I do not know whether this library is working. Thus I will test with this as well. Later we can move to a better solution like WebCrypto.

Suggestion: I think, we better remove the dependency with JQuery for Ajax requests and use basic pure xmlhttprequest, because using JQuery, React or Angular is users' preference. And it is not in this library's scope.

Addition: Later I will send a separate PR or add code to this same PR with userInfo route implementation. Because we need that too.

[tikurahul]

I am not debating the need for PKCE in Web browsers. 😄
I am asking if we have the right interfaces, which can be used instead of using node-crypto or WebCrypto directly. I want to do this while providing the appropriate interfaces so we get consistent behavior on all the platforms that AppAuth-JS supports while not making assumptions about the capabilities of the platform itself.

WebCrypto is not a WIP. Its supported in all modern browsers, so using it is not problematic. We would always fallback to closure crypto as a fallback. The real issue is WebCrypto implies the page needs to be served off a secure origin (or localhost). It would be very confusing when that does not work.

[ChinthakaSenanayaka]

Hi Rahul,

Could you please explain the total comment. I am bit confused here.

Q1. What do you mean right interfaces? Is it about not to generate verifier and challenge but to give an interface to input to the library?
Q2. Yes, I saw webcrypto - npm. What is your end solution or suggestion? Shall we move to webcrypto or just crypto though it is not working on non secure origins (as per your comment, but I have not tested with webcrypto)? Anyway, I will try with webcrypto as well.

[ChinthakaSenanayaka]

Hi Rahul,

I made some commits. Could you please have a look on them?

1. Removed node crypto but did not use WebCrypto window.crypto because if it is nodeJs then we have to adapt the code again. So, I used js-sha256 since it is compatible with typescript.
2. Change the app/index.ts as wrapper app than just a sample app. Still the app can be used as sample app with basic html and JS.
3. Tested.

[ChinthakaSenanayaka]

Hi,
Added userInfo route functionality after testing. Please review and merge. If there were any issues, let me know.

[tikurahul]

Hi @ChinthakaSenanayaka.

Thanks for your interest in AppAuth-JS. I am glad that the library was useful and that you added these features. I am not going to merge your PR as-is. This is because of a few reasons:

• This library does not want to support every optional OAuth2 feature. Things like ending a session, user info etc. need not be a core part of the library. They can be built on top. This is also where some features are unique to an IDP in question, and we don't want to make any assumptions of IDP specific behavior.

• I am going to make a few minor modifications to make implicit flow much more easier. I think that will make a lot of your changes unnecessary. I am also going to add a top level Flow construct to the library which will help make a lot of common tasks easy.

• I also don't want to add dependencies for PKCE. I want to use WebCrypto while providing hooks for library users who want to use something different. That way no one needs to incur the cost of an additional dependency.

Going forward, I suggest proposing your changes to our mailing list or the GitHub tracker, and ask for feedback. Its also important to align with the design of all AppAuth libraries (on iOS and the Web).

---

For your use case, I suggest forking AppAuth-JS and maintaining your changes.