This Bugzilla instance is a read-only archive of historic NetBeans bug reports. To report a bug in NetBeans please follow the project's instructions for reporting issues.

Bug 87600 - Store connection passwords between sessions
Summary: Store connection passwords between sessions
Status: RESOLVED FIXED
Alias: None
Product: db
Classification: Unclassified
Component: Code (show other bugs)
Version: 5.x
Hardware: PC All
: P2 blocker (vote)
Assignee: David Vancouvering
URL: http://wiki.netbeans.org/wiki/view/Sa...
Keywords: API, API_REVIEW_FAST, UI
: 112551 (view as bug list)
Depends on:
Blocks: 99730 117193
  Show dependency tree
 
Reported: 2006-10-20 08:10 UTC by wvreeven
Modified: 2007-10-16 08:21 UTC (History)
6 users (show)

See Also:
Issue Type: ENHANCEMENT
Exception Reporter:


Attachments
Changes to the arch.xml and apichanges.xml files (2.92 KB, text/plain)
2007-10-06 00:50 UTC, David Vancouvering
Details
diff of changes to make for using distinct help ids (2.55 KB, text/plain)
2007-10-12 17:40 UTC, John Baker
Details

Note You need to log in before you can comment on or make changes to this bug.
Description wvreeven 2006-10-20 08:10:20 UTC
Hi,


I use the db explorer in NB a lot. It would be very nice to have NB remember the
db password not just for the current session, but also in between sessions. That
way NB users won't have to enter the db password again after starting up NB.
Perhaps this feature could be configurable on a per database basis.


Thanks in advance,

Wouter van Reeven
Comment 1 Andrei Badea 2006-10-20 10:12:20 UTC
This is a bit controversial, since it is insecure: it requires saving an
unencrypted password on the disk. But perhaps it could be implemented, provided
a big warning message is displayed.
Comment 2 wvreeven 2006-10-20 10:18:34 UTC
For my work I use Oracle's JDeveloper a lot. That saves the db password in an
encrypted form on the disk. Perhaps this could be an option for NetBeans as well?


Thanks for considering this.


Wouter
Comment 3 wvreeven 2006-10-20 10:33:29 UTC
See issue 84654
Comment 4 Andrei Badea 2006-10-20 13:11:36 UTC
Why did you close this as WONTFIX?

Re. JDeveloper: are you sure the password is really encrypted? Isn't is just
scrambled, and as such easy to recover? If it is encrypted, do you know how? Did
you provide an encryption key, or does JDeveloper just somehow encrypt it?
Comment 5 wvreeven 2006-10-20 13:16:51 UTC
Hold on, I am sorry, but what I said is wrong. The password isn't encrypted at
all. It is stored in plain text.
I share your concern about the security issue involved here. But still it
appeals to me. I like your idea about the security warning. Would you please
consider that?


Greets, Wouter
Comment 6 Andrei Badea 2006-10-20 13:18:48 UTC
Yes. Reopening then.
Comment 7 Andrei Badea 2006-11-28 17:18:56 UTC
It would be good to at least parts of issue 68991 and the changes to the Connect
and New DB Connection dialogs specified in

http://j2ee.netbeans.org/docs/promog/persistence-ui-spec.html#Step_1_New_Database_Connection_Wizard

together with this.
Comment 8 giorgio42 2007-06-27 20:52:21 UTC
I once started to write a data-oriented NB module that should connect to a database in the background without
any user intervention. With the current dbexplorer API this is not possible, because the database password is
not stored in the connection description and the implementation has a bug that makes the dialog pop up, even if all
parameters to connect to the database have been specified, including the password.

My opinion on the security concern: persistence.xml contains database passwords, too. Nobody ever complained.

Cheers,
Georg
Comment 9 Andrei Badea 2007-06-28 10:37:27 UTC
Right, I have since come to agree that storing passwords in plain text is not a big problem. I still plan to scramble
them to prevent accidental reveal, but I don't want to pretend that scrambling offers any security.
Comment 10 Andrei Badea 2007-06-28 10:38:10 UTC
Re. bug mentioned in desc9: it you have reproducible steps please file it.
Comment 11 giorgio42 2007-06-28 15:26:15 UTC
Andrei,

the bug mentioned is described under item 3 in this detailed post to openide-dev from September 2006:
http://www.nabble.com/Experimenting-with-the-Database-Explorer-API-tf2280369.html#a6334571

Issue http://www.netbeans.org/issues/show_bug.cgi?id=108422 has just been created.

I do not know to which degree the db explorer API has changed since then, but if some of the issues mentioned in this
post have been fixed or ceased to exist, it would be really great.

(OBTW: The request to store the password in the connection description is under item 5).

Best regards,
Georg

Comment 12 David Vancouvering 2007-06-28 17:19:44 UTC
I just wanted to make sure we're all clear that this is a piece of the puzzle around providing the ability for users to
opt-in to having a DB connection automatically opened on NB startup, and not the whole solution?

I have made issue 99730 dependent on this change, does that make sense?
Comment 13 _ jimdavidson 2007-06-28 18:43:52 UTC
FYI, Java Studio Creator did store the password on the disk in encrypted form (using javax.crypto.Cipher).  With the
integration to NB6.0 and the move to open source, we've moved away from that.
Comment 14 John Baker 2007-08-10 16:48:40 UTC
*** Issue 112551 has been marked as a duplicate of this issue. ***
Comment 15 John Baker 2007-09-13 23:01:08 UTC
Will this be fixed for 6.0?
Or should this be reassigned?
Comment 16 David Vancouvering 2007-09-14 01:17:38 UTC
I would *love* to fix this.  But since we're in beta, and this is marked as an enhancement, I think it can't go into
6.0.  Does anyone believe otherwise?
Comment 17 John Baker 2007-09-14 02:12:13 UTC
I disagree this is an enhancement, from a user's point of view.
There is a related/dependent issue that is marked as a defect
http://www.netbeans.org/issues/show_bug.cgi?id=108422
I suppose 108422 is blocked by this issue, I'll let the owner decide
Comment 18 wvreeven 2007-09-14 08:09:49 UTC
Considering the comments in
http://www.netbeans.org/issues/show_bug.cgi?id=108422
I consider this issue as a bug. Would you please try and get this fixed for 6.0? Thanks a lot!
Comment 19 Andrei Badea 2007-09-14 10:46:19 UTC
This issue has two aspects:

1. Storing the password so that the user doesn't have to enter it each time when making a connection.
2. Not displaying the Connect dialog when making a connection and the password is known.

#1 does not necessarily imply #2. We might choose to do #1, but still always display the Connect dialog (with the
passwoed pre-filled) to allow the user to enter another user name, password or schema. Or we might alternatively do #1
and #2 and have a nice Properties dialog on the connection to allow the user to change the schema, etc. We don't have
that dialog now, nor do we have the resources to implement it for 6.0. So if we do anything, it should only be #1.

Re. 108422: that is an enhancement, not a defect. There was never a plan to support connecting to the database without
displaying an UI.

John, do you (or anyone else) have cycles to work on this?
Comment 20 David Vancouvering 2007-09-14 17:57:40 UTC
A couple of comments:

- I think it would be a nice EOU enhancement to let users choose to auto-connect to a connection so they don't have to
manually connect each time. I have received this complaint during my interviews as well.  So I know it's never been
something we've planned to do, but maybe we should look at that - *post* 6.0, I agree this is an enhancement, not a defect.

- As a small step towards that, storing the password so it's pre-filled in across runs of NB is not a big UI change
(we'd have to either remove the prompt to store the password for the session, or add a new prompt to also store it
permanently).  I'm not sure if it's too late to put in such a small UI change.

- I would also like to know if you have time to work on this, John.  Let's make sure we get someone on it.

David
Comment 21 John Baker 2007-09-14 18:14:05 UTC
I must have heard wrong at one of the meetings, that Andrei would resolve this issue 87600.
And also it was stated that 108422 required 87600 to be fixed first.  Now in desc20, this has been
 re-clarified, thank you.  It doesn't appear that the 2 aspects are interdependent and for my expectation
(below) I don't think this issue 87600 needs to be fixed first.

We don't need to create another issue: to connect without posting the connection dialog or at least 
filling in the password is tracked by 108422.

My expectation is when the password is provided to DatabaseConnection object and this object is used
to show the Connection dialog then at minimum I expect the password to be filled in the dialog.

I agree this issue 87600 is an enhancement.

David, the small step should be doable - it doesn't sound like much of a change to pre-fill in the password 
of the Connection dialog

Comment 22 John Baker 2007-09-14 19:03:45 UTC
The Connection dialog relies on parameters from the Connection node, not the parameters
passed to the DatabaseConnection.  DatabaseConnection knows the password but not the Connection node.
I presume because a connection to the connection node hasn't been established.

Once connected to an existing connection (node) then the Connection dialog will know the password and instead
of displaying the the dialog with the username/password, a progress dialog is displayed.

I guess information is retrieved from the connection node to be sure the DatabaseConnection object
pertains to an existing connection (any parameters could be passed even if a connection (node) doesn't exists)

Seems like extra work.  Instead of obtaining connection information from the connection node, instead
be sure a connection node matches up with the DatabaseConnection information.  Unless there's another
usage of DatabaseConnection that I don't know about.
Comment 23 John Baker 2007-09-14 19:41:08 UTC
A suggested fix (hack)

1. In ConnectAction.ConnectionDialogDisplayer.showDialog
an instance of DatabaseConnection is being retrieved already (creating a dependency on DatabaseConnection).

If the pwd from the Connection node is null, check if DatabaseConnection's pwd is also null and if it's not the
use DatabaseConnection pwd  (still the parameters of the Connection node will be used so there's no chance
of a connection to a non-existant connection)

sample code:


if (pwd == null) {
    if (dbcon.getPassword() != null) {
        pwd = dbcon.getPassword();
    }
}

To be sure hack is not too limited - need to make sure all relevant use cases work
Comment 24 John Baker 2007-09-14 19:45:54 UTC
desc24 doesn't work, dbcon.getPassword() is still null
Comment 25 _ jimdavidson 2007-09-14 20:45:41 UTC
Andrei's comment in desc20 lays things out pretty clearly.  There are two separate issues here.

This issue (#87600) is a pre-cursor to any of the other things we want to do, like not bringing up a dialog (#108422). 
So, we have to start here.

This change would entail at least some UI impact, albeit not large; maybe just change the wording in the New Database
Connection dialog.

In terms of functionality, we would probably have to provide some encryption.  Note that the secret key wouldn't be so
secret for an open-source product.  I think users would accept that it's not ultra-secure; web browsers do this sort of
thing all the time.

The reason we haven't done this has been largely a question of priority.  We've been working on other things.  There are
(only) two votes for this enhancement, although David has reported that he got comments on this in the interviews.

If we think it's a big usability issue, I think we could make a case for taking it on.  Is there a consensus that this
is worth pushing for?
Comment 26 David Vancouvering 2007-09-14 22:21:25 UTC
John Baker says:
Andrei's comment in desc20 lays things out pretty clearly.  There are two separate issues here.

This issue (#87600) is a pre-cursor to any of the other things we want to do, like not bringing up a dialog (#108422). 
So, we have to start here.

This change would entail at least some UI impact, albeit not large; maybe just change the wording in the New Database
Connection dialog.

David: agreed

John Baker says:
In terms of functionality, we would probably have to provide some encryption.  Note that the secret key wouldn't be so
secret for an open-source product.  I think users would accept that it's not ultra-secure; web browsers do this sort of
thing all the time.

David: I don't know about encryption.  That's generally not a requirement by ARC.  My memory is that, as long as the
password file is created with appropriate permissions (e.g. rw----), then that is satisfactory.  It might be worth
investigating what is really required by ARC here.

If we do do encryption, I am not sure what you mean by "the secret key is not so secret since we're in open source."  I
haven't done this encryption before, but the UNIX passwd file is encrypted, and Solaris is open source.  Maybe you could
go and see how they do it.

John Baker says:
The reason we haven't done this has been largely a question of priority.  We've been working on other things.  There are
(only) two votes for this enhancement, although David has reported that he got comments on this in the interviews.
 
If we think it's a big usability issue, I think we could make a case for taking it on.  Is there a consensus that this
is worth pushing for?

David says: Well, I think if it's not a huge amount of work, it's the kind of "thoughtfulness" that makes the product
better to work with.  Many things we can't do right now because of UI freeze, but this is something that I think could
have a good positive impact.  IMHO.  Note that most issues don't have *any* votes for them, so two votes is pretty
significant.  IMHO. :)
Comment 27 John Baker 2007-09-14 22:45:15 UTC
David, in the last comment desc27, you were quoting Jim not me :-)
Comment 28 John Baker 2007-09-14 22:56:00 UTC
After looking at the code, I think this is more of a design issue than limitation.

It doesn't make sense that a new DatabaseConnection object can be used to show the Connection dialog since
in the showDialog method there is a node.getDatabaseConnection()

In other words, there's a responsibility issue here.
The Connection dialog should know who invoked it.
Currently, showDialog gets the connection object from the node, which is a hack to me.

The current ability to show the dialog is flawed since the Connection dialog has a tight integration with the node, so 
it doesn't make sense for any DatabaseConnection object to invoke the Connection dialog

Instead the dialog could be invoked something like node.getDatabaseConnection().showDialog
or by a new DatabaseConnection object.  Then the showDialog method should know which DatabaseConnection to use.

I think the encryption is handled by the database.  I don't think encryption should be implemented in the dialog.
Comment 29 David Vancouvering 2007-09-15 00:58:28 UTC
Sorry, you're right, I was responding to Jim's comments, not John Baker's...

John Baker says:
After looking at the code, I think this is more of a design issue than limitation.

It doesn't make sense that a new DatabaseConnection object can be used to show the Connection dialog since
in the showDialog method there is a node.getDatabaseConnection()

In other words, there's a responsibility issue here.
The Connection dialog should know who invoked it.
Currently, showDialog gets the connection object from the node, which is a hack to me.

The current ability to show the dialog is flawed since the Connection dialog has a tight integration with the node, so 
it doesn't make sense for any DatabaseConnection object to invoke the Connection dialog

Instead the dialog could be invoked something like node.getDatabaseConnection().showDialog
or by a new DatabaseConnection object.  Then the showDialog method should know which DatabaseConnection to use.

David:
If I follow things correctly, I think what you're saying is:
- DatabaseConnection may or may not have an active connection
- If it doesn't, then the right approach is to bring up the Connection dialog to establish a new "live" connection to
the database
- Rather than associating the dialog with the node, and then having the node figure out what DatabaseConnection this is
for, the dialog should be associated with a DatabaseConnection, and keep the node out of the picture, e.g.

         DatabaseConnection
                 |
      -----------------------
      |                     |
    ConnectionDialog     JDBCConnection

Right?  

Are there any important reasons I'm missing why the dialog needs to be associated with the connection node rather than
the DatabaseConnection?


John says:
I think the encryption is handled by the database.  I don't think encryption should be implemented in the dialog.

David: Um, I'm not sure what you mean.  I was thinking we were talking about encryption of the passwords in the password
file we use so we can remember passwords...  Are you proposing to store these passwords in a database?  
Comment 30 John Baker 2007-09-15 01:18:34 UTC
I wasn't sure whether to make comments in the issue or by email.  To track the discussion, I opted for 
entering comments in the issue.  

> David:
> If I follow things correctly, I think what you're saying is:
> - DatabaseConnection may or may not have an active connection
> - If it doesn't, then the right approach is to bring up the Connection dialog to establish a new "live" connection to
> the database
> - Rather than associating the dialog with the node, and then having the node figure out what DatabaseConnection this is
> for, the dialog should be associated with a DatabaseConnection, and keep the node out of the picture, e.g.

>          DatabaseConnection
>                  |
>       -----------------------
>       |                     |
>     ConnectionDialog     JDBCConnection
> 

Yes, something like this.  It doesn't make sense to me that I can create a DatabaseConnection object
then later a different DatabaseConnection object from the node is used to make the connection.
Yes, I think the Connection dialog should know which DatabaseConnection requests the connection.

> 
> David: Um, I'm not sure what you mean.  I was thinking we were talking about encryption of the passwords in the password
> file we use so we can remember passwords...  Are you proposing to store these passwords in a database?  

No not in a database.  We seem to be going in different directions.  I didn't realize Jim was referring
to using encryption to store the password between sessions. I don't see a need to store passwords
between sessions but instead safely change or extend the existing API 


Comment 31 Andrei Badea 2007-09-17 11:19:54 UTC
I don't really understand the discussion about DatabaseConnection and nodes. It is true that the dependency on nodes is
wrong and that the dialog should only work with DatabaseConnection's. But does that need to be fixed at this moment?
Currently the dialog extracts the database URL and the user name from the database connection that the user is
connecting. Why can't it just to the same with the password?

Re. encryption: there is no point in doing it, it can never be secure (given the constraints). Unix can be secure
because it doesn't store the password, it stores just a hash (e.g., MD5) of it. When the user logs in, the hash of the
entered password is compared to the stored hash. The password is never retrieved from the stored hash (that's not
possible, because the hash is one-way). But we need to retrieve the password in order to send it to the database, so we
can't use a hash.

What might make sense is scrambling the password in order to prevent accidental reveal. Not sure this is needed, and it
definitely complicates things.

The password needs to be stored in the XML connection file in the userdir. (Note: this file, currently in version 1.0,
needs to be upgraded to version 1.1.) So we would add a password element to the DTD containing the scrambled value:

<password value="5cramb13d"/>

However, it is possible for a module writer to register connections in his module by putting such XML files in the
module layer. So the scrambling algorithm needs to be documented as an API. If we do the scrambling, I propose something
simple, such as base64 of the original password xor-ed with a constant like 42.

I don't understand the last sentence in desc31. This issue *is* about storing the password between sessions.
Comment 32 _ jimdavidson 2007-09-17 22:00:31 UTC
There have been some questions about encryption of passwords, so let me elaborate.

I brought this up because it's how Creator did things.  We encrypted the password using a secret key that was baked into
the Creator source code.  The encrypted password was stored as part of the DataSource (in context.xml in the userdir),
which could be exported from one machine to another.  It's actually a pretty secure method.

It may not be necessary to do anything like this.  That's fine.  I only bring it up as an option, and to point out that
the Creator method will no longer work, because our source code is now open.

One other point: like Andrei, I'm a little puzzled about the sentence, "I don't see a need to store passwords between
sessions but instead safely change or extend the existing API".  I'm not sure what that is getting at.
Comment 33 John Baker 2007-09-18 06:26:56 UTC
I see this issue is about saving passwords between sessions.  My comments earlier concern the design of the API.

It's been suggested that saving the password would solve other issues such as 108422, which may be true.
It sounds like this won't get solved in 6.0

I was hoping to resolve 108422 without dealing with saving the password.

Comment 34 John Baker 2007-09-18 06:48:13 UTC
As Jim mentioned in desc33, Creator encrypted the password that stored along with the other data source information and
persisted in file, context.xml.

Creator only used simple encryption with a secret pass key.  Yes, at least use the BASE64Encoder to encrypt



Comment 35 John Baker 2007-09-18 06:58:41 UTC
>I don't really understand the discussion about DatabaseConnection and nodes. It is true that the dependency on nodes is
>wrong and that the dialog should only work with DatabaseConnection's. But does that need to be fixed at this moment?
>Currently the dialog extracts the database URL and the user name from the database connection that the user is
>connecting. Why can't it just to the same with the password?

dbcon is created from the node - cne.getDatabaseConnection()
and in the showDialog method, dbcon.getPassword() is null 
since the password isn't known until the connection is made (user provides password or password is remembered or stored)

Instead, we can discuss this more in 108422.  Previously I wanted to keep discussions in one issue but it's getting off
the topic
Comment 36 Andrei Badea 2007-09-18 17:00:38 UTC
dbcon.getPassword() is null when there is no stored password. But when you check the "Store password for this session"
checkbox in the Connect dialog, disconnect and connect again, dbcon.getPassword() will start returning the stored value.
Comment 37 _ jimdavidson 2007-09-18 19:53:12 UTC
At today's I-Team, I agreed to follow up on this.

It looks like there are three things to do:

A. The technical work, which is mainly writing code to store the password into the xml connection file in
<userdir>/config/Databases/Connections.  Also, work to retrieve it and pre-populate the password field when connecting.  
 
  I'll put together a more detailed spec, but that's basically it.  I don't know whether it's worth scrambling the password.

B. UI change.  I propose changing "Remember password during this session" to "Remember password", with Help that
explains more.  This needs to be cleared at: <http://wiki.netbeans.org/wiki/view/NB6LateUIChanges>

C. ARC case, since we're storing a password.  Indications from David are this this should be OK.  I'll check with Petr
Suchomel, who is apparently the person doing NB ARC.

Sound reasonable?  (B) and (C) are the approvals that we need.  Anything else?
Comment 38 David Vancouvering 2007-09-19 00:06:32 UTC
This looks good to me.
Comment 39 _ jimdavidson 2007-09-19 21:21:45 UTC
New Wiki page added, linked in the URL field.

Two issues need to be decided, both fairly minor.
Comment 40 David Vancouvering 2007-10-04 23:22:56 UTC
Now you can save the password across sessions.  I'm not closing because I want to discuss the user being able to not
have to see the dialog each time, to make it an even better experience for the common case where you just want to
reconnect quickly.

IDE: [10/4/07 3:18 PM] Committing "Database Explorer" started
cvs server: scheduling file `src/org/netbeans/modules/db/resources/connection-1_1.dtd' for addition
cvs server: scheduling file `src/org/netbeans/modules/db/util/Base64.java' for addition
cvs server: scheduling file `test/unit/src/org/netbeans/modules/db/util/Base64Test.java' for addition
cvs server: use 'cvs commit' to add these files permanently
Checking in src/org/netbeans/modules/db/explorer/infos/RootNodeInfo.java;
/cvs/db/src/org/netbeans/modules/db/explorer/infos/RootNodeInfo.java,v  <--  RootNodeInfo.java
new revision: 1.35; previous revision: 1.34
done
Checking in src/org/netbeans/modules/db/explorer/infos/DatabaseNodeInfo.java;
/cvs/db/src/org/netbeans/modules/db/explorer/infos/DatabaseNodeInfo.java,v  <--  DatabaseNodeInfo.java
new revision: 1.58; previous revision: 1.57
done
Checking in src/org/netbeans/modules/db/explorer/infos/ConnectionNodeInfo.java;
/cvs/db/src/org/netbeans/modules/db/explorer/infos/ConnectionNodeInfo.java,v  <--  ConnectionNodeInfo.java
new revision: 1.47; previous revision: 1.46
done
Checking in src/org/netbeans/modules/db/explorer/DatabaseConnection.java;
/cvs/db/src/org/netbeans/modules/db/explorer/DatabaseConnection.java,v  <--  DatabaseConnection.java
new revision: 1.53; previous revision: 1.52
done
Checking in src/org/netbeans/modules/db/explorer/DatabaseConnectionConvertor.java;
/cvs/db/src/org/netbeans/modules/db/explorer/DatabaseConnectionConvertor.java,v  <--  DatabaseConnectionConvertor.java
new revision: 1.11; previous revision: 1.10
done
Checking in src/org/netbeans/modules/db/explorer/dlg/ConnectPanel.form;
/cvs/db/src/org/netbeans/modules/db/explorer/dlg/ConnectPanel.form,v  <--  ConnectPanel.form
new revision: 1.6; previous revision: 1.5
done
Checking in src/org/netbeans/modules/db/explorer/dlg/ConnectPanel.java;
/cvs/db/src/org/netbeans/modules/db/explorer/dlg/ConnectPanel.java,v  <--  ConnectPanel.java
new revision: 1.23; previous revision: 1.22
done
Checking in src/org/netbeans/modules/db/resources/mf-layer.xml;
/cvs/db/src/org/netbeans/modules/db/resources/mf-layer.xml,v  <--  mf-layer.xml
new revision: 1.18; previous revision: 1.17
done
Checking in src/org/netbeans/modules/db/resources/Bundle.properties;
/cvs/db/src/org/netbeans/modules/db/resources/Bundle.properties,v  <--  Bundle.properties
new revision: 1.113; previous revision: 1.112
done
RCS file: /cvs/db/src/org/netbeans/modules/db/resources/connection-1_1.dtd,v
done
Checking in src/org/netbeans/modules/db/resources/connection-1_1.dtd;
/cvs/db/src/org/netbeans/modules/db/resources/connection-1_1.dtd,v  <--  connection-1_1.dtd
initial revision: 1.1
done
RCS file: /cvs/db/test/unit/src/org/netbeans/modules/db/util/Base64Test.java,v
done
Checking in test/unit/src/org/netbeans/modules/db/util/Base64Test.java;
/cvs/db/test/unit/src/org/netbeans/modules/db/util/Base64Test.java,v  <--  Base64Test.java
initial revision: 1.1
done
Checking in src/org/netbeans/modules/db/explorer/actions/ConnectAction.java;
/cvs/db/src/org/netbeans/modules/db/explorer/actions/ConnectAction.java,v  <--  ConnectAction.java
new revision: 1.46; previous revision: 1.45
done
Checking in test/unit/src/org/netbeans/modules/db/explorer/DatabaseConnectionConvertorTest.java;
/cvs/db/test/unit/src/org/netbeans/modules/db/explorer/DatabaseConnectionConvertorTest.java,v  <-- 
DatabaseConnectionConvertorTest.java
new revision: 1.11; previous revision: 1.10
done
Checking in test/unit/src/org/netbeans/modules/db/explorer/bar-connection.xml;
/cvs/db/test/unit/src/org/netbeans/modules/db/explorer/bar-connection.xml,v  <--  bar-connection.xml
new revision: 1.5; previous revision: 1.4
done
RCS file: /cvs/db/src/org/netbeans/modules/db/util/Base64.java,v
done
Checking in src/org/netbeans/modules/db/util/Base64.java;
/cvs/db/src/org/netbeans/modules/db/util/Base64.java,v  <--  Base64.java
initial revision: 1.1
done
IDE: [10/4/07 3:18 PM] Committing "Database Explorer" finished
Comment 41 Andrei Badea 2007-10-05 13:26:59 UTC
Nice work, David. I have some comments though:

AB01: I strongly suggest against having the whole Base64 class in the repository. It does too much. A simple
implementation such as the one in java.util.prefs.Base64 would suit the needs of the DB Explorer just as well, and,
unlike Base64, it can actually be understood in a couple of minutes. Now that NetBeans is under GPL as well we probably
can just take java.util.prefs.Base64.

AB02: I don't think Base64.encodeObject() is the method you want to call. What you want is to convert the password
string to UTF-8 bytes and convert that to base64. Consider that we are allowing module writers to specify a password
when registering a connection in the layer, which is a new API. What are you going to tell an API client? "The password
needs to be encoded by the Base64.encodeObject() method"? You don't want to tell him that. Base64 is not in the API, and
what encodeObject() does is too Java-specific. You need a method like encodeUTF8Bytes().

AB03: The new password element and its value needs to be documented in arch.xml in the use cases section.

AB04: No need for the base64 constant in DatabaseConnectionConvertorTest. Just call Base64.encodeUTF8Bytes('password').

AB05: Instead of calling "conn.setRememberPassword(true)" in DatabaseConnectionConvertorTest can't you just use the new
constructor in DatabaseConnection taking remembedPassword as a parameter?
Comment 42 Andrei Badea 2007-10-05 13:29:47 UTC
AB06: You need to upload connection-1_1.dtd to http://www.netbeans.org/dtds/connection-1_1.dtd by putting it in the
www/www/dtds directory in CVS.
Comment 43 David Vancouvering 2007-10-05 21:14:55 UTC
AB01: I strongly suggest against having the whole Base64 class in the repository. It does too much. A simple
implementation such as the one in java.util.prefs.Base64 would suit the needs of the DB Explorer just as well, and,
unlike Base64, it can actually be understood in a couple of minutes. Now that NetBeans is under GPL as well we probably
can just take java.util.prefs.Base64.

David: I didn't know java.util.prefs.Base64 existed, and very good point that since we're both GPL now (OpenJDK as well
as NB) then I should be able to use it.  Checking with legal to make sure we're kosher.

AB02: I don't think Base64.encodeObject() is the method you want to call. What you want is to convert the password
string to UTF-8 bytes and convert that to base64. Consider that we are allowing module writers to specify a password
when registering a connection in the layer, which is a new API. What are you going to tell an API client? "The password
needs to be encoded by the Base64.encodeObject() method"? You don't want to tell him that. Base64 is not in the API, and
what encodeObject() does is too Java-specific. You need a method like encodeUTF8Bytes().

David: Ah, I see.  I missed that users can register their own connections, thanks for alerting me to that.  And yes, I
see that the password string needs to be an encoding of bytes, not of a Java object, that makes total sense.  But
clients of the API are still going to need to encode the password if they want to include it in their XML file to
auto-register a connection.  What are we going to suggest they do to achieve that?  "You need to encode the password as
a Base64 encoding of a UTF8 string."?  I guess so... But if I were an API client, I'd probably just not specify the
password...

Another question: Does this change need an API review since I'm adding password to the DTD?

AB03: The new password element and its value needs to be documented in arch.xml in the use cases section.

David: OK.

AB04: No need for the base64 constant in DatabaseConnectionConvertorTest. Just call Base64.encodeUTF8Bytes('password').

David: OK, makes sense, thanks.

AB05: Instead of calling "conn.setRememberPassword(true)" in DatabaseConnectionConvertorTest can't you just use the new
constructor in DatabaseConnection taking rememberPassword as a parameter?

David: I'll take a look at that.
Comment 44 David Vancouvering 2007-10-05 21:20:27 UTC
AB06: You need to upload connection-1_1.dtd to http://www.netbeans.org/dtds/connection-1_1.dtd by putting it in the
www/www/dtds directory in CVS.

David: Cheez, how am I supposed to know this stuff? :)  OK, will do.
Comment 45 Andrei Badea 2007-10-06 00:11:33 UTC
> What are we going to suggest they do to achieve that?  "You need to encode
> the password as a Base64 encoding of a UTF8 string."?

Yeah, just that. It might look complex at first, but usually it will not be. Most people will use only ASCII characters
in their passwords, whose UTF-8 encoding is exactly those characters (UTF-8 for 'foo' is 'foo'). So all they need to do
is feed the password through a base64 online encoder, for example.

> Another question: Does this change need an API review since I'm adding
> password to the DTD?

Very good catch. Yes, it would be good to do a fast-track review for this.

> Cheez, how am I supposed to know this stuff? :)

You aren't :-) I don't know if this is documented somewhere or it is just passed down through generations. I was told
about it in IssueZilla too.
Comment 46 David Vancouvering 2007-10-06 00:33:46 UTC
This is a request for a fast-track review for this API change to the DB explorer.  

I am adding a new 'password' element to the connection DTD that is used for storing connection information, upgrading
the version from 1.0 to 1.1.

I will attach a diff to the change between versions 1.0 and 1.1 of the DTD, as well as changes to the arch.xml and
apichanges.xml files.

Andrei and I caught that this needed a review only after checking in the change.  Please let me know if you need
anything else for this review.
Comment 47 David Vancouvering 2007-10-06 00:37:10 UTC
Here is the diff between connection-1_0.dtd and connection-1_1.dtd:

a1
> <?xml version="1.0" encoding="UTF-8"?>
21c22
< <!ELEMENT connection (driver-class,driver-name,database-url,schema,user)>
---
> <!ELEMENT connection (driver-class,driver-name,database-url,schema,user,password)>
25c26
51a53,57
> <!--- The database password (hashed). -->
> <!ELEMENT password EMPTY>
> <!ATTLIST password
>     value CDATA #REQUIRED
Comment 48 David Vancouvering 2007-10-06 00:50:51 UTC
Created attachment 50293 [details]
Changes to the arch.xml and apichanges.xml files
Comment 49 Andrei Badea 2007-10-08 13:27:55 UTC
AB01: You need to increment the specification version to 1.23 in project.properties.

AB02: The class element in apichanges.xml mentions a non-API DatabaseConnection class. Actually probably there is no
need for a class element at all.

AB03: Since the password is optional, the connection element in connection-1_1.dtd should be defined as

<!ELEMENT connection (driver-class,driver-name,database-url,schema,user,password?)>

AB04: Suggest not mentioning any external URLs such as http://www.motobit.com/util/base64-decoder-encoder.asp, since
they can become broken at any time. Actually, suggest the following explanation:

The password element is optional, but if it is included, its value must be the Base64 encoding of the UTF-8
representation of the password. Note the Base64 encoding serves as a simple scrambling to prevent accidental reveal and
it is not indended to offer any real security. Also note that the UTF-8 representation of passwords composed entirely of
ASCII characters is the same as their ASCII representation, so for such passwords all that needs to be done is
converting them to Base64.
Comment 50 David Vancouvering 2007-10-08 17:47:28 UTC
Thanks, Andrei, I'll take care of these.
Comment 51 Andrei Badea 2007-10-09 19:48:59 UTC
I agree with the change.
Comment 52 Andrei Badea 2007-10-10 09:28:40 UTC
Just re-adding the API and API_REVIEW_FAST keywords.
Comment 53 David Vancouvering 2007-10-10 19:10:31 UTC
Requesting another late UI change for this issue.  This was not initially proposed, but further work with this feature
has made it clear we should automatically connect without using the Connect dialog when the password has been saved.  

The user experience will change like this:

*Password not saved*
No change from current experience

*Password saved*
User chooses Connect... from the context menu for a connection, and a progress bar is displayed until the connection is
established.  If the connection fails, the Connect dialog is displayed along with an error message, so the user has the
opportunity to change the parameters for the connection.

If the user wants to stop saving the password or otherwise view the Connect dialog, they can choose Properties... from
the context menu for the connection, and unclick 'Save Password'.  Then they can connect and the Connect dialog is
displayed.

The justification for this change is that for most users, once they save the password, they want to quickly reconnect. 
The exception case is if the user wants to change the password or stop saving it. So we should optimize for the common
case where a quick reconnect is desired.
Comment 54 David Vancouvering 2007-10-10 20:02:00 UTC
Committing changes based on API review feedback.

Awaiting late UI approval before committing change so that the connect dialog is not brought up if the password is saved.

Checking in src/org/netbeans/modules/db/explorer/DatabaseConnectionConvertor.java;
/cvs/db/src/org/netbeans/modules/db/explorer/DatabaseConnectionConvertor.java,v  <--  DatabaseConnectionConvertor.java
new revision: 1.12; previous revision: 1.11
done
Checking in src/org/netbeans/modules/db/explorer/DatabaseConnection.java;
/cvs/db/src/org/netbeans/modules/db/explorer/DatabaseConnection.java,v  <--  DatabaseConnection.java
new revision: 1.54; previous revision: 1.53
done
Checking in test/unit/src/org/netbeans/modules/db/explorer/DatabaseConnectionConvertorTest.java;
/cvs/db/test/unit/src/org/netbeans/modules/db/explorer/DatabaseConnectionConvertorTest.java,v  <-- 
DatabaseConnectionConvertorTest.java
new revision: 1.12; previous revision: 1.11
done
Checking in test/unit/src/org/netbeans/modules/db/explorer/bar-connection.xml;
/cvs/db/test/unit/src/org/netbeans/modules/db/explorer/bar-connection.xml,v  <--  bar-connection.xml
new revision: 1.6; previous revision: 1.5
done
Checking in test/unit/src/org/netbeans/modules/db/util/Base64Test.java;
/cvs/db/test/unit/src/org/netbeans/modules/db/util/Base64Test.java,v  <--  Base64Test.java
new revision: 1.2; previous revision: 1.1
done
Checking in apichanges.xml;
/cvs/db/apichanges.xml,v  <--  apichanges.xml
new revision: 1.9; previous revision: 1.8
done
Checking in arch.xml;
/cvs/db/arch.xml,v  <--  arch.xml
new revision: 1.18; previous revision: 1.17
done
Checking in src/org/netbeans/modules/db/resources/connection-1_1.dtd;
/cvs/db/src/org/netbeans/modules/db/resources/connection-1_1.dtd,v  <--  connection-1_1.dtd
new revision: 1.2; previous revision: 1.1
done
Checking in nbproject/project.properties;
/cvs/db/nbproject/project.properties,v  <--  project.properties
new revision: 1.28; previous revision: 1.27
done
Checking in src/org/netbeans/modules/db/util/Base64.java;
/cvs/db/src/org/netbeans/modules/db/util/Base64.java,v  <--  Base64.java
new revision: 1.2; previous revision: 1.1
done
Comment 55 John Baker 2007-10-11 04:46:53 UTC
In the Connection dialog, the label next to the Remember password mentions to look in the help
It would be better to have a Help button or have a link to an article / whitepaper on how the passwords
are stored
Comment 56 Kenneth Ganfield 2007-10-11 09:39:14 UTC
I think that the message next to remember password should be removed. please consider moving it to the line below or to
an inline text area below. 

I would agree with a help id and will approve such a UI change.
I can add the help topic explaining the new behaviour (though cannot promise for beta2 at this stage, but possibly if id
is there before branch) and I would suggest the help topic also have a link to the wiki faqs with greater detail on how
the password is saved and on how to secure the password.
Comment 57 David Vancouvering 2007-10-11 19:10:38 UTC
Jim had said the UI change to save the password already passed Late UI Change review, and his change didn't include a
help button.  So  this code is already checked in, I thought we were cool with this.

But anyway, water under the bridge, we can try to fix this in the next few days.

I didn't hear any objections about not bringing up the dialog, which was the focus of the Late UI Change review request
I created.  Does that means it's approved?  I'm not sure how this process works...

Thanks,

David
Comment 58 John Baker 2007-10-11 19:17:19 UTC
The late UI process:  the late UI page will get updated with an OK for this task
Jano approved and if "OK" wasn't added for the late UI item  then mark it OK with a 
commment - approved by Jano
Comment 59 John Baker 2007-10-12 01:55:00 UTC
 Added Help button to the dialog, removed extra checkbox text in the dialog and created help id "db_save_password" 
 After removing the extra text, the dialog was packed, thus shrunk, so had to set the preferred size (prototype display
value) of the schema dropdown
 

cvs commit: Examining db/src/org/netbeans/modules/db/explorer/dlg
cvs commit: Examining db/src/org/netbeans/modules/db/resources
Checking in db/src/org/netbeans/modules/db/explorer/dlg/ConnectionDialog.java;
/cvs/db/src/org/netbeans/modules/db/explorer/dlg/ConnectionDialog.java,v  <--  ConnectionDialog.java
new revision: 1.15; previous revision: 1.14
done
Checking in db/src/org/netbeans/modules/db/explorer/dlg/SchemaPanel.form;
/cvs/db/src/org/netbeans/modules/db/explorer/dlg/SchemaPanel.form,v  <--  SchemaPanel.form
new revision: 1.7; previous revision: 1.6
done
Checking in db/src/org/netbeans/modules/db/explorer/dlg/SchemaPanel.java;
/cvs/db/src/org/netbeans/modules/db/explorer/dlg/SchemaPanel.java,v  <--  SchemaPanel.java
new revision: 1.19; previous revision: 1.18
done
Checking in db/src/org/netbeans/modules/db/resources/Bundle.properties;
/cvs/db/src/org/netbeans/modules/db/resources/Bundle.properties,v  <--  Bundle.properties
new revision: 1.114; previous revision: 1.113
done
Checking in db/src/org/netbeans/modules/db/resources/org-netbeans-modules-db-mainpage.xml;
/cvs/db/src/org/netbeans/modules/db/resources/org-netbeans-modules-db-mainpage.xml,v  <-- 
org-netbeans-modules-db-mainpage.xml
new revision: 1.4; previous revision: 1.3
done
Comment 60 John Baker 2007-10-12 03:20:48 UTC
I ended up removing the extra text for the checkbox 

Before:
&Remember password (potentially insecure; see help for how to secure your password)
After:
&Remember password

Is this sufficient or should the warning about a potentially insecure password stand out?
This will be documented in the Help topic, I assume ?

Comment 61 John Baker 2007-10-12 17:38:30 UTC
Ken requested distinct help ids for the Connect and New Database Connection dialog
I will attach diffs
Comment 62 John Baker 2007-10-12 17:40:21 UTC
Created attachment 50843 [details]
diff of changes to make for using distinct help ids
Comment 63 David Vancouvering 2007-10-12 18:02:52 UTC
This looks good to me.
Comment 64 David Vancouvering 2007-10-13 00:53:03 UTC
Connect dialog now is not brought up - just progress bar - when the password is saved and you connect from DB Explorer.

Checking in ConnectAction.java;
/cvs/db/src/org/netbeans/modules/db/explorer/actions/ConnectAction.java,v  <--  ConnectAction.java
new revision: 1.47; previous revision: 1.46
done
Comment 65 David Vancouvering 2007-10-13 00:54:39 UTC
There is one problem remaining with this issue.  If the password is changed for a user, then an error dialog is raised,
but after pressing [OK] the Connect dialog should be brought up but it's not.  I'm working on this.  
Comment 66 David Vancouvering 2007-10-13 04:58:54 UTC
I am still working on solving this problem, but I am not going to push to get this fix in for Beta 2.  

The behavior:

- You save the password
- Administrator changes the password for that user on the database
- You try to connect, and the connection fails, and you don't get the Connect dialog

The reasonable workaround is to disable 'save password' and then you get the Connect dialog.

I'll log a separate P3 bug for this and I'm going to mark this issue as fixed.
Comment 67 John Baker 2007-10-16 08:21:12 UTC
improved the implementation of Help for the Connection dialogs

Checking in db/src/org/netbeans/modules/db/explorer/actions/ConnectAction.java;
/cvs/db/src/org/netbeans/modules/db/explorer/actions/ConnectAction.java,v  <--  ConnectAction.java
new revision: 1.48; previous revision: 1.47
done
Checking in db/src/org/netbeans/modules/db/explorer/actions/ConnectUsingDriverAction.java;
/cvs/db/src/org/netbeans/modules/db/explorer/actions/ConnectUsingDriverAction.java,v  <--  ConnectUsingDriverAction.java
new revision: 1.38; previous revision: 1.37
done
Checking in db/src/org/netbeans/modules/db/explorer/dlg/ConnectionDialog.java;
/cvs/db/src/org/netbeans/modules/db/explorer/dlg/ConnectionDialog.java,v  <--  ConnectionDialog.java
new revision: 1.18; previous revision: 1.17