Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow retrieving claims by using the Claims enum #154

Closed
andreas-eberle opened this issue Feb 28, 2020 · 18 comments · Fixed by #155 or #158
Closed

Allow retrieving claims by using the Claims enum #154

andreas-eberle opened this issue Feb 28, 2020 · 18 comments · Fixed by #155 or #158
Milestone

Comments

@andreas-eberle
Copy link
Contributor

I think it would be a good idea to add a method JsonWebToken.getClaim(Claims claim) to allow retrieving claims by using the Claims enum. At the moment one is forced to write unnecessarily bloated code like jwt.getClaim(Claims.groups.name()). This is even true for the default methods given in the interface JsonWebToken. Adding this method could make code more readable.

Let me know if you want a PR for this.

@sberyozkin sberyozkin added this to the JWT-2.0 milestone Feb 28, 2020
@sberyozkin
Copy link
Contributor

@andreas-eberle +1, 1.2 is the next version so we can't get the implementers updating the existing working code, but for 2.0, yeah, why not. Though it would need to be tied with #100.
Thanks

@rdebusscher
Copy link
Member

This is a compatible change since it just adds a method, so it can be included in 1.2

@sberyozkin
Copy link
Contributor

sberyozkin commented Feb 28, 2020

@rdebusscher it is compatible for the users but requires the implementations to update their existing implementation code. I don't really mind to be honest from our own implementation's point of vew, but we can't just do it for the minor release ?

@rdebusscher
Copy link
Member

It is no problem that a minor release require a change to the implementation code. In fact, that is the general rule, otherwise not much will change.

However, It can't introduce a breaking change.

So I agree that this change, small (only a couple of lines) but very helpful for the developers.

@sberyozkin
Copy link
Contributor

@rdebusscher That said the focus is on the users, so well, I don't mind it be included in 1.2 then.

@sberyozkin
Copy link
Contributor

Sounds good. @andreas-eberle Go ahead please with your PR :-)

@andreas-eberle
Copy link
Contributor Author

I added the methods with default implementations forwarding to the old methods. So the code is doing exactly the same and other implementations also should not have to adapt anything.

@sberyozkin sberyozkin modified the milestones: JWT-2.0, JWT-1.2 Feb 28, 2020
@sberyozkin
Copy link
Contributor

Very good, thanks

@sberyozkin
Copy link
Contributor

@andreas-eberle @rdebusscher
Please see #156 for the reasons why it had to be reopened. The PR for this issue needs to have a baseline plugin updated too. For now I'm changing the milestone to 2.0 but will be happy to put back to 1.1

@sberyozkin sberyozkin modified the milestones: JWT-1.2, JWT-2.0 Feb 29, 2020
@sberyozkin
Copy link
Contributor

sberyozkin commented Feb 29, 2020

My understanding of this plugin is that the interfaces can not be changed for a given baseline, any idea how it can be fixed ?

@sberyozkin
Copy link
Contributor

sberyozkin commented Feb 29, 2020

MP Config has this configuration:

<configuration>
                    <base>
                        <version>1.3</version>
                    </base>
</configuration>

But I'm not sure what can we put there in our case to allow new method additions to the existing interfaces available in 1.1.1. Has someone dealt with a similar issue before ? I can do a PR myself once I know how to fix it

@andreas-eberle
Copy link
Contributor Author

From what I see in the pom, the used bnd plugin versions seem very old (October 2017) while there exist a newer version 5.0 from January 2020. Maybe these problems are fixed in the newer versions?

@andreas-eberle
Copy link
Contributor Author

@sberyozkin, @rdebusscher : It seems the relevant version is actually the version in the annotation here https://github.com/andreas-eberle/microprofile-jwt-auth/blob/add-get-claims-with-claims/api/src/main/java/org/eclipse/microprofile/jwt/package-info.java#L40 and not the dependency version.

The plugin wants us to increase this version from 1.0 to 1.1. It does not accept an increase from 1.0 to 1.0.1. Should I add this increase in my PR or should we try to find a way to not make this increase?

@andreas-eberle
Copy link
Contributor Author

Btw, you can get more details on the error message when using the following as configuration for the bnd-baseline-maven-plugin.

<configuration>
      <fullReport>true</fullReport>
</configuration>

@sberyozkin
Copy link
Contributor

Hi @andreas-eberle Thanks, so the next version will be 1.2 (we are on 1.2-SNAPSHOT) and the current tag is 1.1.1. In that case the package property should be 1.1.1 ? What I don't understand is why the PR build passes. I agree with @rdebusscher that we need to fix the plugin anyway for us to be able to change the existing methods in the future.
Should we remove tat osgi package property, and add a base.version 1.1.1 ?

@andreas-eberle
Copy link
Contributor Author

Although I never really worked with osgi, I think it would make sense to have the same osgi version tag as the module version. However, you have two packages under the org.eclipse.microprofile main package with both having their own package-info.java file. This will mean you sometimes will increase the version of one of these packages without changing the classes in that package. Just so you are aware of that. I don't think this is an issue.

Will you make a PR to increase the version again, or should I send you one?

@sberyozkin
Copy link
Contributor

sberyozkin commented Mar 3, 2020

Hi @andreas-eberle You may have seen a microprofile forum thread, @Emily-Jiang explained how this plugin operates. So indeed, lets do the following:

  • increase that OSGI property to 1.1 to indicate we are doing the minor changes;
  • add that full report property
  • and update to the latest version

And please resubmit your PR alongside those changes; please use mvn clean install to verify.
I've said I can do a PR myself, would appreciate though if you could do it all as part of a single PR

Thanks

@andreas-eberle
Copy link
Contributor Author

@sberyozkin: I added the changes to my branch and opened a new PR #158.

@sberyozkin sberyozkin modified the milestones: JWT-2.0, JWT-1.2 Mar 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment