You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@tomee.apache.org by GitBox <gi...@apache.org> on 2022/12/18 11:40:49 UTC
[GitHub] [tomee] tichovz opened a new pull request, #990: Implement tomee.mp.jwt.allow.no-exp property over mp.jwt.tomee.allow.…
tichovz opened a new pull request, #990:
URL: https://github.com/apache/tomee/pull/990
…no-exp
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@tomee.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] TOMEE-4284 - Implement tomee.mp.jwt.allow.no-exp property over mp.jwt.tomee.allow.no-exp (tomee)
Posted by Richard Zowalla <rz...@apache.org>.
Hi,
it might be related to the recent changes related to logging.
Fixing it would be great to avoid breaking the build.
Gruß
Richard
Am Mittwoch, dem 29.11.2023 um 10:01 +0100 schrieb Zoltán Tichov:
> Hi!
>
> Yes, I can include, but I made itest that were all green under 9.0
> but they
> failed under 10.x.
> Errors may be caused by the server issuing different log messages in
> version 10 than in version 9.
> Should I fix them or is it OK later?
>
> Thanks:
> Zoltán
>
>
> On Wed, Nov 29, 2023 at 8:37 AM rzo1 (via GitHub) <gi...@apache.org>
> wrote:
>
> >
> > rzo1 commented on code in PR #990:
> > URL:
> > https://github.com/apache/tomee/pull/990#discussion_r1408864908
> >
> >
> > ##########
> >
> > mp-
> > jwt/src/main/java/org/apache/tomee/microprofile/jwt/config/JWTAuthC
> > onfigurationProperties.java:
> > ##########
> > @@ -117,6 +119,15 @@ private JWTAuthConfiguration
> > createJWTAuthConfiguration() {
> >
> > config.getOptionalValue("mp.jwt.decrypt.key.algorithm",
> > String.class).orElse(null),
> >
> > config.getOptionalValue("mp.jwt.verify.publickey.algorithm",
> > String.class).orElse(null));
> > }
> > +
> > + private Boolean queryAllowExp(){
> >
> > Review Comment:
> > @tichovz Can you include the feedback provided by Romain? :)
> >
> >
> >
> > --
> > This is an automated message from the Apache Git Service.
> > To respond to the message, please log on to GitHub and use the
> > URL above to go to the specific comment.
> >
> > To unsubscribe, e-mail: dev-unsubscribe@tomee.apache.org
> >
> > For queries about this service, please contact Infrastructure at:
> > users@infra.apache.org
> >
> >
Re: [PR] TOMEE-4284 - Implement tomee.mp.jwt.allow.no-exp property over mp.jwt.tomee.allow.no-exp (tomee)
Posted by Zoltán Tichov <zo...@gmail.com>.
Hi!
Yes, I can include, but I made itest that were all green under 9.0 but they
failed under 10.x.
Errors may be caused by the server issuing different log messages in
version 10 than in version 9.
Should I fix them or is it OK later?
Thanks:
Zoltán
On Wed, Nov 29, 2023 at 8:37 AM rzo1 (via GitHub) <gi...@apache.org> wrote:
>
> rzo1 commented on code in PR #990:
> URL: https://github.com/apache/tomee/pull/990#discussion_r1408864908
>
>
> ##########
>
> mp-jwt/src/main/java/org/apache/tomee/microprofile/jwt/config/JWTAuthConfigurationProperties.java:
> ##########
> @@ -117,6 +119,15 @@ private JWTAuthConfiguration
> createJWTAuthConfiguration() {
> config.getOptionalValue("mp.jwt.decrypt.key.algorithm",
> String.class).orElse(null),
>
> config.getOptionalValue("mp.jwt.verify.publickey.algorithm",
> String.class).orElse(null));
> }
> +
> + private Boolean queryAllowExp(){
>
> Review Comment:
> @tichovz Can you include the feedback provided by Romain? :)
>
>
>
> --
> This is an automated message from the Apache Git Service.
> To respond to the message, please log on to GitHub and use the
> URL above to go to the specific comment.
>
> To unsubscribe, e-mail: dev-unsubscribe@tomee.apache.org
>
> For queries about this service, please contact Infrastructure at:
> users@infra.apache.org
>
>
Re: [PR] TOMEE-4284 - Implement tomee.mp.jwt.allow.no-exp property over mp.jwt.tomee.allow.no-exp (tomee)
Posted by "rzo1 (via GitHub)" <gi...@apache.org>.
rzo1 commented on code in PR #990:
URL: https://github.com/apache/tomee/pull/990#discussion_r1408864908
##########
mp-jwt/src/main/java/org/apache/tomee/microprofile/jwt/config/JWTAuthConfigurationProperties.java:
##########
@@ -117,6 +119,15 @@ private JWTAuthConfiguration createJWTAuthConfiguration() {
config.getOptionalValue("mp.jwt.decrypt.key.algorithm", String.class).orElse(null),
config.getOptionalValue("mp.jwt.verify.publickey.algorithm", String.class).orElse(null));
}
+
+ private Boolean queryAllowExp(){
Review Comment:
@tichovz Can you include the feedback provided by Romain? :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@tomee.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Implement tomee.mp.jwt.allow.no-exp property over mp.jwt.tomee.allow.… (tomee)
Posted by "tichovz (via GitHub)" <gi...@apache.org>.
tichovz commented on PR #990:
URL: https://github.com/apache/tomee/pull/990#issuecomment-1818743070
I made a rebase to main and it seems ok.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@tomee.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] TOMEE-4284 - Implement tomee.mp.jwt.allow.no-exp property over mp.jwt.tomee.allow.no-exp (tomee)
Posted by "tichovz (via GitHub)" <gi...@apache.org>.
tichovz closed pull request #990: TOMEE-4284 - Implement tomee.mp.jwt.allow.no-exp property over mp.jwt.tomee.allow.no-exp
URL: https://github.com/apache/tomee/pull/990
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@tomee.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Implement tomee.mp.jwt.allow.no-exp property over mp.jwt.tomee.allow.… (tomee)
Posted by "rzo1 (via GitHub)" <gi...@apache.org>.
rzo1 commented on PR #990:
URL: https://github.com/apache/tomee/pull/990#issuecomment-1812386914
Let's test against main and if it looks nice, we should backport to 9.1.x too
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@tomee.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Implement tomee.mp.jwt.allow.no-exp property over mp.jwt.tomee.allow.… (tomee)
Posted by "rmannibucau (via GitHub)" <gi...@apache.org>.
rmannibucau commented on code in PR #990:
URL: https://github.com/apache/tomee/pull/990#discussion_r1398970401
##########
mp-jwt/src/main/java/org/apache/tomee/microprofile/jwt/config/JWTAuthConfigurationProperties.java:
##########
@@ -117,6 +119,15 @@ private JWTAuthConfiguration createJWTAuthConfiguration() {
config.getOptionalValue("mp.jwt.decrypt.key.algorithm", String.class).orElse(null),
config.getOptionalValue("mp.jwt.verify.publickey.algorithm", String.class).orElse(null));
}
+
+ private Boolean queryAllowExp(){
Review Comment:
```
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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@tomee.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Implement tomee.mp.jwt.allow.no-exp property over mp.jwt.tomee.allow.… (tomee)
Posted by "tichovz (via GitHub)" <gi...@apache.org>.
tichovz commented on code in PR #990:
URL: https://github.com/apache/tomee/pull/990#discussion_r1399037910
##########
mp-jwt/src/main/java/org/apache/tomee/microprofile/jwt/config/JWTAuthConfigurationProperties.java:
##########
@@ -117,6 +119,15 @@ private JWTAuthConfiguration createJWTAuthConfiguration() {
config.getOptionalValue("mp.jwt.decrypt.key.algorithm", String.class).orElse(null),
config.getOptionalValue("mp.jwt.verify.publickey.algorithm", String.class).orElse(null));
}
+
+ private Boolean queryAllowExp(){
Review Comment:
Thanks for the improvement.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@tomee.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tomee] tichovz commented on pull request #990: Implement tomee.mp.jwt.allow.no-exp property over mp.jwt.tomee.allow.…
Posted by GitBox <gi...@apache.org>.
tichovz commented on PR #990:
URL: https://github.com/apache/tomee/pull/990#issuecomment-1356781783
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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@tomee.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Implement tomee.mp.jwt.allow.no-exp property over mp.jwt.tomee.allow.… (tomee)
Posted by "rzo1 (via GitHub)" <gi...@apache.org>.
rzo1 commented on PR #990:
URL: https://github.com/apache/tomee/pull/990#issuecomment-1812357259
Thanks. That was the missing link, so maybe you can just rebase and we do a CI build to see, if it breaks anything?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@tomee.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Implement tomee.mp.jwt.allow.no-exp property over mp.jwt.tomee.allow.… (tomee)
Posted by "tichovz (via GitHub)" <gi...@apache.org>.
tichovz commented on PR #990:
URL: https://github.com/apache/tomee/pull/990#issuecomment-1812361136
Thanks, Would I make a rebase with the main branch or the 9.x branch?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@tomee.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Implement tomee.mp.jwt.allow.no-exp property over mp.jwt.tomee.allow.… (tomee)
Posted by "tichovz (via GitHub)" <gi...@apache.org>.
tichovz commented on PR #990:
URL: https://github.com/apache/tomee/pull/990#issuecomment-1812354597
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 <[zoltan.tichov@gmail.com](mailto:zoltan.tichov@gmail.com)> 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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@tomee.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Implement tomee.mp.jwt.allow.no-exp property over mp.jwt.tomee.allow.… (tomee)
Posted by "rzo1 (via GitHub)" <gi...@apache.org>.
rzo1 commented on PR #990:
URL: https://github.com/apache/tomee/pull/990#issuecomment-1812319594
Do we have an issue for this one @tichovz ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@tomee.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org