[keycloak-dev] Javascript Adapter Feedback
Guillaume Vincent
gvincent at redhat.com
Thu Nov 22 08:31:22 EST 2018
a friend of mine told me that I looked angry.
I apologize, I did not want to sound harsh.
I specify that I do not target anyone in particular when I give my opinion
on the quality of the code.
I'll put more smiley next time :)
Cheers
On Thu, Nov 22, 2018 at 2:21 PM Sebastien Blanc <sblanc at redhat.com> wrote:
>
>
>
> 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
>>
>
--
Guillaume Vincent
Senior FrontEnd Engineer
gvincent at redhat.com
<https://red.ht/sig>
TRIED. TESTED. TRUSTED. <https://redhat.com/trusted>
More information about the keycloak-dev
mailing list