[keycloak-dev] Javascript Adapter Feedback

Thu Nov 22 06:11:13 EST 2018

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.

>
>
>
> # 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.

> # 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.

> # 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.

>
> # 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.

>
>
>
>
> # 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

>
>
>
> # 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

> # 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.

> # 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.

>
>
>
> 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
>