Login for desktop application

Question

I am building a login using an API call to a URL that returns JSON data. The login works and is functional but in terms of "correctness" and "professionalism", I am looking to get input from some of the devs here. Please give me an tips that could possibly aide in making the most functional program here possible. (Please note I am using MATISSE to create the GUI)

This section is the Login GUI. Left out Matisse generate text:

public class LogIn extends javax.swing.JFrame {
   ImageIcon imageIcon;
   /**
   * Creates new form LogIn
   */
   public LogIn() 
   {
      imageIcon = new ImageIcon(LogIn.this.getClass().getResource("/Res/loading.gif"));
      initComponents();
   }

   /*
   This actually completes the login and displays the LOADING dialog box. 
   */
    class loginAction extends AbstractAction {
    LoginHandle lh = new LoginHandle();
    String devid = "&t2mdeveloperid=#####-####-####-####-#######";
    String account, username, password;
    String[] loginDataArr;

   @Override
   public void actionPerformed(ActionEvent evt) 
   {
   SwingWorker<Void, Void> mySwingWorker = new SwingWorker<Void, Void>()
   {
      @Override
      protected Void doInBackground() throws Exception 
      {
         account = "&t2maccount=" + txtAccount.getText();
         username = "&t2musername=" + txtUsername.getText();
         password = "&t2mpassword=" + txtPassword.getText();
         loginDataArr = lh.loginData(account, username, password, devid);
         return null;
      }
    };

    Window win = SwingUtilities.getWindowAncestor((AbstractButton)evt.getSource());
    mySwingWorker.addPropertyChangeListener(new PropertyChangeListener() 
    {      
        @Override
        public void propertyChange(PropertyChangeEvent evt) 
        {

            if (evt.getPropertyName().equals("state")) 
            {
                if (evt.getNewValue() == SwingWorker.StateValue.DONE) 
                {
                   loadingDialog.dispose();
                }
            }
        }
    });
    mySwingWorker.execute();
    loadingDialog.setLocationRelativeTo(win);
    loadingDialog.setVisible(true);
  }
}

This class actually does the URL request and parsing of the return data:

 public class LoginHandle 
 {
    public String[] loginData(String account, String username, String password, String devId)
    {
        String[] data = new String[2];
        JsonParser parser = new JsonParser();
        String receipt = "";

        /*
        Creating our trust manager. The program isn't accepting the SSL from the server for some reason. 
        */
        TrustManager[] trustAllCerts = new TrustManager[]
        {
            new X509TrustManager()
            {
                public java.security.cert.X509Certificate[] getAcceptedIssuers()
                {
                    return null;                    
                }
                public void checkClientTrusted(
                  java.security.cert.X509Certificate[] certs, String authType) {
                }
                public void checkServerTrusted(
                  java.security.cert.X509Certificate[] certs, String authType) {
                }
            }
        };

        /*
        Setting trust
        */
        try
        {
            SSLContext sc = SSLContext.getInstance("SSL");
            sc.init(null, trustAllCerts, new java.security.SecureRandom());
            HttpsURLConnection.setDefaultSSLSocketFactory(sc.getSocketFactory());
        }
        catch(Exception e)
        {
            System.err.println("Unable to set secure.");
        }

        /*
        Trying to establish the url connection and getting the data back.
        */
        try
        {
            URL url = new URL("https://m2web.talk2m.com/t2mapi/login?" + devId + account + username + password);
            System.out.println(url);
            URLConnection yc = url.openConnection();
            BufferedReader br = new BufferedReader(new InputStreamReader(yc.getInputStream()));
            String strTemp = "";
            while(null != (strTemp = br.readLine()))
            {
                receipt = strTemp;
            }
        }
        catch(Exception e)
        {
            System.err.println("Error getting data.");
        }

        /*
        Parsing the return JSON
        */
        try
        {
            Object obj = parser.parse(receipt);
            JsonObject jsonObj = (JsonObject) obj;
            JsonElement loginSuccess = jsonObj.get("success");
            JsonElement loginSession = jsonObj.get("t2msession");
            data[0] = loginSuccess.toString();
            data[1] = loginSession.toString();
        }
        catch(Exception e)
        {
            System.err.println("Error parsing json");
            e.printStackTrace();
        }

        return data;
    }

}

Answer 1

1. lh isn't a clear variable name (unless it's used in immediate context only, but it isn't here). I'd rather call it loginHandle.

2. devid misses camel case. I'd name it developerId.

3. String[] loginDataArr;: It's obvious that this is an array. I'd not state it extra.

4. Why is your URLConnection called yc?

5. brreminds me of <br /> every time I look at it. And it refers to a technical aspect. I rather like to see where I get data from or where I put them to. I prefer to call my streams/readers/writers inSomething/outSomething. So it'd be inUrlConnection (or inUC if you like it short) in your case. But that might be personal taste.

6. I'd rename strTemp to line since this is what it is supposed to contain.

Answer 2

URL Structure

The URL structure is hardcoded in the account/username/etc fields, which doesn't seem like a good idea. It is confusing, and it means that LogIn and LoginHandle are responsible for how the actual remote call looks, instead of having it all in one place.

This means that in case the call ever changes, you have to change your code in two completely different classes.

So what I would do is move the &t2mFIELDNAME= strings into the LoginHandle class.

You don't specify if you have control over how the API works, but if you do, change it. Sensitive data should not be submitted via GET, as URLs are frequently logged (client- as well as server-side), while POST data is mostly not logged.

Exception Handling

Your exception handling could be improved. Just catching and printing is rarely the best choice. It's not user friendly, it's not development friendly (the stacktrace is swallowed), and it's not good for reusability.

It also takes away any control the calling class would have if you just threw the exceptions. What if it doesn't want to print? And what if it doesn't want to continue? Eg Unable to set secure seems like a good reason not to continue, except if specifically order to do so (you could add a fallbackToInsecure flag to loginData for this).

Misc

• Your indentation is off, and the position of your curly brackets is not what Java programmers are used to.
• Don't unnecessarily shorten variable names. win can be window, lh can be loginHandle, etc.
• declare variables as late as possible in as small a scope as possible (see eg parser, data, etc.

Answer 3

This code code not properly escape data for use in a URL:

account = "&t2maccount=" + txtAccount.getText();
username = "&t2musername=" + txtUsername.getText();
password = "&t2mpassword=" + txtPassword.getText();

You must escape data used in the context of a URL to remove ambiguity. What if someone had an ampersand in their password? With this method, the remainder of the password would be cut off.

In any case, URL building does not belong here. You should move the building of the URL into the loginData function where you build the rest of the URL. I'm no Java programmer, but I assume there is a simple method for taking a hash and converting it into a query string. Use it.