Paul Clapham wrote:Unfortunately you've posted several hundred lines of code, so it's quite likely that I may have missed something while scrolling up and down. However... you create a new Session object in each of the two servlets. So if your first servlet put something into the Session object, the second servlet isn't going to see that because it creates another Session object.
Paul Clapham wrote:Third, okay three problems then, you've written a Session class which looks like a wrapper for the session.
Paul Clapham wrote:Well, I have two problems dealing with this question.
First, not enough information. You have two servlets and I can't tell which is run first and which is run in response to the JSP written by the first one.
Second, too much information. There's way too much code to go through.
Third, okay three problems then, you've written a Session class which looks like a wrapper for the session. It may or may not work properly. It would help to test it with a much less complicated pair of servlets.
Bear Bibeault wrote:
Paul Clapham wrote:Third, okay three problems then, you've written a Session class which looks like a wrapper for the session.
A good point. I've written dozens of web apps and never felt a need to do this. You might perhaps think about whether you're over-engineering things (which frequently leads to just adding more points of possible failure).
Michael Portman wrote:Step 1: User visits index page/servlet.
Step 2. User enters his login details.
Step 3. The login form submits to the formController servlet decides which form we're dealing with, lets take login for an example.
Step 4. The formController creates a Login object to to process the users details. Then invokes the login(Map<sring, string> form data) method.
Step 5. If everything checks out the Loging bean will set the user's details as session vars using my Session API/Wrapper and returns true.
Step 7. formController redirects the user back to the index page/servlet logged in.
Michael Portman wrote:Step 1: User visits index page/servlet.
Step 2. User enters his login details.
Dave Tolls wrote:Not gone through the code (there's too much for a quick look first thing on a Friday) but...you have state in a servlet.
Specifically your formController (which should, by Java naming standards, be FormController) store the request data as a HashMap as an attribute. That is seriously prone to going wrong in a multi-threaded environment like a webapp. If you have two people hitting the servlet at the same time, the second one could easily overwrite the formData object before the first one gets to the Login class constructor, resulting in the first user ending up logged in as the second user. This would often be classed as a Bad Thing.
Michael Portman wrote:
So how do I do fix this, correct my code if you can please? Be easy on me as im relatively new to Java, im migrating from PHP.
Dave Tolls wrote:OK, looking at that Login class.
The use of that MySQL class seems to imply you are not using a connection pool, which is generally not a good idea with a webapp. Depending on how many concurrent users you are likely to have you may find yourself running out of connections.
I'm going to treat the Login class like a DAO (Data Access Object), since that seems to be what it is.
So, when interacting with the database using JDBC in a webapp you'll generally see the following structure in a method:
You need those try-with-resources blocks to ensure the resources (the result set, statement and connection) are closed, otherwise you are going to run out.
The getConnection() call simply hides whatever mechanism you are using to get a connection...as I say above, it should be using a connection pool.
So that's the basic structure you will have in your db interaction method(s).
For the Login, it's a single query:
If that returns a row then the user is valid, otherwise it is not.
So, I would expect a single method on your Login class, called getUser(String name, String password), which returns a User containing whatever data you need.
Hope that's not too much of an overload!
Dave Tolls wrote:No.
You user the query I gave you above.
You only want to find a user who matches both the username and password given.
So the query needs to have a WHERE clause with that AND in it.
That's how all login methods work.
Michael Portman wrote:Can everybody agree this is the correct way of doing things? Cheers.
Paul Clapham wrote:
Michael Portman wrote:Can everybody agree this is the correct way of doing things? Cheers.
Not for a real application that's going into production for real users, no. If you're just trying to learn Java web apps then I guess it's okay, but for a real application you should use a PreparedStatement to access the database; building SQL using string concatenations allows SQL injection attacks to corrupt or damage the database. At least you're not storing plain-text passwords in the database, gotta give you credit for that.
Here's another quibble:
When you see Java code like "if (booleanValue) variable = true; else variable = false;" you can replace that code by "variable = booleanValue".
Michael Portman wrote:I'm little confused over these preparedStatements are talking about. Couldn't I just escape all of my string which go into DB with replaceAll()/replace(), or maybe even use Regex?
Paul Clapham wrote:
Michael Portman wrote:I'm little confused over these preparedStatements are talking about. Couldn't I just escape all of my string which go into DB with replaceAll()/replace(), or maybe even use Regex?
Here's the tutorial: Using Prepared Statements
As for doing something yourself to avoid SQL injection attacks: if you were a better programmer and experienced in the art of SQL injection attacks, that might be a possibility. But why go to all that work when you can just use a PreparedStatement?
(And as for escaping things, that's one of the tasks of PreparedStatement. You haven't written the code to escape quotes in the user name, anyway. But that code would be dependent on what database you were using, which is another reason to let PreparedStatement do it for you.)
Michael Portman wrote:Preventing a SQL attack is the least of my worries while my site isnt live yet. Getting the user login system working is my main priority atm.
Paul Clapham wrote:
Michael Portman wrote:Preventing a SQL attack is the least of my worries while my site isnt live yet. Getting the user login system working is my main priority atm.
"I know this is wrong, but I'll fix it later."
Problem is, there's always more important things to do later -- until the security breach happens. Really, just use a PreparedStatement. It's not like it's any harder than using a Statement, in fact it's easier because you don't have to mess with getting the single-quotes and double-quotes right.
Michael Portman wrote:As you can see I wouldnt know what queries went getting passed into the mysql.class if you know what I mean?