1
\$\begingroup\$

This is a follow-up question for Android APP User class implementation. I am attempting to create a password strength assessment class which is named PasswordStrengthAssessment in Java. Instead of adding "111111", "123123" or the other weak passwords one by one in program, the 10k-most-common.txt on Github is used as the list of common passwords. After constructing PasswordStrengthAssessment class, this 10k-most-common.txt is going to be downloaded and this design may make the matching operation for weak password detection be better. However, the network resource availability and the download time are need to be considered.

The experimental implementation

  • Project name: PasswordStrengthAssessment

  • PasswordStrengthAssessment class implementation:

    package com.example.passwordstrengthassessment;

import android.util.Log;

import androidx.appcompat.app.AppCompatActivity;

import java.io.BufferedReader;
import java.io.IOException;
import java.io.InputStreamReader;
import java.net.MalformedURLException;
import java.net.URL;
import java.util.ArrayList;
import java.util.List;
import java.util.concurrent.TimeUnit;

public class PasswordStrengthAssessment extends AppCompatActivity {

    private List<String> commonPasswords = new ArrayList<String>();
    private final int minimumLengthRequired = 8;

    public enum Level {
        Weak,
        Medium,
        Strong          //  TODO: Implement the determination of strong level password
    }

    //  Empty constructor
    public PasswordStrengthAssessment() {
        try {
            createCommonPasswordsList();
        } catch (InterruptedException ex) {
            ex.printStackTrace();
        }
    }

    //  Reference:
    //  10k-most-common.txt: https://github.com/danielmiessler/SecLists/blob/master/Passwords/Common-Credentials/10k-most-common.txt
    private void createCommonPasswordsList() throws InterruptedException {
        Runnable downloadCommonPasswordsListRunnable = () -> {
            final String mostCommonPasswordList = "https://raw.githubusercontent.com/danielmiessler/SecLists/master/Passwords/Common-Credentials/10k-most-common.txt";
            try {
                // Create a URL for the desired page
                URL url = new URL(mostCommonPasswordList);
                // Read all the text returned by the server
                BufferedReader in = new BufferedReader(new InputStreamReader(url.openStream()));

                String str;
                while ((str = in.readLine()) != null) {
                    //  Reference: https://stackoverflow.com/a/15949198/6667035
                    synchronized(this.commonPasswords) {
                        this.commonPasswords.add(str);
                    }
                    Log.d("downloadCommonPasswordsListRunnable", str);
                }
                in.close();
            } catch (MalformedURLException e) {
                Log.d("createCommonPasswordsList", e.getMessage());
                e.printStackTrace();
            } catch (IOException e) {
                Log.d("createCommonPasswordsList", e.getMessage());
                e.printStackTrace();
            }
        };
        Thread thread = new Thread(downloadCommonPasswordsListRunnable);
        thread.start();

        try {
            TimeUnit.SECONDS.sleep(2);
        } catch (InterruptedException e) {
            e.printStackTrace();
        }

        if (this.commonPasswords.size() == 0)                   //  Common Passwords List download fail
        {
            createCommonPasswordsListLocal();
        }
        return;
    }

    private void createCommonPasswordsListLocal()
    {
        this.commonPasswords.add(getResources().getString(R.string.common_password1));
        this.commonPasswords.add(getResources().getString(R.string.common_password2));
        this.commonPasswords.add(getResources().getString(R.string.common_password3));
        this.commonPasswords.add(getResources().getString(R.string.common_password4));
        this.commonPasswords.add(getResources().getString(R.string.common_password5));
        this.commonPasswords.add(getResources().getString(R.string.common_password6));
        this.commonPasswords.add(getResources().getString(R.string.common_password7));
        this.commonPasswords.add(getResources().getString(R.string.common_password8));
        this.commonPasswords.add(getResources().getString(R.string.common_password9));
        this.commonPasswords.add(getResources().getString(R.string.common_password10));
        this.commonPasswords.add(getResources().getString(R.string.common_password11));
        this.commonPasswords.add(getResources().getString(R.string.common_password12));
        return;
    }

    private boolean lengthCheck(final String input)
    {
        if (input.length() > minimumLengthRequired)
        {
            return true;
        }
        return false;
    }

    public boolean isPasswordWeak(final String passwordInput)
    {
        return determinePasswordStrength(passwordInput).equals(Level.Weak);
    }

    private Level determinePasswordStrength(final String passwordInput)
    {
        if (lengthCheck(passwordInput) == false)
        {
            return Level.Weak;
        }
        if (this.commonPasswords.contains(passwordInput))
        {
            return Level.Weak;
        }
        return Level.Medium;
    }

}

  • strings.xml

    <resources>
    <string name="app_name">PasswordStrengthAssessment</string>
    <string name="common_password1">111111</string>
    <string name="common_password2">123123</string>
    <string name="common_password3">12345</string>
    <string name="common_password4">123456</string>
    <string name="common_password5">12345678</string>
    <string name="common_password6">123456789</string>
    <string name="common_password7">1234567890</string>
    <string name="common_password8">picture1</string>
    <string name="common_password9">password</string>
    <string name="common_password10">password123</string>
    <string name="common_password11">Password</string>
    <string name="common_password12">Password123</string>
</resources>

  • User permission setting

    <uses-permission android:name="android.permission.INTERNET" />
<uses-permission android:name="android.permission.ACCESS_NETWORK_STATE" />
<uses-permission android:name="android.permission.ACCESS_WIFI_STATE" />

  • AndroidManifest.xml

    <?xml version="1.0" encoding="utf-8"?>
<manifest xmlns:android="http://schemas.android.com/apk/res/android"
    package="com.example.passwordstrengthassessment">

    <application
        android:allowBackup="true"
        android:icon="@mipmap/ic_launcher"
        android:label="@string/app_name"
        android:roundIcon="@mipmap/ic_launcher_round"
        android:supportsRtl="true"
        android:theme="@style/Theme.PasswordStrengthAssessment">
        <activity
            android:name=".MainActivity"
            android:exported="true"
            android:usesCleartextTraffic="true">
            <intent-filter>
                <action android:name="android.intent.action.MAIN" />

                <category android:name="android.intent.category.LAUNCHER" />
            </intent-filter>
        </activity>
    </application>


    <uses-permission android:name="android.permission.INTERNET" />
    <uses-permission android:name="android.permission.ACCESS_NETWORK_STATE" />
    <uses-permission android:name="android.permission.ACCESS_WIFI_STATE" />
</manifest>

  • build.gradle

    plugins {
    id 'com.android.application'
}

android {
    compileSdk 30

    defaultConfig {
        applicationId "com.example.passwordstrengthassessment"
        minSdk 26
        targetSdk 30
        versionCode 1
        versionName "1.0"

        testInstrumentationRunner "androidx.test.runner.AndroidJUnitRunner"
    }

    buildTypes {
        release {
            minifyEnabled false
            proguardFiles getDefaultProguardFile('proguard-android-optimize.txt'), 'proguard-rules.pro'
        }
    }
    compileOptions {
        sourceCompatibility JavaVersion.VERSION_11
        targetCompatibility JavaVersion.VERSION_11
    }
}

dependencies {

    implementation 'androidx.appcompat:appcompat:1.3.0'
    implementation fileTree(include: ['*.jar'], dir: 'libs')
    // https://mvnrepository.com/artifact/commons-net/commons-net
    implementation group: 'commons-net', name: 'commons-net', version: '20030805.205232'
    implementation 'com.google.android.material:material:1.3.0'
    implementation 'androidx.constraintlayout:constraintlayout:2.0.4'
    testImplementation 'junit:junit:4.+'
    androidTestImplementation 'androidx.test.ext:junit:1.1.2'
    androidTestImplementation 'androidx.test.espresso:espresso-core:3.3.0'
}

Full Testing Code

  • MainActivity.java implementation:

    package com.example.passwordstrengthassessment;

import androidx.appcompat.app.AppCompatActivity;
import androidx.core.app.ActivityCompat;
import androidx.core.content.ContextCompat;

import android.Manifest;
import android.content.Context;
import android.content.pm.PackageManager;
import android.os.Bundle;
import android.util.Log;
import android.widget.Toast;

import java.util.concurrent.TimeUnit;

public class MainActivity extends AppCompatActivity {

    private static final int READ_EXTERNAL_STORAGE_CODE = 1;
    private static final int WRITE_EXTERNAL_STORAGE_CODE = 2;
    private static final int ACCESS_NETWORK_STATE_CODE = 3;

    @Override
    protected void onCreate(Bundle savedInstanceState) {
        super.onCreate(savedInstanceState);
        setContentView(R.layout.activity_main);

        checkPermission(Manifest.permission.WRITE_EXTERNAL_STORAGE, WRITE_EXTERNAL_STORAGE_CODE);
        checkPermission(Manifest.permission.READ_EXTERNAL_STORAGE, READ_EXTERNAL_STORAGE_CODE);
        checkPermission(Manifest.permission.ACCESS_NETWORK_STATE, ACCESS_NETWORK_STATE_CODE);

        PasswordStrengthAssessment passwordStrengthAssessment = new PasswordStrengthAssessment();
        showToast("Is dogggg a weak password:" + String.valueOf(passwordStrengthAssessment.isPasswordWeak("dogggg")), Toast.LENGTH_SHORT);
    }

    // Function to check and request permission.
    public void checkPermission(String permission, int requestCode)
    {
        if (ContextCompat.checkSelfPermission(MainActivity.this, permission) == PackageManager.PERMISSION_DENIED) {

            // Requesting the permission
            ActivityCompat.requestPermissions(MainActivity.this, new String[] { permission }, requestCode);
        }
        else {
            Toast.makeText(MainActivity.this, "Permission already granted", Toast.LENGTH_SHORT).show();
        }
    }

    void showToast(String Text, int Duration)
    {
        Context context = getApplicationContext();
        CharSequence text = Text;
        int duration = Duration;

        Toast toast = Toast.makeText(context, text, duration);
        toast.show();
    }
}

All suggestions are welcome.

The summary information:

  • Which question it is a follow-up to?

    Android APP User class implementation

  • What changes has been made in the code since last question?

    Instead of adding "111111", "123123" or the other weak passwords one by one in program, the 10k-most-common.txt on Github is used as the list of common passwords here.

  • Why a new review is being asked for?

    If there is any possible improvement, please let me know.

Improve this question
\$\endgroup\$

1 Answer 1

Reset to default
2
\$\begingroup\$

Test Harness

Your question presents a main activity that seems to act as a test harness for a particular case 'dogggg'. If the common password list has been downloaded then 'dogggg' is weak, if it hasn't then it's not. This is OK if you are just trialling something, however it makes review feedback less useful than it could otherwise be. We effectively have no real knowledge about how it is you're planning on using your PasswordStregthAssessment class. The use cases feel like they could frame future code improvements. You should consider actually using and presenting the class in a project to aid ours and your understanding of the requirements. Some of the below probably won't be relevant to your specific use cases...

Is it worth it?

The password file that you're downloading is about 70K (this doesn't seem like it's very big) and it hasn't been updated for 12 months and from what I can tell you don't own the project. If I'd decided that it was a useful list for evaluating passwords, I would consider just taking a cut of the file, either manually, or as part of your build process, so that the file is actually bundled with your application as a resource. If it does change, and you wanted the change, then you'd have the option of redeploying a new version of your app with the updated file. Obviously this adds an overhead to taking updates, particularly if you don't normally need to update your app, however it gives you more control and reduces uncertainty about behaviour.

What's the goal?

This comes back to my first point a bit, as the actual usage of the checker seems relevant. If you don't want to package the password file along with your application, then the next relevant question is how often are you going to check the strength of a password? Is it a one-shot evaluation that happens when the user registers your application, in which case downloading the list on-demand might make sense, or is the purpose of your application to let users try passwords and have them evaluated by your app, in which case many passwords are going to be checked. If this is the case, then it might make more sense to actually download the password file once and save it locally, then use the local version for future validations.

Consistency

You app's behaviour is inconsistent, it'll return different results if you're connected / not connected to the internet. As a user, this might be confusing. If you're using the password to connect to a remote system, does that have the same password validation rules or is that another inconsistency?

Variables

Naming for variables in Java is generally camelCase. I wouldn't expect to see int Duration for example. Adding extra variables isn't always necessary and creates extra noise that the reader has to work through. So, for example:

int duration = Duration;

Where Duration is already an int doesn't really add any value. You don't mutate either duration or Duration. Instead you could just pass Duration directly into the makeText.

So many strings...

Your backup list of passwords has a string resource for each password, which means you need to give each password a name, and add it to your list. A more efficient approach would be to create a single string-array resource which contained all of the passwords. You could then add passwords to the file without needing to change the code. There's a tutorial on how to create and use a string-array resource here.

Out of sync

You're synchronising if you add items to your commonPasswords during the runnable, however you're not synchronising if you perform the add of your backup items, or when you're reading from the list. This doesn't look guaranteed to work to me. Some discussion here.

Something to consider is what happens if it takes 3 seconds to read the passwords from the remote file list (my phone will sometimes take longer than that just to make a connection, particularly if it decides it wants to switch from Wi-Fi to mobile data.

Threads are expensive

You're starting a brand new thread to go and do some work, then sleeping for 2 seconds whilst you wait for the thread to perform the fetch. If the fetch only takes 1 second, you still wait for 2. If it takes longer, then you give up waiting even though the thread keeps running (and may finish when you're no longer expecting it to). If you really want to take this approach, consider killing the thread at the point that you give up on it. But really this seems like a bad use of a thread. Kicking the work to a background thread (preferably from a thread pool) could make sense if you're going to return the main thread to so that it can continue to handle GUI interactions, however it can't do that if it's blocking in a sleep.

Really, it feels like a better approach would be to add a time-out to your http request so that if it's unable to complete within a given period of time you can give up on the request and then continue processing with your alternative. Additional reading here and here.

Improve this answer
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

Not the answer you're looking for? Browse other questions tagged or ask your own question.