PAYARA-3793 certificate group mapping #4272
Conversation
|
Jenkins test please |
dmatej
changed the title
|
Jenkins test please |
|
Jenkins test please |
MarkWareham
changed the title
There was a problem hiding this comment.
Looks fine, just these final variable names - pretty sure its a Payara convention for final variables to have names in the format VARIABLE_NAME rather than standard camel case. There are more than the one's I've highlighted - Approved anyway as it's purely cosmetic but it is a convention that I feel should be followed
| private final boolean useCertificate; | ||
| private final SecurityContext securityContext; |
There was a problem hiding this comment.
For the sake of consistency these variables should be refactored to reflect the fact that they're constant (i.e. USE_CERTIFICATE)
There was a problem hiding this comment.
They are not constants! They are only final, not static final. They don't change for the life of the instance, but each instance has it's own field ;)
There was a problem hiding this comment.
I don't think we have a naming rule in sonar for class level private final fields, we do for public final fields and for private final static fields, so technically David is right 😄. Confusingly, we do have a naming rule for local final fields (https://sonarcloud.io/organizations/payara/rules?languages=java&open=squid%3AS4174&q=final&tags=convention)
There was a problem hiding this comment.
Yeah no David's point makes sense I'm happy for it to stay as is ( ͡~ ͜ʖ ͡°)
There was a problem hiding this comment.
@AlanRoth Interesting rule, I'm sure not standard and it may start great confusion, perhaps that's why it is not active in any profile :D
Nice example is here: https://softwareengineering.stackexchange.com/questions/252243/naming-convention-final-fields-not-static
| private final String prname; | ||
| private final String name; | ||
| private final Codec codec; | ||
| private final SecurityContextUtil secContextUtil; | ||
| private final GlassFishORBHelper orbHelper; | ||
| private final SecurityMechanismSelector smSelector; |
There was a problem hiding this comment.
Same as previous comment
| * | ||
| * @author Harish Prabandham | ||
| */ | ||
| public class Group extends PrincipalImpl { | ||
|
|
||
|
|
||
| private static final long serialVersionUID = -3087471149205106412L; |
There was a problem hiding this comment.
Same as above
Hmmm. Same as above -> about upper/camel case?
(There is no freedom in choosing this name, it's specified by Serializable).
There was a problem hiding this comment.
| final LdapName dn = getLdapName(principal); | ||
| _logger.log(Level.FINE, "dn={0}", dn); | ||
| final String principalName = getPrincipalName(dn); | ||
| _logger.log(Level.FINE, "Certificate realm is setting up security context for principal: '{0}'", principalName); |
There was a problem hiding this comment.
Tricky bug ... apostrophes in '{0}' prevent replacement of the value.
There was a problem hiding this comment.
I'm starting to hate JUL pretty much ... :-(
And I know one way how to speed up anything in Payara :D
- automatic, semiautomatic and manual changes
- reduced visibility of realm.init methods to same as in parent
- some comments changed to javadocs
- enhanced loops (automatically)
- removed copypasted javadocs, removed commented out code
- correct generics on some places
- OIDs class transformed to OID enum
- JDBCRealm
- renamed cr to connectorRuntimeDescriptor
- PamRealm
- deprecated PAM.getGroupsOfUser method replaced with it's content:
new UnixUser(username).getGroups()
- BaseRealm
- removed deprecated constant - copy from parent class.
- AbstractStatefulRealm
- addAssignGroups reimplemented
- ClientCertificateLoginModule
- reduced logging complexity
- removed redundant catch block
- LoginException
- added constructor with cause
- CertificateRealm
- OID_MAP moved to OID enum
- using same constant for realm property: COMMON_NAME_AS_PRINCIPAL_NAME
- Util
- removed getDefaultHabitat method - potential cause of NPE if called sooner
than Globals initialized. Replaced with direct usage of Globals.
- realm can be configured to use some parts of DN as group names
|
Jenkins test please |
Description
This is a feature allowing group and role mapping based on the certificate's DN parts.
Important Info
Blockers
#4263 - contains test for the CertificateRealm
Testing
New tests
CertificateRealmITest
Testing Performed
Test suites executed
Testing Environment
Kubuntu 19.04