[FIX] Add attempt and recover to prevent ftl from crashing

[Romcol]

Hi,

I have notice 500 errors on some of my customs .ftl in some scenarios (update-user-profile.ftl , idp-review-user-profile.ftl) probably due to the data_model not being converted in a successfully way.
Adding <#attempt> and <#recover> inside the whole function prevent at least the page from crashing and going 500.

Thanks for merging!

[garronej]

Hi @Romcol,
Thank you for the PR!
A few things:

• All the new work should now be done on the v6 branch. v6 should be released in the next few days. (It's already available on NPM in beta)
• Please keep the diff as small as possible, I can't see what you have changed since you have changed the indentation.
• We should avoid wrapping the whole block into an attemp/recover block but instead wrap every instruction that can possibly throw within it's own attempt/recover. I thought I did that but obviously there must be a hole somewhere since you get a 500. Let's find where this hole is!
• Unfortunately, while they avoid getting a 500 atempt/recover aren't a sollution since Exception are logged to the Keycloak logs no matter what, it's impossible to avoid it. As a concequence we should figure out what exactly is causing the error and make a special case for it. See all those lines are special cases.

PS: Thank you for noticing the old warning about the documentation website being down.

[Romcol]

Hi @garronej,

Thanks for the reply. Ok I can make a PR from V6 branch.

It's very hard to debug, on my page there's an issue somewhere in some nested enumerable. Wrapping
<#local rec_out = ftl_object_to_js_code_declaring_an_object(array_item, path + [ i ])> fixes the issue as well.
Regarding Keycloak exceptions there are inreadable, they simply state that there's an issue within ftl_object_to_js_code_declaring_an_object on the first line of the function.
If you have any method on how to debug the data_model, I would be glad to hear it. Otherwise it will be hard to look into it.

[garronej]

@Romcol,

Thanks for the update,
If you can give me as much context as possible it would help greatly.
A clear reproduction path === a quick fix from me.

[Romcol]

Hi @garronej,

Here are the steps to reproduce.
I have two realms, an administration realm and a platform realm. The plateform realm has the admin realm connected as identity provider. When we connect on the platform realm via the admin realm, and when some fields (e.g. lastName) are missing in the admin realm user we are redirected to this template update-user-profile.ftl to fill the missing field(s).

Without the fix in this PR the page goes Error 500. With the fix we are able to show the page.

I've had similar issue with idp-review-user-profile.ftl (#86).

I have updated my PR to show minimal changelog. Since it's a bugfix, I think it would be better to release it as V5 or we would have to migrate to V6 which will cause more work and QA/testing....

Thanks for reading,

[garronej]

@Romcol I didn't forget you, I'll review and merge this very soon.
V6 have been released btw check it out. :)

[garronej]

I'm implementing update-user-profile.ftl I'll will adress this immediatly after.

[garronej]

I have updated my PR to show minimal changelog. Since it's a bugfix, I think it would be better to release it as V5 or we would have to migrate to V6 which will cause more work and QA/testing....

I don't know what to think about this.
On the one hand I see you point, updating is a pain but I would prefer to have a clear policy of not updating older major since it's very time consuming for me, every time there is a new issue, to figure out if it's something that has already been fixed or not.
I work hard to improve Keycloakify, I want pepole to upgrade...
Especially from v5 to v6 since the performance are so much better in the new release...

[Romcol]

Hi @garronej ,

Thanks for the review. I understand that you would want us to upgrade. I personally don't mind if it's only merged on the V6 branch instead of both branches even if I would prefer on both branches :). I think this fix can be easily cherry-picked from my PR to the V6 branch, but tell me if you need help in doing so.

[garronej]

Hi @Romcol,
I changed my mind about this, I will release the fix for v5 as well.
I didn't merge the PR yet because so far I didn't manage to reproduce and I would like to snipe the exact instruction that throws the error instead of wrapping the all function call.

I'm on it!

[garronej]

I don't know if you saw but I implemented pdate-user-profile.ftl and idp-review-user-profile.ftl, with client side validation based on attributes.

[garronej]

Hi @Romcol,
I have released an update to v5 that includes your fix.
For v6 however I would like to be able to reproduce the error myself.

The plateform realm has the admin realm connected as identity provider.
I don't know how to do that... would you provide me with a short video or a step by step guide on how to set this up?

Thanks very much for your help.

[Romcol]

Hi @garronej,

Thanks for merging!

We've done a short video here to reproduce our case : https://vimeo.com/748716173.
Apparently you don't need to set up an identity provider to reproduce the bug.

These are the steps to reproduce:

1. Add user with a missing field (either firstname or lastname)
2. Email verified
3. Set credentials
4. Log in with credentials
5. Update account

Hope it'll help!

[garronej]

I reproduced the bug and I came up with a clean fix. 🥳
Thank you for your help and your pacience.

[Romcol]

Nice, I'll try it!