On 31/10/2017 17:07, mandy chung wrote:
On 10/31/17 8:55 AM, Harsha Wardhana B wrote:
Hi Mandy,
Below is the new webrev incorporating below review comments.
http://cr.openjdk.java.net/~hb/5016517/webrev.06/
Looks okay in general except this:
286 // Check if header needs to be inserted
287 if (sbuf.indexOf("# The passwords in this file are hashed") != 0)
{
288 String lastUpdated = "# file last updated on - "
289 + new SimpleDateFormat("MM/dd/yyyy HH:mm:ss").format(new
Date()) + "\n\n";
290 sbuf.insert(0, header + lastUpdated);
291 }
Relying on matching the partial header string is fragile.
Also the timestamp is not updated if the file contains such
heading but the file is re-written again.
You should probably drop the header (auto-inserted), not add
it to sbuf, and always add the header when updating the
password file.
Sorry Mandy, that was my demand. The header is several lines long.
It will look strange if it's added several times to the password file
every time the user/administrator adds/changes a password.
Concerning FileLock - I'm not an expert there, but the Javadoc says:
https://docs.oracle.com/javase/9/docs/api/java/nio/channels/FileLock.html
"File locks are held on behalf of the entire Java virtual machine.
They are not suitable for controlling access to a file by multiple
threads within the same virtual machine."
Which means you need to also protect against concurrent writes to
the same file from within the same JVM by some other means.
Also I wonder if HashedPasswordManager.authenticate should take
a char[] array rather than a String for the password.
best regards,
-- daniel
Mandy
