[keycloak-dev] Javascript Adapter Feedback

Thu Nov 22 07:46:03 EST 2018

On Thu, Nov 22, 2018 at 12:11 PM Stian Thorgersen <sthorger at redhat.com>
wrote:

> Thanks for your feedback, some comments below:
>
> On Wed, 21 Nov 2018, 13:26 Guillaume Vincent <gvincent at redhat.com wrote:
>
>> Hello,
>>
>> this mail is an email to give my feedback on Keycloak Javascript Adapter.
>> The feedback is not good. If you are not open to criticism, do not read
>> this email.
>>
>> TLDR: The Javascript code is a 1500+ line of code, untested, with 10
>> levels
>> of indentations, no abstraction, difficult to read, difficult to maintain,
>> include in an web application and not following semver.
>>
>> # loc
>> maintain and fix 1500 line of code is not easy at all. The javascript
>> adapter should be split in small javascript functions/modules/class and
>> respect some basic clean code. SRP, no duplication, etc.
>>
>
> Breaking up also introduces some complexity so I'm not fully convinced.
>
>
I'm speaking about clean code. I encourage you to read Clean Code from
Robert C. Martin.
Maybe because you are working on the adapter for so long you found the code
easy to read and maintain.
But trust me for newcomers like me it's so painful to read.

>
>>
>>
>>
>> # tests
>> No single test on the javascript adapter? really. We should introduce some
>> basic tests and API test to avoid breaking the interface. Basically
>> decrypting a JWT, parsing the url, initialisation could be tested easily.
>>
>
> Of course there are tests. Tests are part of our Arquillian Java testsuite
> though.
>

I'm speaking about unit test, functional test in javascript with the use of
some module like ava, jest or mocha.
Not tests I cannot run.
Tests that are run in nodejs and in browsers with some lib like pupetter or
karma.
Tests should be attach to the project and can be run with a single npm test
command.

>
>
>> # semver
>> In javascript world, the common convention regarding the version number is
>> semver[1]. So when the API change, the major version should be
>> incremented.
>> Not the minor one. We could avoid like that fixing the version number in
>> package.json [2] to avoid those kind of bug [3]
>>
>
> It's not quite that trivial as we release everything in Keycloak as a
> bundle. We can't bump the major version for a small api change and I know
> which one you are referring to. We strive to not break APIs and rather use
> deprecation. However, in the case you are referring to this was not
> possible as it was a broken design and there was no way to fix it. Api
> hasn't really changed either we just require you to set a config option to
> use native promises.
>
>
Api hasn't really changed ? really?
We probably don't have the same understanding of what is an API, and what
is breaking an API.
If I'm oblige to update my options given to a function or replace a
function to have the same behavior when I update your library then you
broke the API.

4.5.0 sso.init().then() works
4.6.0 sso.init().then() doesn't works. You forced dev to do
sso.init({promiseType: "native"}).then() or sso.init().success()

This is why I call a breaking change

If you tied the version of Keycloak the product to the version of the
Javascript adapter then you are not making the work of javascript dev easy.

>
>> # integration in existing app
>> nobody today create real professional frontend application using the plain
>> javascript file. The frontend world is compose of small piece assemble
>> together. We use bundler like webpack, browserify, rollup etc to create
>> our
>> application. Basically we don't do this:
>>
>> ```
>> <script src="keycloak.js"></script>
>> <script>
>>   var keycloak = Keycloak();
>>   keycloak.init()
>> </script>
>> ```
>>
>> we do this
>>
>> ```
>> const Keycloack = require("keycloack");
>> var keycloak = Keycloak();
>> keycloak.init();
>>
>> ```
>>
>> yes like in node
>>
>
> Yes, some do, but not everyone. The adapter has been designed with the aim
> to work for both cases.
>
>
I'm not saying that you should remove the production of the library for the
browser.
I'm saying that the documentation should target the common usage of this
library.
If you think it's the first one, I think you are wrong.

>
>>
>> # modules
>> the keycloak library should use modules or be decomposed into modules.
>> Decoupled in small piece and use rollup or webpack to bundle the
>> application. It will help for example bundler to do tree shacking for
>> example and reduce the bundle size of the library.
>>
>> So instead of try to reinvent the wheel and introduce some insecure
>> code[4]. We should use javascript module, well tested like this one[5] and
>> do something like
>>
>> const uuidv4 = require('uuid/v4');
>> function createUUID() {
>>   return uuidv4();
>>
>
> Perhaps, but using libraries has its own problems. End of the day it's
> easier for us to support an in-line uiid function than support external
> libraries.
>
>
yes and use for example a function that use Math.random() with is not
cryptographically strong.
The choice made by keycloak team is to rewrite everything? zero modules?
My opinion is that by doing this, you will introduce some vulnerability in
the code.

For example uuid is downloaded more than 13 millions times per week.
So a lot of dev and some good ones already audited the 87 lines of code
with 100% code coverage.

I think less people will audit the 1500 loc difficult to read.

>
>>
>>
>>
>>
>> # hashtag
>> redirectURL doesn't work if it contains a hashtag. Popular framework
>> AngularJS, VueJS use hastag technique for routing.
>>
>
> It does. Try the admin console for instance which is angular and uses
> hashtags
>
>
I haven't tested with the latest one, but keycloak.login({ redirectUri: "
http://localhost:8000/#login" }) didn't work
Keycloak consider #login as a fragment and raised an error

>
>>
>>
>>
>> # cookies, iframe
>> This make our work as developer trully hard. Basically the simplest
>> javascript adpater, should be
>>
>> Instantiate a keycloak singleton
>> load existing jwt in localstorage if present
>> load url params to see if jwt in the url
>> if jwt is present, save in local storage
>> update the singleton
>>
>
> Don't know what you are talking about here really. There's no cookies and
> the only iframe is the session iframe and there's no other way to do that
>
>
Cookie are use as a fallback for the localStorage storage used probably to
save the JWT.
Let me explain with more detail this point.
We have in a typical application a login page.
When we instantiate the page we want to check if the user is authenticated
and redirect if so.
>From a dev perspective I just want to do:

const keycloack = Keycloak({...})
keycloak.init({...}).then(isAuthenticated => ...) .catch(...)

The complexity in the documentation between the hybrid and implicit flow,
the redirection or not on keycloak login page.
The way that keycloak.init without onLoad: "check-sso" doesn't load the JWT
token previously saved is also complex.

The library should match the default workflow and make this workflow easy.

>
>> # cordova adapter
>> I don't look at this code, but I'm just asking myself, why a javascript
>> adapter need a special code for cordova application?
>>
>
> Cordova mode opens login page in web view or browser tab, regular mode
> doesn't.
>
>
this again is crazy, really!
why not a react-native mode?
If I'm not targeting cordova, I'm obliged to have this code in my bundle?
Because the library is not decoupled into module, my bundler cannot do tree
shaking automatically.

>
>> # keycloak promise Oo
>>
>> https://www.keycloak.org/docs/latest/upgrading/index.html#javascipt-adapter-promise
>> A promise is standardized in javascript, so you should avoid using this
>> term in the code if you don't return a promise. callback is a better term.
>> A promise should return a unfulfilled method with then and catch methods
>> attach to it.
>>
>
> We have both. Look at docs and upgrade guide.
>

please, can you stop saying to look at the guide?
I read it carefully and already update my code to fix the breaking change.
The documentation is partially updated and at some place wrong.
Look at the example code here
https://www.keycloak.org/docs/latest/securing_apps/index.html#_javascript_adapter
keycloak.init().then() doesn't work. final point.

I think for this problem we can maybe create another thread but:
 * you call an object a Keycloak promise who is not a promise
(documentation problem and code problem) away async doesn't work with the
keycloak "promise"
 * you introduce a breaking change (removing the default behavior) because
callback and native promise doesn't work well together.
   but in the same time you encourage people to add a polyfill for Promises
(
https://www.keycloak.org/docs/latest/securing_apps/index.html#earlier-browsers
)?
  (ambiguity and a lot of extra code, difficult to maintain)

For me you should use only native promise
But obviously this is only my opinion

>
>
>>
>>
>>
>> In French we have a proverb: "Criticism is easy, art is difficult". So I
>> will definitely create a smaller javascript adapter that does not suffer
>> from the problems listed above. And I would come and show the code here.
>>
>> Thanks for reading me
>>
>>
>> [1] https://semver.org/
>> [2]
>>
>> https://github.com/redhat-cip/dci-ui/commit/e4d0208128afa05c33e72abe606cdcd93594c93e
>> [3] https://issues.jboss.org/browse/KEYCLOAK-8839
>> [4]
>>
>> https://github.com/keycloak/keycloak/blob/master/adapters/oidc/js/src/main/resources/keycloak.js#L882-L893
>> [5] https://github.com/kelektiv/node-uuid
>>
>> --
>>
>> Guillaume  Vincent
>>
>> Senior FrontEnd Engineer
>>
>> gvincent at redhat.com
>> <https://red.ht/sig>
>> TRIED. TESTED. TRUSTED. <https://redhat.com/trusted>
>> _______________________________________________
>> keycloak-dev mailing list
>> keycloak-dev at lists.jboss.org
>> https://lists.jboss.org/mailman/listinfo/keycloak-dev
>>
>

I don't know how many javascript devs do some web application and embed
keycloak in their application.
But my experience was painful.
Maybe it's only me so keep doing what your are doing.
I will rewrite a module for my need.

Cheers

Guillaume