KEYCLOAK-11195 Restore default exported module in JS adapter

[jonkoops]

This PR fixes the bug where the default exported module of the Keycloak JavaScript adapter becomes one of it's included dependencies instead of the expected Keycloak API. This is simular to @karelhala's approach in #6279 but with a few distinct differences:

1. This version does not require additional dependencies to be installed.
2. The module registration code has been removed for dependencies, opting to just return the API of the module into a variable which is then passed into Keycloak.
3. This version has the exact same module registration code for Keycloak itself as before.

Although we are now using modified dependencies I think this is the best way forward now without introducing too many changes. In the future we should look at the possibility of using a module bundler that can support both the NPM and static file scenario's with proper dependency management.

[stianst]

I wonder if we should switch to copying the non-minifjed versions

[jonkoops]

@stianst It's certainly possible, it would however clutter the existing file with a rather large amount of unrelated code. I would recommend to move to a build system in the future that can take care of the dependencies for us.

I am willing to put the work in to make that happen, but I would consider that out of scope for now.

[jonkoops]

I wont be able to make any changes to this PR as I will be on vacation for the coming week. Please feel free to make any changes where deemed necessary.

In order to reproduce the minified files for the sha256 code I've added the source code on my Github: https://github.com/jonkoops/js-sha256/blob/no-modules/src/sha256.js. As for the base64 code just compile the original library and remove the module code directly.

[karelhala]

I really don't like this approach of pulling dependencies to your codebase, I know that you have no other choice for direct JS (accessible on window), but for standard packages it would be better to use require[1] which is standard way how to import JS packages.

Why I don't like this approach? Well if you'd were just to copy + paste the minified code I would be somewhat OK with it. We can perform md5 compare and update packages if there are some vulnerabilities (npm now offers auditing[2] - they work only on packages in dependencies list). If you are pulling 3rd party library and changing minified code not only you are bringing untested code, you can bring security propblems that not visible, because it's practicly a blob of code that is not easy (olmost impossible) to read. Also updating these libraries is almost impossible because you have to pull the minified code, change it and use it.

Another option is to bring unminified code, this can be slightly better than using minified code of 3rd party library. If we don't want to use require and use libraries directly pulled in I would vote for this approach. However add tests to fully cover what hese libraries are used for.

[1] https://www.w3schools.com/nodejs/nodejs_modules.asp
[2] https://docs.npmjs.com/auditing-package-dependencies-for-security-vulnerabilities

[jonkoops]

@karelhala Yeah, I totally agree that that we should not be embedding dependencies like this - at least for the NPM distributed version.

Unfortunately this has now already happened so we need a best way forward. The next step of this process should be to add a build system in order to fix these problems. This is a complicated issue as we also need to change the existing pipeline and testing to have things sustainable long term.

[jonkoops]

@stianst Would you like me to include these dependencies unminified? If so let me know then I will change this PR accordingly.

I would like to stress again that there is currently a version of keycloak-js published on NPM that is entirely non-functional. I would highly recommend this to be unpublished because it is causing a lot of confusion amongst users, including but not limited to:

• https://issues.jboss.org/browse/KEYCLOAK-11228
• https://issues.jboss.org/browse/KEYCLOAK-11187
• https://issues.jboss.org/browse/KEYCLOAK-11195
• https://issues.jboss.org/browse/KEYCLOAK-11217
• https://issues.jboss.org/browse/KEYCLOAK-11223
• ERROR TypeError: Keycloak is not a function mauriciovigolo/keycloak-angular#153
• Failed to integrate keycloak to shards-dashboard DesignRevision/shards-dashboard-react#15

[thomasdarimont]

Initial author here, mea culpa - sorry to have caused so many issues... :( apparently the public review wasn't enough this time, but it's great to see how you all step in to fix this problem :)

As you can see from the original discussion in: #6047 I tried to get this working for quite a while without an additional library, but all my attempts either failed or required to write a decent amount of code to provide browser compatibility. That's the reason why I choose to embed the small support libraries in the code in a, let's say sub-optimal way.

But perhaps one of you can see another way of dealing with this in a way that works cross-browser / platform.

Also, to avoid this from happening in the future: How can we write an automated test that checks if the keycloak-js module is properly packaged and can be consumed in browser and node environments?

[thomasdarimont]

How about stepping back again and serve keycloak.js without any dependencies but dynamic module loading/feature detection and provide a new resource called keycloak-deps.js, that contains all the support libraries (managed via npm/yarn, bundled and minified during build process) which provides the dependencies for browser environments. The keycloak.js could do some feature detection and load the keycloak-deps.js if necessary.
The user can then choose to just include /auth/js/keycloak.js and provide the required dependencies themselves or automatically include /auth/js/keycloak-deps.js via /auth/js/keycloak.js.

In node environments, one could package a wrapper around keycloak-js that performs the dependency resolution via require(..).

[karelhala]

@thomasdarimont you've basically described what I did in #6279 - use require or define if in UMD or AMD world and if none use global dependency.

About the testing, the best way here would be to write unit tests for JS. I am not good with MVN anymore and setting tasks in it (I'll probably do more harm than good) so I'll kindly leave this out to someone with more experience.

[thomasdarimont]

thanks @karelhala for your explaination, I now read through your PR #6279 and I think it's great work!

Also, I think this is similar to what I wrote with the exception that in your case the base64 and sha256 libs are still included in the file.

If you would externalize the dependencies and bundle them together you could dynamically retrieve them via a new resource /auth/js/keycloak-deps.js or through something like /auth/js/keycloak.js?include_deps=1.
This resource would only serve the bundled dependencies, not the original keycloak.js and then proceed with the initialization of Keycloak in browser context.
The benefit of this would be, that the library code is not embedded anymore in keycloak.js.

[karelhala]

and bundle them together you could dynamically retrieve them via a new resource /auth/js/keycloak-deps.js

That's a great idea! If KC team is fine with adding new file to browser and fron now on you'd have to load KC in window mode

<script src="/auth/js/keycloak-deps.js"></script>
<script src="/auth/js/keycloak.js"></script>

I am absolutetely fine with it.

[thomasdarimont]

Thanks karelhala

<script src="/auth/js/keycloak-deps.js"></script>
<script src="/auth/js/keycloak.js"></script>

This way would require users to change their code, which is not nice but acceptable.

A more convenient way would be to add dynamic script loading code to keycloak.js that could dynamically include a script element for something like /auth/js/keycloak-deps.js if it is necessary.
With this approach - user's wouldn't have to change their code.

Btw. /auth/js/keycloak.js maps to a resource in Keycloak not directly to a file, which means that this can have parameters like /auth/js/keycloak.js?include=dependencies or /auth/js/keycloak.js?mode=dependencies-only.
See: https://github.com/keycloak/keycloak/blob/master/services/src/main/java/org/keycloak/services/resources/JsResource.java

[jonkoops]

I have some concerns with the approach of side-loading dependencies as it introduces additional complexity to the code where I think it is not is needed. I do not think users that load keycloak-js statically care about the dependencies being included in the bundle. They simply want to load the file and have Keycloak available.

For users of the NPM package it's a different story. They have an environment where dependency management is expected and keycloak-js should do the right thing here and not include the dependencies in itself but specify them as proper dependencies in the package.json.

This means that in order to make Keycloak work for all users two separate distributions should be created. One for users that use the static files, and one for users of NPM. This means that we either manage two separate files, or introduce a build system to take care of this for us.

I strongly believe that introducing a build system is the most preferable option of these. However doing so would mean that we might have to introduce serious changes to the project structure, build system, testing, etc.

It is also an entirely different problem that we're trying to resolve in this PR. I would like to stress again that currently keycloak-js distributed through NPM does not work at all, for anyone. This change is simply a fix for the problems people are experiencing in the wild right now.

[mhajas]

@jonkoops @karelhala @thomasdarimont Thank you very much for looking into this. Great work and discussion!

Regarding this PR, I am not very happy about changing third party libraries, I like the approach in #6279 much more because of this.

I like also the idea of having dependencies in a separate .js file, however, only in case we are able to load keycloak-deps file dynamically based on the current environment and avoid this:

<script src="/auth/js/keycloak-deps.js"></script>
<script src="/auth/js/keycloak.js"></script>

My suggestion here is to merge #6279 as soon as possible (maybe with some changes so that all of us are happy about it) so that we have a functional keycloak-js module. I will do some testing to make sure we are not breaking anything in current behaviour in non-module "world".

As second step we will start a thread on keycloak-dev mailing list and discuss how to proceed with dependencies further.

WDYT?

[thomasdarimont]

@jonkoops I agree with you that this PR should be about the short-term fix.
Let's leave the other problem for another day and focus on this instead.

I never unpublished a library on npm before, but I think it's necessary in this case, to mitigate the confusion around developers that you mentioned.
I wonder what happens after unpublishing?

• Users should be able to use the keycloak.js version from 6.0.1 with Keycloak 7.0 (just without PKCE Support).
• Users who are currently using keycloak.js 7.0.0 probably realized that it is broken and went back to 6.0.1 or earlier anyways.
• Users who want to use new keycloak.js need to wait for the next Keycloak release (7.0.1 or 8.0.0?) to ship, since AFAIK there is no dedicated keycloak-js adpater project that would allow a release cycle independent of the Keycloak project.
• Users that are really eager or required to use PKCE with the Keycloak JS adapter could just use the fixed version from master (that's marked with a special tag). Another option would be to ask the Keycloak team to do an maintenance release for KC 7.0.x.

[jonkoops]

@mhajas Sounds good, editing dependencies is definitely not a favourable approach. I've we are going to go with the approach in #6279 I would like some to do a bit of testing locally to ensure everything works as expected with a Webpack build.

And yes, I completely agree we need to start a discussion on the mailing list about how we can proceed further.

@thomasdarimont I believe we can re-publish a version 7.0.0 but considering there are new changes that happened since the release we should publish version 7.0.1. However since the release cycle is locked to Keycloak itself this might be problematic.

[jonkoops]

Did some quick checks setting up a Webpack build and also using the static file method. All of the duplicated module code also seems to be getting eliminated properly so I am gonna go ahead and close this PR.