Page 1 of 1
:?: Patch for ZConnection.Connect
Posted: 12.05.2009, 10:58
by Ostfriese
Currently the username and password properties are not updated if they were changed in the login dialog. So I've modified
Code: Select all
procedure TZConnection.Connect;
var
//Local variables declared in order to preserve the original property value
//and to avoid the storage of password
Username, Password: string;
begin
if FConnection = nil then
begin
// Fixes Zeos Bug 00056
// try
DoBeforeConnect;
// except
//This is here to support aborting the Connection in BeforeConnect event without fatal errors
// on E: EAbort do
// Exit;
// end;
UserName := FUser;
Password := FPassword;
if FLoginPrompt then
begin
{ Defines user name }
if UserName = '' then
UserName := FProperties.Values['UID'];
if UserName = '' then
UserName := FProperties.Values['username'];
{ Defines user password }
if Password = '' then
Password := FProperties.Values['PWD'];
if Password = '' then
Password := FProperties.Values['password'];
if Assigned(FOnLogin) then
FOnLogin(Self, UserName, Password)
else
begin
if Assigned(LoginDialogProc) then
begin
if not LoginDialogProc(FDatabase, UserName, Password) then
Exit
end
else
raise Exception.Create(SLoginPromptFailure);
end;
end;
ShowSqlHourGlass;
try
FConnection := DriverManager.GetConnectionWithParams(
ConstructURL(UserName, Password), FProperties);
try
with FConnection do
begin
SetAutoCommit(FAutoCommit);
SetReadOnly(FReadOnly);
SetCatalog(FCatalog);
SetTransactionIsolation(FTransactIsolationLevel);
Open;
end;
except
FConnection := nil;
raise;
end;
finally
HideSqlHourGlass;
end;
if not FConnection.IsClosed then
DoAfterConnect;
end;
end;
to
Code: Select all
procedure TZConnection.Connect;
var
//Local variables declared in order to preserve the original property value
//and to avoid the storage of password
Username, Password: string;
begin
if FConnection = nil then
begin
// Fixes Zeos Bug 00056
// try
DoBeforeConnect;
// except
//This is here to support aborting the Connection in BeforeConnect event without fatal errors
// on E: EAbort do
// Exit;
// end;
UserName := FUser;
Password := FPassword;
if FLoginPrompt then
begin
{ Defines user name }
if UserName = '' then
UserName := FProperties.Values['UID'];
if UserName = '' then
UserName := FProperties.Values['username'];
{ Defines user password }
if Password = '' then
Password := FProperties.Values['PWD'];
if Password = '' then
Password := FProperties.Values['password'];
if Assigned(FOnLogin) then
FOnLogin(Self, UserName, Password)
else
begin
if Assigned(LoginDialogProc) then
begin
if not LoginDialogProc(FDatabase, UserName, Password) then
Exit
else
//HA 090512 Update username and password after login dialog closed
begin
FUser := UserName;
FPassword := Password;
end;
end
else
raise Exception.Create(SLoginPromptFailure);
end;
end;
ShowSqlHourGlass;
try
FConnection := DriverManager.GetConnectionWithParams(
ConstructURL(UserName, Password), FProperties);
try
with FConnection do
begin
SetAutoCommit(FAutoCommit);
SetReadOnly(FReadOnly);
SetCatalog(FCatalog);
SetTransactionIsolation(FTransactIsolationLevel);
Open;
end;
except
FConnection := nil;
raise;
end;
finally
HideSqlHourGlass;
end;
if not FConnection.IsClosed then
DoAfterConnect;
end;
end;
In this way the username and password are set to the username and password which are typed into the login dialog.
Posted: 12.05.2009, 20:53
by seawolf
Looks like similar to Zeos bug 173, never checked
Problem is related to the TLoginEvent where user and password seems to be declared as readonly.
Posted: 13.05.2009, 20:38
by mdaems
var
//Local variables declared in order to preserve the original property value
//and to avoid the storage of password
Username, Password: string;
Doesn't this look like this is done on purpose? In my opinion (but who am I??) this shouldn't be changed. If one really wants to set the component properties to whatever input comes from the user, he can always call LoginDialogProc himself and set the property values.
So, to all others reading : let us know your opinion on this. I'm not applying this patch to the official sources for now.
Mark
Posted: 14.05.2009, 09:59
by Ostfriese
From my point of view I expect that the properties user and password allways contain the current values which are either set by the program or modified by the login prompt. Otherwise they are useless because it's impossible to determine which username is currently used.
Posted: 14.05.2009, 14:25
by Ostfriese
Okay, I've reworked my patch in that way that I've additionaly removed the protected FUser and FPassword variables. The User and Password properties are now setting and getting the coresponding connection properties.
After login dialog is closed the private procedure which set the username and password properties are called to update the connection properties.
All changes are surrounded by
comments for easier finding
Posted: 14.05.2009, 15:11
by mdaems
Ohhh... Stop... wait... my message must have been a little unclear to you. Sorry for that.
The problem is not that the Username and Pasword properties exist. They only provide an alternative way to provide the connection with a username and password to connect. This is good for those who want to avoid using a login prompt or OnLogin event handler. And it is way easier to understand how to set a user/password this way than to use those primitive string settings used by Delphi's standard components. But it's similar in that way that it's a plain text field defined at design time or in the user program. So I certainly did not want to change this current behaviour.
What worried me more was the fact that your initial patch did change the programmatically readable/modifiable component settings. This isn't similar to the way Delphi works AFAIK. Using the Login Dialog doesn't change eventually preset values and it is 'impossible' to get the user provided username and password easily.
In general I believe it's bad practice to keep a user provided username/password combination longer around than is strictly necessary. Apparently Delphi didn't like this either. So I don't like to promote this.
I hope some others want to comment on this as well. As your signature says : just to get the information necessary to guarantee a democratic decision.
Mark
Posted: 14.05.2009, 15:34
by Ostfriese
mdaems: Just take a look at my modified unit. The properties are still there but they use the connection properties username and password instead of extra variables.
This is IMHO more consequent because there will be only one location where the username and password are stored or is there a reason I didn't see for this behaviour?
Posted: 14.05.2009, 19:13
by gto
Leaving my 0.05 cents:
The original behaviour was designed to keep the typed settings just into the login procedure and don't propagate them through ZConnection properties. The patch from Ostfriese make it save the typed user and password over the ZConnection values, right?
Well, if this is a security point, I really don't know. Indeed, seems more logic, as you're storing user/pass in one place only, and keeping them updated through running of application.
But, looking for the login dialog box, it need the user to input user/pass everytime. I don't know people which use the default dialog (seems better to build your own), but storing the values over the ZConnection values have no mean, the user will be prompted for them everytime it's used again.
I can suggest the follow options:
- Save user and pass typed in LoginDialog under ZConnection params. Something like params["logindialog_user" = "john"].
- In future, develop our own LoginDialog box, which save typed passwords between sessions of a running application.
Posted: 18.05.2009, 23:36
by mdaems
Okay,
I had a look again at your patch and gto's comments.
Some new conclusions/questions about your (last) patch.
- What's the use of deprecate-ing FUser and FPassword when they are just ignored? In that case I propose to just drop them.
- Why parsing the Properties object in your own code? Isn't the standard TStrings interface sufficient for access by name? Please correct when you can't justify.
- Changes can not be done to 6.6 for this issue. Let's keep them in the 7.X versions becauseof backward compatibility. Iagine someonwe writing his ZConnection.properties to an ini file. At once the password would be visible to everybody who has access to this file. This is one of the reasons to stick with current implementation using private variables THis is my most mportant reason to keep these variables private/protected.
- Please work startting from testing branch when making patches, unless this branch is too different from 6.6 and the patch is for 6.6 only.
Mark
Posted: 19.05.2009, 07:16
by Ostfriese
mdaems wrote:
- What's the use of deprecate-ing FUser and FPassword when they are just ignored? In that case I propose to just drop them.
It's my case of removing used variables: Mark them as deprecate and remove them some versions later
mdaems wrote:
- Why parsing the Properties object in your own code? Isn't the standard TStrings interface sufficient for access by name? Please correct when you can't justify.
IMHO a TStrings can't look for partial strings so I've to it on my own.
Posted: 19.05.2009, 21:44
by mdaems
Ostfriese,
From your patched version:
Code: Select all
//HA 090514 retrieve username and password from the connection properties ..
//UserName := FUser;
//Password := FPassword;
UserName := GetUsername;
Password := GetPassword;
//..
if FLoginPrompt then
begin
{ Defines user name }
if UserName = '' then
UserName := FProperties.Values['UID'];
if UserName = '' then
UserName := FProperties.Values['username'];
{ Defines user password }
if Password = '' then
Password := FProperties.Values['PWD'];
if Password = '' then
Password := FProperties.Values['password'];
When you look a little below your change you'll notice GetPassword can be replaced by FProperties.Values['password']. Here it's used to prefill the login dialog fields.
Or am I overlooking something?
Mark
Posted: 19.05.2009, 21:47
by seawolf
Hi, loginprompt property does not works (as described in zeos bug 173)
because of TLoginEvent
Posted: 19.05.2009, 21:51
by mdaems
Yes, I know, but that wasn't really the subject of this topic
. Here we're talking about the goods and/or bads of the proposed patch. We welcome your opinion on the subect, however. Please give it a little study.
Mark
Posted: 11.08.2009, 08:15
by Ostfriese
I've modified login event declaration in ZConnection.pas based on the description in Zeos Bug #173 to make the username and password persistent. I hope this will also help to close the corresponding bug.
My patched ZConnection.pas is based on ZeosDBO 6.6.5