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

update jwt #245

Merged
merged 15 commits into from
Apr 24, 2020
Merged

update jwt #245

merged 15 commits into from
Apr 24, 2020

Conversation

vapadwal
Copy link
Member

@vapadwal vapadwal commented Apr 23, 2020

identified few issues while integrating with mtsj , please see mtsj PR for the same devonfw/my-thai-star#334

@vapadwal vapadwal requested a review from hohwille April 23, 2020 11:22
Copy link
Member

@hohwille hohwille left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for PR 👍
I approve as the PR is acceptable for merge. However, please check my review comment for removing the TOKEN_PREFIX.

@@ -45,7 +48,7 @@ public JwtManagerImpl() {
public Jws<Claims> decode(String jwt) {

PublicKey key = this.keyStoreAccess.getPublicKey(this.jwtConfig.getAlias());
Jws<Claims> token = Jwts.parser().setSigningKey(key).parseClaimsJws(jwt);
Jws<Claims> token = Jwts.parser().setSigningKey(key).parseClaimsJws(jwt.replace(JwtConstants.TOKEN_PREFIX, ""));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure about this. Actually the provided jwt argument should be the JWT itself. The code I provided already removes the "Bearer" prefix elsewhere. Surely it wont hurt to also do it here but what if there are multiple whitespaces around or the Upper/Lower-Case does not match the TOKEN_PREFIX. Therefore it would IMHO be better to have only one place in the code that does this. If you want to reuse here, you could expose as static mehtod in the other impl and reuse here... WDYT?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you are right, problem is that i test this with mtsj. while debugging i found , there multipe (two) Bearer in token

Bearer Bearer eyJhbGciOiJSUzI1NiJ9.eyJzdWIiOiJ3YWl0ZXIiLCJyb2xlcyI6IlJPTEVfV2FpdGVyLG10cy5GaW5kQm9va2luZyxtdHMuRmluZE9yZGVyIiwiaXNzIjoiZGV2b25mdyIsImlhdCI6MTU4NzY1OTg5NywibmJmI-----------

basically mtsj angular adds this extra Bearer keyword to the token

latMap((token) => {
const authReq = !!token
? req.clone({
setHeaders: { Authorization: 'Bearer ' + token },
})

I will remove the code (jwt.replace(JwtConstants.TOKEN_PREFIX, "")) from JwtManagerImpl

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Than angular client also is correct. The token itself should not contain Bearer. Maybe the code to extract the token on the client is broken or I did something wrong in JwtLoginFilter so we added Bearer prefix twice (or maybe it does not belong to response header at all)? This part is new to me, however I did not intend to change anything of this behaviour from your code. Maybe it was wrong before or I messed something during refactoring.

@@ -25,6 +27,7 @@
*
* @since 2020.04.001
*/
@Named
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My vision was that we also create a spring-boot starter.
The @Named only helps if the user does a component-scan in this package within its spring-config.
A spring-starter would therefore much better.
Anyhow good that you readded this annotation here. It can only help but not hurt! So thanks for this fix.

Copy link
Member Author

@vapadwal vapadwal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In documentation

<dependency>
	<groupId>com.devonfw.java.modules</groupId>
	<artifactId>devon4j-starter-security-jwt</artifactId>
</dependency>

it should be

<dependency>
	     <groupId>com.devonfw.java.starters</groupId>
        <artifactId>devon4j-starter-security-jwt</artifactId>
    </dependency>

Also
http.addFilterBefore(getJwtAuthenticationFilter(), UsernamePasswordAuthenticationFilter.class);

it should be getJwtLoginFilter()

@hohwille
Copy link
Member

In documentation

<dependency>
	<groupId>com.devonfw.java.modules</groupId>
	<artifactId>devon4j-starter-security-jwt</artifactId>
</dependency>

it should be

<dependency>
	     <groupId>com.devonfw.java.starters</groupId>
        <artifactId>devon4j-starter-security-jwt</artifactId>
    </dependency>

Also
http.addFilterBefore(getJwtAuthenticationFilter(), UsernamePasswordAuthenticationFilter.class);

it should be getJwtLoginFilter()

You can fix this directly next time.
I just comitted the changes to your fork.
Thanks for the review feedback and sorry for my mistakes. Too much hussle at the moment. So great that with our review process we find such issues.

Copy link
Member

@hohwille hohwille left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All looking fine to me now. Thanks @vapadwal 👍

@hohwille hohwille added this to the release:2020.04.001 milestone Apr 24, 2020
@hohwille hohwille added documentation Guides, tutorials, readmes, etc. enhancement New feature or request security labels Apr 24, 2020
@hohwille hohwille merged commit 4443da5 into devonfw:develop Apr 24, 2020
@vapadwal vapadwal mentioned this pull request May 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Guides, tutorials, readmes, etc. enhancement New feature or request security
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants