I have a Java project that copies files and folders to a user's space on the cloud service using a RESTful API. The login design is getting very complicated, and I wanted advice on how to simplify/re-write it.
I wrote this flow to gain an access token from the API and set the destination workspace (directory) through local files if they exist, or dialog boxes prompting the user for these values.
Generally, I set up a User class that dictates login flow and creates dialog boxes, an AuthHandler
that requests and holds the access token and a WorkspaceHandler
that holds the list of available upload workspaces and current upload workspace. These values either come from a local file if it exists, or else from a dialog request to the user.
User.java
/**
* Handles all credential request logic.
*
*/
public class User {
private AuthHandler auth;
private WorkspaceHandler work;
private static User instance = null;
private String developerKey;
private String developerSecret;
private String ciUsername;
private String ciPassword;
private boolean saveOption = false;
private boolean firstRun = true;
private boolean change;
private String workspaceId;
private String workspaceName;
boolean open = false;
public String getName() {
return ciUsername;
}
private User() throws ConfigurationException, SecurityException, IOException, InterruptedException {
auth = AuthHandler.getHandler();
work = WorkspaceHandler.getHandler();
}
public static User getUser() throws ConfigurationException, SecurityException, IOException, InterruptedException {
if(instance == null) {
instance = new User();
}
return instance;
}
public void createUser() throws ConfigurationException, SecurityException, IOException, InterruptedException {
//access token
readCredentials();
while(!auth.login(developerKey, developerSecret, ciUsername, ciPassword)) {
createCredentials();
work.deleteWorkspace();
}
work.resetWorkspaceList();
firstRun = false;
if(saveOption)
saveToProperties();
else
deleteProperties();
//workspace
readWorkspace();
}
private void readWorkspace() throws ConfigurationException, IOException, InterruptedException {
if(!new File("config/workspace.properties").exists()) {
selectWorkspace();
}
else {
PropertiesConfiguration credentialProps = new PropertiesConfiguration("config/workspace.properties");
workspaceId = credentialProps.getString("workspaceId");
workspaceName = credentialProps.getString("workspaceName");
work.setWorkspace(workspaceName, workspaceId);
}
}
public void changeUser() throws SecurityException, ConfigurationException, IOException, InterruptedException {
change = true;
do {
createCredentials();
if(!change)
return;
} while(!auth.login(developerKey, developerSecret, ciUsername, ciPassword));
if(saveOption)
saveToProperties();
else
deleteProperties();
work.deleteWorkspace();
selectWorkspace();
}
private void selectWorkspace() throws ConfigurationException, IOException, InterruptedException {
work.resetWorkspaceList();
work.requestWorkspace();
}
private void deleteProperties() throws IOException {
Files.deleteIfExists(Paths.get("config/credentials.properties"));
}
private void readCredentials() throws ConfigurationException, SecurityException, IOException, InterruptedException {
if(!new File("config/credentials.properties").exists()) {
createCredentials();
}
else {
PropertiesConfiguration credentialProps = new PropertiesConfiguration("config/credentials.properties");
developerKey = credentialProps.getString("key");
developerSecret = credentialProps.getString("secret");
ciUsername = credentialProps.getString("username");
ciPassword = credentialProps.getString("password");
saveOption = true;
}
}
private void createCredentials() throws SecurityException, IOException, ConfigurationException, InterruptedException {
if(open) {
change = false;
return;
}
open = true;
LoginDialog login = new LoginDialog();
if (login.isOk()) {
developerKey = login.getKeyField().getText();
developerSecret = login.getSecretField().getText();
ciUsername = login.getUsernameField().getText();
ciPassword = login.getPasswordField().getText();
saveOption = login.getSave().isSelected();
}
// if you cancel at start, program closes
else if(firstRun) {
System.exit(0);
}
else {
change = false;
}
open = false;
}
private void saveToProperties() throws ConfigurationException, SecurityException, IOException {
// skip if already saved
if(new File("config/credentials.properties").exists()) {
return;
}
PropertiesConfiguration config = new PropertiesConfiguration();
config.setProperty("key", developerKey);
config.setProperty("secret", developerSecret);
config.setProperty("username", ciUsername);
config.setProperty("password", ciPassword);
config.save("config/credentials.properties");
CustomLogger.writeToLog("Credentials saved for next run.");
}
}
AuthHandler.java
/**
* Handles access token creation.
*
*/
public class AuthHandler {
// authentication details
private AccessToken accessToken;
private static AuthHandler instance = null;
// date for access token renewal
private GregorianCalendar renewDate;
// JSON parser
private Gson gson = new Gson();
private String developerKey;
private String developerSecret;
private String ciUsername;
private String ciPassword;
public static AuthHandler getHandler() throws ConfigurationException, SecurityException, IOException, InterruptedException {
if(instance == null) {
instance = new AuthHandler();
}
return instance;
}
/**
* To ensure only one instance of access token.
*/
private AuthHandler() {
}
public boolean login(String key, String secret, String username, String password) throws InterruptedException {
accessToken = createAccessToken(key, secret, username, password);
if(accessToken != null) {
setRenewDate();
return true;
}
return false;
}
/**
* @return Access token for Ci requests.
*/
public String getAccessToken() {
return accessToken.getToken();
}
/**
* If access token needs renewal, requests new access token.
* @throws InterruptedException
*/
public void renewToken() throws InterruptedException {
if(new GregorianCalendar().compareTo(renewDate) > 0) {
System.out.print("Renewing access token: ");
accessToken = createAccessToken(developerKey, developerSecret, ciUsername, ciPassword);
if(accessToken != null)
setRenewDate();
}
}
private void setRenewDate() {
renewDate = new GregorianCalendar();
renewDate.add(Calendar.SECOND, accessToken.getExpiresIn()/2);
}
/**
* Request access token from Ci server.
* @param key
* @param secret
* @param username
* @param password
* @return the access token
* @throws InterruptedException
*/
private AccessToken createAccessToken(String key, String secret, String username, String password) throws InterruptedException {
this.developerKey = key;
this.developerSecret = secret;
this.ciUsername = username;
this.ciPassword = password;
HttpPost request = null;
String reasonPhrase;
try {
String credentials = buildCredentials(username, password);
HttpClient client = buildTokenClient(credentials);
String postUrl = String.format("https://api.cimediacloud.com/oauth2/"
+ "token?grant_type=password_credentials&client_id="
+ "%s&client_secret=%s", key, secret);
request = new HttpPost(postUrl);
// execute request
System.out.print("Requesting access token: ");
HttpResponse response = client.execute(request);
reasonPhrase = response.getStatusLine().getReasonPhrase();
System.out.println(reasonPhrase);
if(!reasonPhrase.equals("OK")) {
JOptionPane.showMessageDialog(new JPanel(), "Invalid credentials, please re-enter.");
System.out.println("TAST" + EntityUtils.toString(response.getEntity()));
}
else {
CustomLogger.writeToLog("Access token granted for 24 hours.");
String responseString = EntityUtils.toString(response.getEntity());
AccessToken token = gson.fromJson(responseString, AccessToken.class);
return token;
}
} catch (IOException e) {
e.printStackTrace();
} catch (SecurityException e) {
e.printStackTrace();
} finally {
request.releaseConnection();
}
return null;
}
private String buildCredentials(String username, String password) {
String userPass = username + ":" + password;
String credentials = new String(
Base64.encodeBase64(userPass.getBytes()));
return credentials;
}
/**
* Returns client with authorization details for access token request.
* @param credentials
* @return
*/
private HttpClient buildTokenClient(String credentials) {
// add authorization header to request
List<Header> headers = new ArrayList<Header>();
Header header = new BasicHeader("Authorization", "Basic " + credentials);
headers.add(header);
return HttpClients.custom().setDefaultHeaders(headers).build();
}
}
WorkspaceHandler.java
/**
* Holds current workspace for uploads and facilitates changing of upload workspace.
*
*/
public class WorkspaceHandler {
private AuthHandler auth;
private Gson gson = new Gson();
//choices
private List<String> workspaceList;
private Workspace[] workspaces;
private Workspace uploadWorkspace;
//settings
private String workspaceId;
private String workspaceName;
private static WorkspaceHandler instance;
public static WorkspaceHandler getHandler() throws ConfigurationException, SecurityException, IOException, InterruptedException {
if(instance == null) {
instance = new WorkspaceHandler();
}
return instance;
}
/**
* Set up current workspace value and gather workspaces available for user.
* @throws ConfigurationException
* @throws SecurityException
* @throws IOException
* @throws InterruptedException
*/
private WorkspaceHandler() throws ConfigurationException, SecurityException, IOException, InterruptedException {
auth = AuthHandler.getHandler();
}
public void resetWorkspaceList() throws IOException, InterruptedException {
// set up choices
uploadWorkspace = null;
workspaceId = workspaceName = null;
workspaces = getWorkspaces().getItems();
workspaceList = workspaceToString(workspaces);
}
public void deleteWorkspace() throws IOException {
Files.deleteIfExists(Paths.get("config/workspace.properties"));
}
public void requestWorkspace() throws ConfigurationException, IOException, InterruptedException {
do {
uploadWorkspace = selectWorkspace(workspaces, workspaceList);
if(uploadWorkspace == null) {
JOptionPane.showMessageDialog(null, "Please select a workspace.");
}
} while(uploadWorkspace == null);
if(!uploadWorkspace.getId().equals(workspaceId)) {
updateWorkspace(uploadWorkspace);
}
}
private void updateWorkspace(Workspace workspace) throws ConfigurationException, SecurityException, IOException {
workspaceName = uploadWorkspace.getName();
workspaceId = uploadWorkspace.getId();
saveToProperties();
CustomLogger.writeToLog(String.format("Workspace set: %s", workspaceName));
SysTray.message("Workspace changed", String.format("The next job will upload to %s.", workspaceName));
}
private void saveToProperties() throws ConfigurationException {
PropertiesConfiguration config = new PropertiesConfiguration();
config.setProperty("workspaceId", workspaceId);
config.setProperty("workspaceName", workspaceName);
config.save("config/workspace.properties");
}
private Workspace selectWorkspace(Workspace[] workspaces, List<String> workspaceList) throws IOException, HeadlessException, ConfigurationException, SecurityException, InterruptedException {
String message = "Hello " + User.getUser().getName() + ", \nSelect your desired upload workspace:";
WorkspaceDialog select = new WorkspaceDialog(message);
JFrame frame = new JFrame();
frame.setAlwaysOnTop(true);
String choice = (String)JOptionPane.showInputDialog(
frame,
"Hello " + User.getUser().getName() + ", \nSelect your desired upload workspace:",
"Upload workspace",
JOptionPane.PLAIN_MESSAGE,
null,
workspaceList.toArray(),
workspaceName);
if(choice == null) {
return uploadWorkspace;
}
return workspaces[workspaceList.indexOf(choice)];
}
private HttpClient buildAuthorizedClient() {
//add authorization token to request
List<Header> headers = new ArrayList<Header>();
Header header = new BasicHeader("Authorization", "Bearer " + auth.getAccessToken());
headers.add(header);
return HttpClients.custom().setDefaultHeaders(headers).build();
}
private WorkspaceList getWorkspaces() throws IOException, InterruptedException {
HttpClient client = buildAuthorizedClient();
//set up get request
String url = "https://api.cimediacloud.com/workspaces";
HttpGet request = new HttpGet(url);
//execute request
HttpResponse response = null;
WorkspaceList workspaceList = null;
try {
response = client.execute(request);
String reasonPhrase = response.getStatusLine().getReasonPhrase();
if(!reasonPhrase.equals("OK")) {
SysTray.message("Error", "Server error occurred. Please try request again.");
Thread.sleep(3000);
System.exit(0);
}
//parse json response for information
HttpEntity entity = response.getEntity();
String responseString = EntityUtils.toString(entity, "UTF-8");
workspaceList = gson.fromJson(responseString, WorkspaceList.class);
return workspaceList;
}
finally {
request.releaseConnection();
}
}
private List<String> workspaceToString(Workspace[] workspaces) {
List<String> workspaceList = new ArrayList<String>();
for(int i = 0; i < workspaces.length; i++) {
String name = workspaces[i].getName();
workspaceList.add(name);
}
return workspaceList;
}
public String getId() {
return workspaceId;
}
public String getName() {
return workspaceName;
}
public void changeWorkspace() {
// TODO Auto-generated method stub
}
public void setWorkspace(String workspaceName, String workspaceId) {
this.workspaceId = workspaceId;
this.workspaceName = workspaceName;
uploadWorkspace = new Workspace(workspaceName, workspaceId);
}
}
Couple questions:
- Is the overall separation of responsibilities sound?
- The login flow is getting extremely polluted with
boolean
values that control flow, which indicates to me that I need to redesign the login process. Any pointers on how I can do this? - If my code is a mess, simply give me advice on how I should structure this to best manage the values involved.
1 Answer 1
Is the overall separation of responsibilities sound?
Nope. Take for example User
, the biggest offender. When I see User
, the first thought that comes to my mind is a dumb object with user id, first name, last name. Your class looks nothing like that. Worse, it's a singleton! (What year is this? Back when Windows 98 was cool? ;-)
Calling it UserManager
would be closer to the truth, but it would still have too many responsibilities. It has a AuthHandler
, and it has a WorkspaceHandler
. You need to rethink this. A User
should be just a container of simple attributes. A UserManager
could have responsibilities like this:
- register a new user
- find an existing user
- check if a user exists
Another clear warning sign here is the presence of fields like this:
private String workspaceId; private String workspaceName;
These look like implementation details of a Workspace
class.
It might make sense for a User
to have an associated Workspace
,
but a User
class should not have to know how a Workspace
works.
This is violating good encapsulation.
If a User
doesn't need to do something with a Workspace
,
then he shouldn't have a Workspace
.
For example if the purpose of the Workspace
is to save files uploaded by this user,
that logic should be outside of the User
class.
You could have an UploadHandler
class for that,
which receives a User
object,
it gets the associated Workspace
from a WorkspaceManager
,
and uploads the file. Something like that.
I suggest to start fresh with a clear mind. Forget about the existing classes and your existing code. Imagine the most basic objects you would need for your purpose. For each and every one of them, think of the single responsibility it has to do. Write them all down, on a blank sheet of paper. If you want to add an additional feature, behavior, check carefully if it falls within the defined single responsibility. If it doesn't then think of another class that could naturally contain that responsibility.
Don't think about implementation. Think at a conceptual level what each class should do, how it should behave, how it can be used together with other classes, without thinking about the implementation. You should not think about files, databases, urls here, you should think at a higher level.
The end result will be your interfaces.
Not classes, interfaces.
You can go ahead and start coding some interfaces.
Simple container objects like a User
can be a concrete class,
but things like WorkspaceManager
, UserManager
, CredentialManager
should all be interfaces, working with other interfaces or simple container objects.
Finally you can move on to creating implementations of the interfaces.
It may seem like a lot of tedious work up front without coding, but it will be worth it. Once your class design is clear on paper in terms of interfaces, the implementation becomes surprisingly straightforward. Give it a try!
The login flow is getting extremely polluted with boolean values that control flow, which indicates to me that I need to redesign the login process. Any pointers on how I can do this?
If you follow the above points faithfully, this might naturally get cleared up.
One good sign of going in the right direction is when you are able to write unit tests for your functionality with dummy authentication classes. Being able to replace the real implementation with a dummy confirms that your authentication is modular enough that you can make changes to it without affecting the rest of the program.
Finally, in the provided code I don't see a good reason to use singletons. After you redesign, I'm sure you will find that you don't actually need singletons, regular classes will be just one. But if you do have to use singletons for something, then use the cleaner, modern enum pattern, for example:
enum UserManager {
INSTANCE;
// ...
User createUser(String username, String password) {
return new User(username, password);
}
}
-
2\$\begingroup\$ I have since removed the User object and switched out the singletons for dependency injection. No more bloated global objects and a much cleaner interface. Huge strides in my design maturity. Thank you! \$\endgroup\$jahmezz– jahmezz2014年09月17日 18:09:46 +00:00Commented Sep 17, 2014 at 18:09
-
\$\begingroup\$ Glad to hear that! Sounds like you're doing great, keep it up! \$\endgroup\$janos– janos2014年09月17日 18:34:30 +00:00Commented Sep 17, 2014 at 18:34