WIP: addition of module for jwt and kerberos #51
Conversation
|
|
There was a problem hiding this comment.
@ssarmokadam Thank you for your work.
During the review I started to understand that you wanted to get this done to give your work in progress to the community. This is great. I initially expected this PR to be complete to be merged into the official repo. Therefore do not get offended by my many picky review comments.
Instead we should clarify how to proceed. AFAIK you might not be available to do all the rework. Is that correct? Then we should find someone else to take over the rework.
Thanks for your input and bringing this important task forward! 👍
| <!-- https://mvnrepository.com/artifact/io.jsonwebtoken/jjwt --> | ||
| <dependency> | ||
| <groupId>io.jsonwebtoken</groupId> | ||
| <artifactId>jjwt</artifactId> |
There was a problem hiding this comment.
I was using https://mvnrepository.com/artifact/org.springframework.security/spring-security-jwt so far. I assume however this is quite exchangeable.
| <dependency> | ||
| <groupId>io.jsonwebtoken</groupId> | ||
| <artifactId>jjwt</artifactId> | ||
| <version>0.7.0</version> |
There was a problem hiding this comment.
We should always move versions to dependencyManagement in our BOM (to bring this also to the projects and avoid redundancy)
| <artifactId>javax.activation-api</artifactId> | ||
| </dependency> | ||
| </dependencies> | ||
| </profile> |
There was a problem hiding this comment.
Nice that you care about Java9+. However, we should address this more general in our parent POM of the modules as this more or less applies to all modules. Or to even go further, we should simply include these dependencies independent of the JDK version.
For your info:
- We are using flatten plugin that will remove this profile anyway and embed dependency depending on the JVM used in our build and release process.
- You can also define open version ranges. Limiting this to Java 12 seems kind of a random decision to me.
There was a problem hiding this comment.
So to summarize: Simply add these dependencies as regular dependencies without profile. Support for Java8 is going to run out anyhow and the additional deps will not hurt. Benefit is that we create results that run on all JVMs (hopefully). See #16.
| @@ -0,0 +1,28 @@ | |||
| package com.devonfw.module.security.common; | |||
|
|
|||
| public class AccountCredentials { | |||
There was a problem hiding this comment.
Can we just use an existing class for this (e.g. from Spring as this module is spring-specific anyhow)?
| * Filter for Json Web Tokens Authentication | ||
| * | ||
| */ | ||
| public class JWTAuthenticationFilter extends GenericFilterBean { |
There was a problem hiding this comment.
should be JwtAuthenticationFilter see https://github.com/devonfw/devon4j/wiki/coding-conventions#naming
| /** | ||
| * | ||
| * | ||
| */ |
| private String keytabLocation; | ||
|
|
||
| @Value("${kerberos.service-principal}") | ||
| private String servicePrincipalName; |
There was a problem hiding this comment.
Instead we need to create a KerberosConfigProperties class. Using @ConfigurationProperties(prefix = "kerberos") you can configure the config prefix. Should IMHO also be more specific. Many people do not understand the context as they might not have heared about kerberos. Hence the prefix should at least be security.kerberos. Maybe even more specific.
Here is an example (with empty prefix however)
https://github.com/devonfw/devon4j/blob/develop/modules/service/src/main/java/com/devonfw/module/service/common/base/config/ServiceConfigProperties.java#L36
| // TODO:: generalise properties injected | ||
| // TODO: Extend SpnegoEntryPoint and create a class where you can handle errors | ||
| // TODO:: AbstractAuthenticationProvider class needs to be created | ||
| // TOD: kerberos config class |
There was a problem hiding this comment.
Aha, seems the above was planned but maybe out of time...
| // .logout().logoutSuccessUrl("/login.html").and(). | ||
| // | ||
| // addFilterBefore( | ||
| // spnegoAuthenticationProcessingFilter(authenticationManagerBean()), BasicAuthenticationFilter.class); |
There was a problem hiding this comment.
OK, I am repeating myself. Clean code, redundant code, etc.
| SunJaasKerberosTicketValidator ticketValidator = new SunJaasKerberosTicketValidator(); | ||
| ticketValidator.setServicePrincipal(this.kerbprop.getServicePrincipalName()); | ||
| ticketValidator.setKeyTabLocation(new FileSystemResource(this.kerbprop.getKeytabLocation())); | ||
| ticketValidator.setDebug(true); |
There was a problem hiding this comment.
Should not be hardcoded but configurable.
hohwille
changed the title
|
closing this as create issue for the same -#262 |
This PR includes module for jwt and kerberos for authentication of user. I was able to test JWT module successfully. In kerberos I have created dummy userdetails which try to login in kerberos using JAAS. Here I was facing some issues. So some changes might be needed in kerberos module.