[keycloak-dev] Javascript Adapter Feedback

Thu Nov 22 08:21:16 EST 2018

On Thu, Nov 22, 2018 at 1:47 PM Guillaume Vincent <gvincent at redhat.com>
wrote:

> 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.
>
You can also open some tickets and provide pull requests, we will be more
than happy to review them there and have some serene discussions.

>
> Cheers
>
> Guillaume
> _______________________________________________
> keycloak-dev mailing list
> keycloak-dev at lists.jboss.org
> https://lists.jboss.org/mailman/listinfo/keycloak-dev
>