mneethiraj commented on code in PR #444:
URL: https://github.com/apache/ranger/pull/444#discussion_r1878428792
##########
credentialbuilder/src/main/java/org/apache/ranger/credentialapi/CredentialReader.java:
##########
@@ -17,80 +17,82 @@
* under the License.
*/
- package org.apache.ranger.credentialapi;
-import java.util.ArrayList;
-import java.util.List;
+package org.apache.ranger.credentialapi;
import org.apache.commons.lang3.StringUtils;
import org.apache.hadoop.conf.Configuration;
import org.apache.hadoop.security.alias.CredentialProvider;
import org.apache.hadoop.security.alias.CredentialProviderFactory;
import org.apache.hadoop.security.alias.JavaKeyStoreProvider;
-public class CredentialReader {
+import java.util.ArrayList;
+import java.util.List;
- public static String getDecryptedString(String
CrendentialProviderPath,String alias, String storeType) {
- String credential=null;
- try{
- if(CrendentialProviderPath==null || alias==null){
- return null;
- }
- char[] pass = null;
- Configuration conf = new Configuration();
- String
crendentialProviderPrefixJceks=JavaKeyStoreProvider.SCHEME_NAME + "://file";
- String
crendentialProviderPrefixLocalJceks="localjceks://file";
-
crendentialProviderPrefixJceks=crendentialProviderPrefixJceks.toLowerCase();
+public class CredentialReader {
+ private CredentialReader() {
+ }
Review Comment:
I suggest adding a comment here, to enable quickly finding classes that
can't be instantiated anymore:
```
private CredentialReader() {
// to block instantiation
}
```
##########
credentialbuilder/src/main/java/org/apache/ranger/credentialapi/CredentialReader.java:
##########
@@ -17,80 +17,82 @@
* under the License.
*/
- package org.apache.ranger.credentialapi;
-import java.util.ArrayList;
-import java.util.List;
+package org.apache.ranger.credentialapi;
import org.apache.commons.lang3.StringUtils;
import org.apache.hadoop.conf.Configuration;
import org.apache.hadoop.security.alias.CredentialProvider;
import org.apache.hadoop.security.alias.CredentialProviderFactory;
import org.apache.hadoop.security.alias.JavaKeyStoreProvider;
-public class CredentialReader {
+import java.util.ArrayList;
+import java.util.List;
- public static String getDecryptedString(String
CrendentialProviderPath,String alias, String storeType) {
- String credential=null;
- try{
- if(CrendentialProviderPath==null || alias==null){
- return null;
- }
- char[] pass = null;
- Configuration conf = new Configuration();
- String
crendentialProviderPrefixJceks=JavaKeyStoreProvider.SCHEME_NAME + "://file";
- String
crendentialProviderPrefixLocalJceks="localjceks://file";
-
crendentialProviderPrefixJceks=crendentialProviderPrefixJceks.toLowerCase();
+public class CredentialReader {
+ private CredentialReader() {
+ }
- String crendentialProviderPrefixBcfks = "bcfks" + "://file";
- String crendentialProviderPrefixLocalBcfks= "localbcfks" +
"://file";
-
crendentialProviderPrefixBcfks=crendentialProviderPrefixBcfks.toLowerCase();
-
crendentialProviderPrefixLocalBcfks=crendentialProviderPrefixLocalBcfks.toLowerCase();
+ public static String getDecryptedString(String credentialProviderPath,
String alias, String storeType) {
+ String credential = null;
+ try {
+ if (credentialProviderPath == null || alias == null) {
+ return null;
+ }
+ char[] pass = null;
Review Comment:
@pradeepagrawal8184 - did you reformat the code using codestyle template at
dev-support/RangerCodeScheme-IntelliJ.xml? It is important to maintain
consistency across the code base.
##########
credentialbuilder/src/main/java/org/apache/ranger/credentialapi/CredentialReader.java:
##########
@@ -17,80 +17,82 @@
* under the License.
*/
- package org.apache.ranger.credentialapi;
-import java.util.ArrayList;
-import java.util.List;
+package org.apache.ranger.credentialapi;
import org.apache.commons.lang3.StringUtils;
import org.apache.hadoop.conf.Configuration;
import org.apache.hadoop.security.alias.CredentialProvider;
import org.apache.hadoop.security.alias.CredentialProviderFactory;
import org.apache.hadoop.security.alias.JavaKeyStoreProvider;
-public class CredentialReader {
+import java.util.ArrayList;
+import java.util.List;
- public static String getDecryptedString(String
CrendentialProviderPath,String alias, String storeType) {
- String credential=null;
- try{
- if(CrendentialProviderPath==null || alias==null){
- return null;
- }
- char[] pass = null;
- Configuration conf = new Configuration();
- String
crendentialProviderPrefixJceks=JavaKeyStoreProvider.SCHEME_NAME + "://file";
- String
crendentialProviderPrefixLocalJceks="localjceks://file";
-
crendentialProviderPrefixJceks=crendentialProviderPrefixJceks.toLowerCase();
+public class CredentialReader {
+ private CredentialReader() {
+ }
- String crendentialProviderPrefixBcfks = "bcfks" + "://file";
- String crendentialProviderPrefixLocalBcfks= "localbcfks" +
"://file";
-
crendentialProviderPrefixBcfks=crendentialProviderPrefixBcfks.toLowerCase();
-
crendentialProviderPrefixLocalBcfks=crendentialProviderPrefixLocalBcfks.toLowerCase();
+ public static String getDecryptedString(String credentialProviderPath,
String alias, String storeType) {
+ String credential = null;
+ try {
+ if (credentialProviderPath == null || alias == null) {
+ return null;
+ }
+ char[] pass = null;
+ Configuration conf = new Configuration();
+ String crendentialProviderPrefixJceks =
JavaKeyStoreProvider.SCHEME_NAME + "://file";
+ String crendentialProviderPrefixLocalJceks = "localjceks://file";
+ crendentialProviderPrefixJceks =
crendentialProviderPrefixJceks.toLowerCase();
- CrendentialProviderPath=CrendentialProviderPath.trim();
- alias=alias.trim();
-
if(CrendentialProviderPath.toLowerCase().startsWith(crendentialProviderPrefixJceks)
||
-
CrendentialProviderPath.toLowerCase().startsWith(crendentialProviderPrefixLocalJceks)
||
-
CrendentialProviderPath.toLowerCase().startsWith(crendentialProviderPrefixBcfks)
||
-
CrendentialProviderPath.toLowerCase().startsWith(crendentialProviderPrefixLocalBcfks)){
-
conf.set(CredentialProviderFactory.CREDENTIAL_PROVIDER_PATH,
- //UserProvider.SCHEME_NAME + ":///,"
+
- CrendentialProviderPath);
- }else{
- if(CrendentialProviderPath.startsWith("/")){
- if(StringUtils.equalsIgnoreCase(storeType,
"bcfks")) {
-
conf.set(CredentialProviderFactory.CREDENTIAL_PROVIDER_PATH,
CrendentialProviderPath);
- } else {
-
conf.set(CredentialProviderFactory.CREDENTIAL_PROVIDER_PATH,
-
//UserProvider.SCHEME_NAME + ":///," +
-
JavaKeyStoreProvider.SCHEME_NAME + "://file" + CrendentialProviderPath);
- }
+ String crendentialProviderPrefixBcfks = "bcfks" + "://file";
+ String crendentialProviderPrefixLocalBcfks = "localbcfks" +
"://file";
+ crendentialProviderPrefixBcfks =
crendentialProviderPrefixBcfks.toLowerCase();
Review Comment:
To improve readability, I suggest separating variable declarations,
assignments and rest of the code.
##########
credentialbuilder/src/main/java/org/apache/ranger/credentialapi/CredentialReader.java:
##########
@@ -17,80 +17,82 @@
* under the License.
*/
- package org.apache.ranger.credentialapi;
-import java.util.ArrayList;
-import java.util.List;
+package org.apache.ranger.credentialapi;
import org.apache.commons.lang3.StringUtils;
import org.apache.hadoop.conf.Configuration;
import org.apache.hadoop.security.alias.CredentialProvider;
import org.apache.hadoop.security.alias.CredentialProviderFactory;
import org.apache.hadoop.security.alias.JavaKeyStoreProvider;
-public class CredentialReader {
+import java.util.ArrayList;
+import java.util.List;
- public static String getDecryptedString(String
CrendentialProviderPath,String alias, String storeType) {
- String credential=null;
- try{
- if(CrendentialProviderPath==null || alias==null){
- return null;
- }
- char[] pass = null;
- Configuration conf = new Configuration();
- String
crendentialProviderPrefixJceks=JavaKeyStoreProvider.SCHEME_NAME + "://file";
- String
crendentialProviderPrefixLocalJceks="localjceks://file";
-
crendentialProviderPrefixJceks=crendentialProviderPrefixJceks.toLowerCase();
+public class CredentialReader {
+ private CredentialReader() {
+ }
- String crendentialProviderPrefixBcfks = "bcfks" + "://file";
- String crendentialProviderPrefixLocalBcfks= "localbcfks" +
"://file";
-
crendentialProviderPrefixBcfks=crendentialProviderPrefixBcfks.toLowerCase();
-
crendentialProviderPrefixLocalBcfks=crendentialProviderPrefixLocalBcfks.toLowerCase();
+ public static String getDecryptedString(String credentialProviderPath,
String alias, String storeType) {
+ String credential = null;
+ try {
+ if (credentialProviderPath == null || alias == null) {
+ return null;
+ }
+ char[] pass = null;
+ Configuration conf = new Configuration();
+ String crendentialProviderPrefixJceks =
JavaKeyStoreProvider.SCHEME_NAME + "://file";
+ String crendentialProviderPrefixLocalJceks = "localjceks://file";
+ crendentialProviderPrefixJceks =
crendentialProviderPrefixJceks.toLowerCase();
- CrendentialProviderPath=CrendentialProviderPath.trim();
- alias=alias.trim();
-
if(CrendentialProviderPath.toLowerCase().startsWith(crendentialProviderPrefixJceks)
||
-
CrendentialProviderPath.toLowerCase().startsWith(crendentialProviderPrefixLocalJceks)
||
-
CrendentialProviderPath.toLowerCase().startsWith(crendentialProviderPrefixBcfks)
||
-
CrendentialProviderPath.toLowerCase().startsWith(crendentialProviderPrefixLocalBcfks)){
-
conf.set(CredentialProviderFactory.CREDENTIAL_PROVIDER_PATH,
- //UserProvider.SCHEME_NAME + ":///,"
+
- CrendentialProviderPath);
- }else{
- if(CrendentialProviderPath.startsWith("/")){
- if(StringUtils.equalsIgnoreCase(storeType,
"bcfks")) {
-
conf.set(CredentialProviderFactory.CREDENTIAL_PROVIDER_PATH,
CrendentialProviderPath);
- } else {
-
conf.set(CredentialProviderFactory.CREDENTIAL_PROVIDER_PATH,
-
//UserProvider.SCHEME_NAME + ":///," +
-
JavaKeyStoreProvider.SCHEME_NAME + "://file" + CrendentialProviderPath);
- }
+ String crendentialProviderPrefixBcfks = "bcfks" + "://file";
+ String crendentialProviderPrefixLocalBcfks = "localbcfks" +
"://file";
+ crendentialProviderPrefixBcfks =
crendentialProviderPrefixBcfks.toLowerCase();
+ crendentialProviderPrefixLocalBcfks =
crendentialProviderPrefixLocalBcfks.toLowerCase();
- }else{
-
conf.set(CredentialProviderFactory.CREDENTIAL_PROVIDER_PATH,
- //UserProvider.SCHEME_NAME +
":///," +
-
JavaKeyStoreProvider.SCHEME_NAME + "://file/" + CrendentialProviderPath);
- }
- }
- List<CredentialProvider> providers =
CredentialProviderFactory.getProviders(conf);
- List<String> aliasesList=new ArrayList<String>();
- CredentialProvider.CredentialEntry credEntry=null;
- for(CredentialProvider provider: providers) {
- //System.out.println("Credential Provider :" + provider);
- aliasesList=provider.getAliases();
- if(aliasesList!=null &&
aliasesList.contains(alias.toLowerCase())){
- credEntry=null;
- credEntry=
provider.getCredentialEntry(alias.toLowerCase());
- pass = credEntry.getCredential();
- if(pass!=null && pass.length>0){
- credential=String.valueOf(pass);
- break;
- }
- }
- }
- }catch(Exception ex){
- ex.printStackTrace();
- credential=null;
- }
- return credential;
- }
+ credentialProviderPath = credentialProviderPath.trim();
+ alias = alias.trim();
+ if
(credentialProviderPath.toLowerCase().startsWith(crendentialProviderPrefixJceks)
||
+
credentialProviderPath.toLowerCase().startsWith(crendentialProviderPrefixLocalJceks)
||
+
credentialProviderPath.toLowerCase().startsWith(crendentialProviderPrefixBcfks)
||
+
credentialProviderPath.toLowerCase().startsWith(crendentialProviderPrefixLocalBcfks))
{
+ conf.set(CredentialProviderFactory.CREDENTIAL_PROVIDER_PATH,
Review Comment:
Please fold unnecessary splits into a single line, here and other places;
this is a major code readability issue in several modules.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscr...@ranger.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
