PAYARA-2575 Yubikey Integration #2702
Conversation
appserver/featuresets/payara/pom.xml
Outdated
| @@ -138,5 +138,12 @@ | |||
| <version>${project.version}</version> | |||
| <type>zip</type> | |||
| </dependency> | |||
| <!-- Yubikey Authentication --> | |||
There was a problem hiding this comment.
Copyright needs updating in this file
| * | ||
| * When distributing the software, include this License Header Notice in each | ||
| * file and include the License file at packager/legal/LICENSE.txt. | ||
| --> |
There was a problem hiding this comment.
Use longer copyright text, as you did in the pom.xml
| # language governing permissions and limitations under the License. | ||
| # | ||
| # When distributing the software, include this License Header Notice in each | ||
| # file and include the License file at packager/legal/LICENSE.txt. |
There was a problem hiding this comment.
Again, use longer copyright
| * @author Mark Wareham | ||
| */ | ||
| @SuppressWarnings("AnnotationAsSuperInterface") | ||
| public class TwoFactorAuthenticationMechanismDefinitionAnnotationLiteral |
There was a problem hiding this comment.
Does this class actually do anything?
| * | ||
| * @author Mark Wareham | ||
| */ | ||
| public class YubicoClientFactory { |
There was a problem hiding this comment.
Could this be done as an application-scoped CDI bean instead?
There was a problem hiding this comment.
Good idea. This way the YubicoClient is per-app
| if (fullKey.isEmpty()){ | ||
| return ""; | ||
| } | ||
| if(fullKey.length()<12){ |
There was a problem hiding this comment.
fullKey.length<12 makes fulKey.isEmpty redundant as the same result would be returned by this one
There was a problem hiding this comment.
I was thinking of StringUtils.isEmpty() which checks whitespace insensitively
|
Also Yubikey needs adding to embedded and web build |
| */ | ||
| @Retention(RetentionPolicy.RUNTIME) | ||
| @Target(ElementType.TYPE) | ||
| public @interface TwoFactorAuthenticationMechanismDefinition { |
There was a problem hiding this comment.
I think this would be better named something more like YubikeyMechanismDefiniton, as a 2FA definition won't necessarily use Yubikey as one of its methods. If you do stick with TwoFactorAuthenticationMechanismDefinition, then using yubikey should be another option inside it.
There was a problem hiding this comment.
The 2FA Mechanism is completely independent of yubikey. You could use the 2FA mech with an LDAPIdentityStore and a DatabaseIdentityStore for example. It selects the identity stores based on what's defined in the app and their priority
There was a problem hiding this comment.
This is still not giving the option of multiple authentication mechanism, which is what 2FA is, instead this is making multiple identity stores used in an AND system rather than the default OR.
There was a problem hiding this comment.
I agree, it does not allow two authentication mechanisms, but two identity stores. I will rename to avoid confusion
There was a problem hiding this comment.
I would like to see more integration with Payara Server. For example I can't see any way that the annotation values set by a developer can be overridden either using the domain.xml, environment properties or using Microprofile config api.
Also some integration with Request Tracing would be useful and perhaps at a stretch the security auditing modules (although that may happen within Soteria)
|
Also are the lambdas strictly necessary as this will push the feature to be Payara 5 only. |
|
@MarkWareham If you hadn't noticed, this now has a conflict :) |
|
@MarkWareham My review comments are still relevant |
|
For this issue the same comment holds as #2704 (comment) |
|
jenkins test please |
|
|
||
| String priorityExpression() default ""; | ||
|
|
||
| String yubikeyAPIClientIDExpression() default ""; |
There was a problem hiding this comment.
A Javadoc comment here would be useful
|
Quick build and test passed! |
MarkWareham
removed
PR: DO NOT MERGE
PAYARA-2575 Yubikey Integration
PAYARA-2575 Yubikey Integration