Hi,
As mentioned in my earlier communication code calling this function is checking
for file existence. So if we decide to add code for creation of full path, then
similar code has to be removed from location of call to this function.
Otherwise, it will end up with multiple error messages. It's wxWidget's wxFile
that throws error.
So, I've created two patches, and we can go with one of them.
1. Let GetConfigFile function just read value from PGPASSFILE and return as it
is as like, similar to way it creates default path(It doesn't create file in
case of default path as well). And calling functions are taking care of path
validation and error messages.
2. Let GetConfigFile function read value from PGPASSFILE and create file path
,it will show error message in case it can't. In this case calling code only
should check existence of file before going ahead, and not try to create or
read file, otherwise , user will end up with multiple message boxes with same
error.
regards,
Prasad
Sent: Wednesday, March 04, 2015 at 11:35 AM
From: "Ashesh Vashi" <ashesh.va...@enterprisedb.com>, func
To: "Dave Page" <dp...@pgadmin.org>
Cc: Prasad <prasa...@mail.com>, pgadmin-hackers <pgadmin-hackers@postgresql.org>
Subject: Re: [pgadmin-hackers] Patch : PGPASSFILE fix
On Wed, Mar 4, 2015 at 4:40 PM, Dave Page <dp...@pgadmin.org> wrote:
On Wed, Mar 4, 2015 at 11:06 AM, Ashesh Vashi
<ashesh.va...@enterprisedb.com[ashesh.va...@enterprisedb.com]> wrote:
On Wed, Mar 4, 2015 at 4:09 PM, Dave Page
<dp...@pgadmin.org[dp...@pgadmin.org]> wrote:
I think we should try to create the full path if necessary, and simply
throw an error if we can't.
And, I think - we should switch back to default pgpass configuration file.
No, because that's a security risk (writing the password to a file that wasn't
what the user intended).
Agree.
--
Thanks & Regards,
Ashesh Vashi
EnterpriseDB INDIA: Enterprise PostgreSQL Company[http://www.enterprisedb.com]
http://www.linkedin.com/in/asheshvashi[http://www.linkedin.com/in/asheshvashi]
--
Thanks & Regards,
Ashesh Vashi
EnterpriseDB INDIA: Enterprise PostgreSQL Company[http://www.enterprisedb.com]
http://www.linkedin.com/in/asheshvashi[http://www.linkedin.com/in/asheshvashi]
On Wed, Mar 4, 2015 at 10:01 AM, Prasad <prasa...@mail.com[prasa...@mail.com]>
wrote:
> Alright , I'll revert to PGPASS check.
> Existing function only creates folder containing file. With this case, whats
> expected ? Reading value in PGPASSFILE and try to create folder containing
> pgpass file (Assuming it's valid path)? Remember, it's environment variable.
> User can specify anything in there. Some garbage value as well. If we don't
> do any validation there, user will automatically see error with complain
> about file ?
>
> thanks and regards,
> Prasad
>
>
> Sent: Wednesday, March 04, 2015 at 7:48 AM
> From: "Ashesh Vashi"
> <ashesh.va...@enterprisedb.com[ashesh.va...@enterprisedb.com]>
> To: Prasad <prasa...@mail.com[prasa...@mail.com]>
> Cc: pgadmin-hackers
> <pgadmin-hackers@postgresql.org[pgadmin-hackers@postgresql.org]>
> Subject: Re: [pgadmin-hackers] Patch : PGPASSFILE fix
>
> On Wed, Mar 4, 2015 at 8:44 AM, Prasad <prasa...@mail.com[prasa...@mail.com]>
> wrote:
>
> Ashesh,
>
> Thanks for reviewing patch,
> Code I have removed in I think, was switch statement inside if condition,
> which doesn't make sense.
> ie.
> if (var == 2)
> {
> switch (var)
> case 2:
> .....
> break;
> }
>
> that's why I removed it, because it's redundant.
> Agree about redundancy, but you've also removed the code for checking the
> PGPASS check at the start of the function.
> i.e.
> @@ -762,35 +762,33 @@ void sysSettings::SetCanonicalLanguage(const wxLanguage
> &lang)
> //////////////////////////////////////////////////////////////////////////
> wxString sysSettings::GetConfigFile(configFileName cfgname)
> {
> - if (cfgname == PGPASS)
> - {
>
> I am not agree with that.
> About creation of directory, I'm not sure if this validation is required.
>Existing code creates directory postgresql (only on windows) according to
>http://www.postgresql.org/docs/9.3/static/libpq-pgpass.html[http://www.postgresql.org/docs/9.3/static/libpq-pgpass.html][http://www.postgresql.org/docs/9.3/static/libpq-pgpass.html%5Bhttp://www.postgresql.org/docs/9.3/static/libpq-pgpass.html%5D]
> , and it doesn't create file. I'm not sure whether this kind of validation is
>expected in this function.
> I think - it is.
> Because - it could be used to save the updated password in the PGPASS file.
>
> -- Ashesh
> regards,
> Prasad
>
> Sent: Wednesday, March 04, 2015 at 7:15 AM
> From: "Ashesh Vashi"
> <ashesh.va...@enterprisedb.com[ashesh.va...@enterprisedb.com][ashesh.va...@enterprisedb.com[ashesh.va...@enterprisedb.com]]>
> To: Prasad
> <prasa...@mail.com[prasa...@mail.com][prasa...@mail.com[prasa...@mail.com]]>
> Cc: pgadmin-hackers
> <pgadmin-hackers@postgresql.org[pgadmin-hackers@postgresql.org][pgadmin-hackers@postgresql.org[pgadmin-hackers@postgresql.org]]>
> Subject: Re: [pgadmin-hackers] Patch : PGPASSFILE fix
>
> Hi Prasad,
> I see couple of issues with your patch.* Please generate the patch using
>'git diff'.
> I could not apply your patch straight forwardly.
> I had to use the patch utility.
> * Please follow the coding style of pgAdmin.
> You can find it at
>https://wiki.postgresql.org/wiki/PgAdmin_Internals#Coding_Style.*[https://wiki.postgresql.org/wiki/PgAdmin_Internals#Coding_Style.*][https://wiki.postgresql.org/wiki/PgAdmin_Internals#Coding_Style.*[https://wiki.postgresql.org/wiki/PgAdmin_Internals%23Coding_Style.*]]
> Do not remove any of the existing code.
> It has been kept there keeping in mind about future development extending
>support of the existing functionality.
> You've removed couple of lines in the sysSettings::GetConfigFile(...)
>function, which is not good.
>
> In your code:* Checked only for PGPASSFILE environment variable.
> * Need to check the existence of the file.
> * Take required actions (if that file/parent directory does not exists).
> i.e. Create parent directory
>
>
>
> --
> Thanks & Regards,
>
> Ashesh Vashi
> EnterpriseDB INDIA: Enterprise PostgreSQL
> Company[http://www.enterprisedb.com[http://www.enterprisedb.com][http://www.enterprisedb.com[http://www.enterprisedb.com]]]
>
> http://www.linkedin.com/in/asheshvashi[http://www.linkedin.com/in/asheshvashi][http://www.linkedin.com/in/asheshvashi[http://www.linkedin.com/in/asheshvashi]][http://www.linkedin.com/in/asheshvashi%5Bhttp://www.linkedin.com/in/asheshvashi%5D%5Bhttp://www.linkedin.com/in/asheshvashi%5Bhttp://www.linkedin.com/in/asheshvashi%5D%5D]
>
> On Sun, Mar 1, 2015 at 11:08 PM, Prasad
> <prasa...@mail.com[prasa...@mail.com][prasa...@mail.com[prasa...@mail.com]][prasa...@mail.com[prasa...@mail.com][prasa...@mail.com[prasa...@mail.com]]]>
> wrote:
> Hi,
>
> Find attached fix for reading PGPASSFILE environment variable for pg password
> file.
>
> regards,
> Prasad
>
> --
> Sent via pgadmin-hackers mailing list
> (pgadmin-hackers@postgresql.org[pgadmin-hackers@postgresql.org][pgadmin-hackers@postgresql.org[pgadmin-hackers@postgresql.org]][pgadmin-hackers@postgresql.org[pgadmin-hackers@postgresql.org][pgadmin-hackers@postgresql.org[pgadmin-hackers@postgresql.org]]])
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgadmin-hackers[http://www.postgresql.org/mailpref/pgadmin-hackers][http://www.postgresql.org/mailpref/pgadmin-hackers[http://www.postgresql.org/mailpref/pgadmin-hackers]][http://www.postgresql.org/mailpref/pgadmin-hackers%5Bhttp://www.postgresql.org/mailpref/pgadmin-hackers%5D%5Bhttp://www.postgresql.org/mailpref/pgadmin-hackers%5Bhttp://www.postgresql.org/mailpref/pgadmin-hackers%5D%5D]
>
>
>
> --> Sent via pgadmin-hackers mailing list
> (pgadmin-hackers@postgresql.org[pgadmin-hackers@postgresql.org])
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgadmin-hackers[http://www.postgresql.org/mailpref/pgadmin-hackers]
--
Dave Page
Blog: http://pgsnake.blogspot.com[http://pgsnake.blogspot.com]
Twitter: @pgsnake
EnterpriseDB UK: http://www.enterprisedb.com[http://www.enterprisedb.com]
The Enterprise PostgreSQL Company
--
Dave Page
Blog: http://pgsnake.blogspot.com[http://pgsnake.blogspot.com]
Twitter: @pgsnake
EnterpriseDB UK: http://www.enterprisedb.com[http://www.enterprisedb.com]
The Enterprise PostgreSQL Companydiff --git a/pgadmin/utils/sysSettings.cpp b/pgadmin/utils/sysSettings.cpp
index e9f270c..a4de0af 100644
--- a/pgadmin/utils/sysSettings.cpp
+++ b/pgadmin/utils/sysSettings.cpp
@@ -762,6 +762,7 @@ void sysSettings::SetCanonicalLanguage(const wxLanguage &lang)
//////////////////////////////////////////////////////////////////////////
wxString sysSettings::GetConfigFile(configFileName cfgname)
{
+ wxASSERT_MSG(cfgname == sysSettings::PGPASS,wxT("Handles only pgpass configuration"));
if (cfgname == PGPASS)
{
#if wxCHECK_VERSION(2, 9, 5)
@@ -769,27 +770,24 @@ wxString sysSettings::GetConfigFile(configFileName cfgname)
#else
wxStandardPaths stdp;
#endif
- wxString fname = stdp.GetUserConfigDir();
+ wxString fname;
+ bool bpsfile = wxGetEnv(wxT("PGPASSFILE"),&fname);
+ if (!bpsfile)
+ {
+ fname = stdp.GetUserConfigDir();
#ifdef WIN32
- fname += wxT("\\postgresql");
- if (!wxDirExists(fname))
+ fname += wxT("\\postgresql");
+ if (!wxDirExists(fname))
wxMkdir(fname);
- switch(cfgname)
- {
- case PGPASS:
- fname += wxT("\\pgpass.conf");
- break;
+ fname += wxT("\\pgpass.conf");
}
#else
- switch(cfgname)
- {
- case PGPASS:
- fname += wxT("/.pgpass");
- break;
+ fname += wxT("/.pgpass");
}
#endif
return fname;
}
+
return wxT("");
}
diff --git a/pgadmin/frm/frmPgpassConfig.cpp b/pgadmin/frm/frmPgpassConfig.cpp
index 19ed7db..438eb3f 100644
--- a/pgadmin/frm/frmPgpassConfig.cpp
+++ b/pgadmin/frm/frmPgpassConfig.cpp
@@ -61,12 +61,12 @@ frmPgpassConfig::frmPgpassConfig(frmMain *parent)
lastPath = sysSettings::GetConfigFile(sysSettings::PGPASS);
wxFile f;
- if (!f.Exists(lastPath))
- f.Create(lastPath, false, wxS_DEFAULT);
-
- OpenLastFile();
-
- helpMenu->Enable(MNU_HINT, false);
+ if (f.Exists(lastPath))
+ {
+ OpenLastFile();
+ }
+
+ helpMenu->Enable(MNU_HINT, false);
toolBar->EnableTool(MNU_HINT, false);
}
diff --git a/pgadmin/schema/pgServer.cpp b/pgadmin/schema/pgServer.cpp
index caba8b1..3e302e7 100644
--- a/pgadmin/schema/pgServer.cpp
+++ b/pgadmin/schema/pgServer.cpp
@@ -610,12 +610,11 @@ void pgServer::StorePassword()
{
wxString fname = passwordFilename();
- wxUtfFile file;
if (!wxFile::Exists(fname))
{
- file.Create(fname, false, S_IREAD | S_IWRITE);
+ return;
}
-
+ wxUtfFile file;
// Don't try to read and write in one OP - it doesn't work well
wxString before;
file.Open(fname, wxFile::read, wxFONTENCODING_SYSTEM);
diff --git a/pgadmin/utils/sysSettings.cpp b/pgadmin/utils/sysSettings.cpp
index e9f270c..61a9e05 100644
--- a/pgadmin/utils/sysSettings.cpp
+++ b/pgadmin/utils/sysSettings.cpp
@@ -762,6 +762,7 @@ void sysSettings::SetCanonicalLanguage(const wxLanguage &lang)
//////////////////////////////////////////////////////////////////////////
wxString sysSettings::GetConfigFile(configFileName cfgname)
{
+ wxASSERT_MSG(cfgname == sysSettings::PGPASS,wxT("Handles only pgpass configuration"));
if (cfgname == PGPASS)
{
#if wxCHECK_VERSION(2, 9, 5)
@@ -769,27 +770,28 @@ wxString sysSettings::GetConfigFile(configFileName cfgname)
#else
wxStandardPaths stdp;
#endif
- wxString fname = stdp.GetUserConfigDir();
+ wxString fname;
+ bool bpsfile = wxGetEnv(wxT("PGPASSFILE"),&fname);
+ if (!bpsfile)
+ {
+ fname = stdp.GetUserConfigDir();
#ifdef WIN32
- fname += wxT("\\postgresql");
- if (!wxDirExists(fname))
+ fname += wxT("\\postgresql");
+ if (!wxDirExists(fname))
wxMkdir(fname);
- switch(cfgname)
- {
- case PGPASS:
- fname += wxT("\\pgpass.conf");
- break;
+ fname += wxT("\\pgpass.conf");
}
#else
- switch(cfgname)
- {
- case PGPASS:
- fname += wxT("/.pgpass");
- break;
+ fname += wxT("/.pgpass");
}
#endif
+ wxFile f;
+ if (!f.Exists(fname))
+ f.Create(fname, false, wxS_DEFAULT);
+
return fname;
}
+
return wxT("");
}
--
Sent via pgadmin-hackers mailing list (pgadmin-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgadmin-hackers
