Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Added support for preparing the encrypted password (PQencryptPasswordConn) #576

Merged
merged 13 commits into from
May 20, 2018
Merged

Added support for preparing the encrypted password (PQencryptPasswordConn) #576

merged 13 commits into from
May 20, 2018

Conversation

asheshv
Copy link
Contributor

@asheshv asheshv commented Jul 17, 2017

Adding support for preparing the encrypted password using the libpq functions - 'PQencryptPasswordConn', and 'PQencryptPassword'.

This will allow the psycopg2 application to prepare the encrypted form of Password based on the current password encryption algorithm.

@asheshv
Added support for preparing the encrypted password of a PostgreSQL
cfb0937 
password using the libpq functions - 'PQencryptPasswordConn', and
'PQencryptPassword'.
@asheshv
Using 'Text_FromUTF8' macro for transforming the encrypted C string to
78eb80d 
Python string to make it Python 3 compatible.
@asheshv
When compiled with libpq version < 10, it raises 'psycopg2.NotSupport…
2c1966a 
…edError' (not, psycopg2.ProgrammingError).
@asheshv
'encrypt_password' raises 'psycopg2.NotSupportedErorr' exception for
e089d94 
server version >= 10, when compiled using libpq version < 10, when no
algorithm is specified.
@dvarrazzo
Copy link
Member

Hi, thank you for this MR, I'll take a look at it.

@asheshv
Copy link
Contributor Author

asheshv commented Aug 2, 2017

Just a gentle reminder!
In order to add complete support for PostgreSQL 10 in pgAdmin 4, we need this.

@dvarrazzo
Copy link
Member

@asheshv the fact this MR is required for pgAdmin 4 support of PG 10 is something I'm learning just now: wouldn't be the case of being more explicit about your goals? I know pgAdmin now depends on psycopg: I'm very honoured about this and I'm happy to support the project responsibly, but I have to know things, in order to act.

I didn't consider merging this in a bugfix release. If you say it has some importance we can discuss about that. But please speak.

The ML is the most appropriate place to discuss about new features and the timeline to push them.

@asheshv
Copy link
Contributor Author

asheshv commented Aug 3, 2017

@dvarrazzo - Totally agree with you.
It was my mistake not to inform you the reason for the MR.
I should have discussed about the feature, and intention behind this MR.

Thanks for your quick reply, and support.
I really appreciate.
Let me send a mail to ML, and discuss further about it there.

@dvarrazzo dvarrazzo mentioned this pull request Dec 4, 2017
Merged
@asheshv
Copy link
Contributor Author

asheshv commented Dec 17, 2017 •
edited
Loading

@dvarrazzo - Would you please review, and commit it?

@dvarrazzo
Copy link
Member

There are several point of my previous review that have not been addressed, but appear outdated because you moved the code into a different file.

@asheshv
Copy link
Contributor Author

asheshv commented Dec 18, 2017 •
edited
Loading

As per the feedback:

I don't like the function as a connection method. The function is a wrapper to both PQencryptPasswordConn and PQencryptPassword, so the connection is
actually an optional parameter.

Hence - I've moved the code such that we can export it through the 'psycopg2.extensions'.
Also - made the connection/cursor object as an optional parameter in the function.

You also said:

There are a few implementation changes pretty unequivocal to fix bad
behaviour in corner cases and to adopt the same coding style of the
rest of the project.

Would you please point that out?
Sorry - but, I am too naive for this project.

-- Thanks, Ashesh

@dvarrazzo
Copy link
Member

Would you please point that out?

I've added a punctual review, no?

@asheshv
Copy link
Contributor Author

asheshv commented Dec 18, 2017

Did you mean?

when to release this function? Should we release a psycopg 2.8 to expose that? I guess releasing in a 2.7.X release is impolite...

To be honest, I am not right person to answer that.
I would accept as per the project standard.

@dvarrazzo dvarrazzo added this to the psycopg 2.8 milestone Jan 10, 2018
@dpage
Copy link

dpage commented Apr 3, 2018

@dvarrazzo Do you have any thoughts on when this might be merged and 2.8 released? PostgreSQL 10 has been out for ~6 months now and we're still unable to support SCRAM.

Thanks!

asheshv and others added 7 commits May 8, 2018 15:17
@asheshv
Merge branch 'master' into master
1bec2bd
@asheshv
Fixed the string format error reported by Travis-CI.
36f0db8 
Reference: https://travis-ci.org/psycopg/psycopg2/jobs/376288585
@dvarrazzo
Merge branch 'master' into encrypt-pass
0161d54
@dvarrazzo
Fixed code flow in encrypt_password()
a306390 
Fixed several shortcomings highlighted in psycopg#576 and not fixed as
requested.

Also fixed broken behaviour of ignoring the algorithm if the connection
is missing.
@dvarrazzo
encrypt_password docs moved to extension module and updated
9e4f89a
@dvarrazzo
Fixed keywords support for encrypt_password and tests completed
abca14d
@dvarrazzo
Fixed refcount handling in encrypt_password
9cf658e 
Added tests to check bad types, which discovered the above problem: on
type error we would have decref'd on exit something that was only
borrowed (because we wouldn't have performed matching increfs).
@dvarrazzo dvarrazzo merged commit 9cf658e into psycopg:master May 20, 2018
@dvarrazzo
Copy link
Member

Sorry if it took longer than expected to merge, but the code had several shortcomings highlighted in the merge request that hadn't been addressed.

I'll look what I'll be realistically able to merge for 2.8 and try to release it in a not too distant future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
psycopg 2.8
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@asheshv @dvarrazzo @dpage