Skip to content

Commit

Permalink
Fix checkstyle (#3283)
Browse files Browse the repository at this point in the history
### Description
Fix checkstyle. Now it works as expected.

The problem is that chectyle works only with one config file. It does
not support multiple files except per-defined files like
`suppressions.xml`.
So the previous config effectively replaced `sun_checks.xml` and
validated only `System.out.println` lines in `tools`.
Since `println_checks.xml` replaced the main file we did not notice that
new version of checktyle removed `PrintlnModule` from the code base.

Changes:
- All code related to checking `tools` folder was moved in the main file
- Renamed the `sun_...xml` file to the `checktyle.xml` which is default
settings for checkstyle so we can track changes in it
- Set the latest version for `checktyle` so it can validate new JDK
features as well
 - Fixed problematic files which checkstyle highlighted     

### Issues Resolved
#3260

### Testing
`System.out.println` I tested manually all I can say it works :-)  

### Check List
- [ ] New functionality includes testing
- [ ] New functionality has been documented
- [ ] Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made
under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and
signing off your commits, please check
[here](https://github.com/opensearch-project/OpenSearch/blob/main/CONTRIBUTING.md#developer-certificate-of-origin).

Signed-off-by: Andrey Pleskach <[email protected]>
  • Loading branch information
willyborankin authored Sep 1, 2023
1 parent 53f64b9 commit cd45e78
Show file tree
Hide file tree
Showing 6 changed files with 33 additions and 32 deletions.
4 changes: 2 additions & 2 deletions build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -322,8 +322,8 @@ jacocoTestReport {
}

checkstyle {
configFile file("checkstyle/sun_checks.xml")
configFile file("checkstyle/println_checks.xml")
toolVersion "10.3.3"
configDirectory.set(rootProject.file("checkstyle/"))
}

opensearchplugin {
Expand Down
18 changes: 16 additions & 2 deletions checkstyle/sun_checks.xml → checkstyle/checkstyle.xml
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@
To suppress certain violations please review suppression filters.
Finally, it is worth reading the documentation.
-->

<!-- exSUN codestyle check with the additional check for System.out.ptintln -->
<module name="Checker">
<!--
If you set the basedir property below, then all reported file
Expand All @@ -36,6 +36,13 @@
<module name="BeforeExecutionExclusionFileFilter">
<property name="fileNamePattern" value="module\-info\.java$"/>
</module>
<!-- System.out.ptintln -->
<module name="BeforeExecutionExclusionFileFilter">
<property name="fileNamePattern" value="src/main/java/org/opensearch/security/tools/*"/>
</module>
<module name="BeforeExecutionExclusionFileFilter">
<property name="fileNamePattern" value="src/main/java/com/amazon/dlic/auth/http/kerberos/HTTPSpnegoAuthenticator.java"/>
</module>

<!-- https://checkstyle.org/config_filters.html#SuppressionFilter -->
<module name="SuppressionFilter">
Expand Down Expand Up @@ -188,6 +195,14 @@
<property name="optional" value="true"/>
</module>

<!-- System.out.println -->
<module name="RegexpSinglelineJava">
<property name="format" value="System.out.println"/>
<property name="ignoreCase" value="true"/>
<property name="message" value="Do not use System.out.println" />
<property name="severity" value="error"/>
</module>

</module>

<module name="RegexpSingleline">
Expand Down Expand Up @@ -222,5 +237,4 @@
<property name="checkFormat" value="$1"/>
</module>

<module name="PrintlnModule"/>
</module>
21 changes: 0 additions & 21 deletions checkstyle/println_checks.xml

This file was deleted.

7 changes: 6 additions & 1 deletion src/main/java/org/opensearch/security/util/KeyUtils.java
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,12 @@
import org.opensearch.SpecialPermission;
import org.opensearch.core.common.Strings;

import java.security.*;
import java.security.AccessController;
import java.security.Key;
import java.security.KeyFactory;
import java.security.NoSuchAlgorithmException;
import java.security.PrivilegedAction;
import java.security.PublicKey;
import java.security.spec.InvalidKeySpecException;
import java.security.spec.X509EncodedKeySpec;
import java.util.Base64;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,10 +11,6 @@

package org.opensearch.security.authtoken.jwt;

import java.util.List;
import java.util.Optional;
import java.util.function.LongSupplier;

import org.apache.commons.lang3.RandomStringUtils;
import org.apache.cxf.rs.security.jose.jwk.JsonWebKey;
import org.apache.cxf.rs.security.jose.jws.JwsJwtCompactConsumer;
Expand All @@ -24,6 +20,10 @@
import org.opensearch.common.settings.Settings;
import org.opensearch.security.support.ConfigConstants;

import java.util.List;
import java.util.Optional;
import java.util.function.LongSupplier;

public class JwtVendorTest {

@Test
Expand Down Expand Up @@ -99,7 +99,9 @@ public void testCreateJwtWithRoleSecurityMode() throws Exception {
Settings settings = Settings.builder()
.put("signing_key", "abc123")
.put("encryption_key", claimsEncryptionKey)
.put(ConfigConstants.EXTENSIONS_BWC_PLUGIN_MODE, true)
// CS-SUPPRESS-SINGLE: RegexpSingleline get Extensions Settings
.put(ConfigConstants.EXTENSIONS_BWC_PLUGIN_MODE, "true")
// CS-ENFORCE-SINGLE
.build();
Long expectedExp = currentTime.getAsLong() + expirySeconds;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,8 @@
import static org.hamcrest.Matchers.equalTo;
import static org.junit.Assert.assertFalse;
import static org.junit.Assert.assertTrue;
import static org.opensearch.security.privileges.PrivilegesEvaluator.*;
import static org.opensearch.security.privileges.PrivilegesEvaluator.DNFOF_MATCHER;
import static org.opensearch.security.privileges.PrivilegesEvaluator.isClusterPerm;

public class PrivilegesEvaluatorUnitTest {

Expand Down

0 comments on commit cd45e78

Please sign in to comment.