TOMEE-4284 - Implement tomee.mp.jwt.allow.no-exp property over mp.jwt.tomee.allow.no-exp

[tichovz]

…no-exp

[tichovz]

Should I change the property name in the JWT TCK?

https://github.com/apache/tomee/blob/main/tck/microprofile-tck/jwt/src/test/java/org/apache/tomee/microprofile/tck/jwt/validation/ExpClaimAllowMissingExpValidationTest.java#L89

[rzo1]

Do we have an issue for this one @tichovz ?

[tichovz]

Hi!

I have no issues for this, but David asked if could take a look at it.

On Nov 9, 2022, at 10:29 AM, Zoltán Tichov <[email protected]> wrote:

Hi!

Is there another task that could be taken care of?

There's a change in the same code that's on my "I should really find the time to fix that" list if you want to dig in.

Basically, we added a TomEE-specific property mp.jwt.tomee.allow.no-exp. We likely should avoid putting custom properties in >the mp.jwt.* namespace and likely we should:

• rename it to something that starts with tomee like say tomee.mp.jwt.allow.no-exp
• ensure both properties work for backwards compatibility
• tomee.mp.jwt.allow.no-exp would win if both were defined
• any use of mp.jwt.tomee.allow.no-exp should get a warning log message
• create an itest or two in itests/microprofile-jwt-itests/ that uses the property
• update docs/microprofile/jwt.adoc
• File JIRA cause I haven't done that yet, LOL :)

The runtime change will be a piece of cake for you. Most the work would be in the itest, which could be kind of new/fun to do.

Thoughts?

[rzo1]

Thanks. That was the missing link, so maybe you can just rebase and we do a CI build to see, if it breaks anything?

[tichovz]

Thanks, Would I make a rebase with the main branch or the 9.x branch?

[rzo1]

Let's test against main and if it looks nice, we should backport to 9.1.x too

[tichovz]

I made a rebase to main and it seems ok.

[rmannibucau]

    private Boolean queryAllowExp(){
        return config.getOptionalValue("tomee.mp.jwt.allow.no-exp", Boolean.class)
                .or(() -> config.getOptionalValue("mp.jwt.tomee.allow.no-exp", Boolean.class)
                        .map(value -> {
                            CONFIGURATION.warning("mp.jwt.tomee.allow.no-exp property is deprecated, use tomee.mp.jwt.allow.no-exp propert instead.");
                            return value;
                        }))
                .orElse(false);
    }

to avoid to read both entries all the time (Config can be slow depending the ConfigSource) and to avoid the AtomicBoolean which is not needed?

[tichovz]

Thanks for the improvement.

[rzo1]

@tichovz Can you include the feedback provided by Romain? :)