6:21 a.m.
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.
# 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.
# 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]
# 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
# 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();
}
# hashtag
redirectURL doesn't work if it contains a hashtag. Popular framework
AngularJS, VueJS use hastag technique for routing.
# 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
# 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?
# keycloak promise Oo
https://www.keycloak.org/docs/latest/upgrading/index.html#javascipt-adapt...
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.
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/e4d0208128afa05c33e72abe606cd...
[3] https://issues.jboss.org/browse/KEYCLOAK-8839
[4]
https://github.com/keycloak/keycloak/blob/master/adapters/oidc/js/src/mai...
[5] https://github.com/kelektiv/node-uuid
--
Guillaume Vincent
Senior FrontEnd Engineer
gvincent(a)redhat.com
<https://red.ht/sig>
TRIED. TESTED. TRUSTED. <https://redhat.com/trusted>
5:11 a.m.
Thanks for your feedback, some comments below:
On Wed, 21 Nov 2018, 13:26 Guillaume Vincent <gvincent(a)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-adapt...
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/e4d0208128afa05c33e72abe606cd...
[3] https://issues.jboss.org/browse/KEYCLOAK-8839
[4]
https://github.com/keycloak/keycloak/blob/master/adapters/oidc/js/src/mai...
[5] https://github.com/kelektiv/node-uuid
--
Guillaume Vincent
Senior FrontEnd Engineer
gvincent(a)redhat.com
<https://red.ht/sig>
TRIED. TESTED. TRUSTED. <https://redhat.com/trusted>
_______________________________________________
keycloak-dev mailing list
keycloak-dev(a)lists.jboss.org
https://lists.jboss.org/mailman/listinfo/keycloak-dev
6:46 a.m.
On Thu, Nov 22, 2018 at 12:11 PM Stian Thorgersen <sthorger(a)redhat.com>
wrote:
Thanks for your feedback, some comments below:
On Wed, 21 Nov 2018, 13:26 Guillaume Vincent <gvincent(a)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-adapt...
> 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...
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-bro...
)?
(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/e4d0208128afa05c33e72abe606cd...
> [3] https://issues.jboss.org/browse/KEYCLOAK-8839
> [4]
>
>
https://github.com/keycloak/keycloak/blob/master/adapters/oidc/js/src/mai...
> [5] https://github.com/kelektiv/node-uuid
>
> --
>
> Guillaume Vincent
>
> Senior FrontEnd Engineer
>
> gvincent(a)redhat.com
> <https://red.ht/sig>
> TRIED. TESTED. TRUSTED. <https://redhat.com/trusted>
> _______________________________________________
> keycloak-dev mailing list
> keycloak-dev(a)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
7:21 a.m.
On Thu, Nov 22, 2018 at 1:47 PM Guillaume Vincent <gvincent(a)redhat.com>
wrote:
On Thu, Nov 22, 2018 at 12:11 PM Stian Thorgersen
<sthorger(a)redhat.com>
wrote:
> Thanks for your feedback, some comments below:
>
> On Wed, 21 Nov 2018, 13:26 Guillaume Vincent <gvincent(a)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-adapt...
>> 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...
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-bro...
)?
(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/e4d0208128afa05c33e72abe606cd...
>> [3] https://issues.jboss.org/browse/KEYCLOAK-8839
>> [4]
>>
>>
https://github.com/keycloak/keycloak/blob/master/adapters/oidc/js/src/mai...
>> [5] https://github.com/kelektiv/node-uuid
>>
>> --
>>
>> Guillaume Vincent
>>
>> Senior FrontEnd Engineer
>>
>> gvincent(a)redhat.com
>> <https://red.ht/sig>
>> TRIED. TESTED. TRUSTED. <https://redhat.com/trusted>
>> _______________________________________________
>> keycloak-dev mailing list
>> keycloak-dev(a)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(a)lists.jboss.org
https://lists.jboss.org/mailman/listinfo/keycloak-dev
7:31 a.m.
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(a)redhat.com> wrote:
On Thu, Nov 22, 2018 at 1:47 PM Guillaume Vincent <gvincent(a)redhat.com>
wrote:
> On Thu, Nov 22, 2018 at 12:11 PM Stian Thorgersen <sthorger(a)redhat.com>
> wrote:
>
> > Thanks for your feedback, some comments below:
> >
> > On Wed, 21 Nov 2018, 13:26 Guillaume Vincent <gvincent(a)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-adapt...
> >> 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...
> 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-bro...
> )?
> (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/e4d0208128afa05c33e72abe606cd...
> >> [3] https://issues.jboss.org/browse/KEYCLOAK-8839
> >> [4]
> >>
> >>
>
https://github.com/keycloak/keycloak/blob/master/adapters/oidc/js/src/mai...
> >> [5] https://github.com/kelektiv/node-uuid
> >>
> >> --
> >>
> >> Guillaume Vincent
> >>
> >> Senior FrontEnd Engineer
> >>
> >> gvincent(a)redhat.com
> >> <https://red.ht/sig>
> >> TRIED. TESTED. TRUSTED. <https://redhat.com/trusted>
> >> _______________________________________________
> >> keycloak-dev mailing list
> >> keycloak-dev(a)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(a)lists.jboss.org
> https://lists.jboss.org/mailman/listinfo/keycloak-dev
>
--
Guillaume Vincent
Senior FrontEnd Engineer
gvincent(a)redhat.com
<https://red.ht/sig>
TRIED. TESTED. TRUSTED. <https://redhat.com/trusted>
2:51 a.m.
I open a bug related to the promise issue.
My comment on the documentation not fully updated will be fixed by
https://github.com/keycloak/keycloak-documentation/commit/da8ce0026eaae5e...
But the documentation is not updated (yet?)
2749
days inactive
2751
days old
5 comments
3 participants
participants (3)
-
Guillaume Vincent -
Sebastien Blanc -
Stian Thorgersen