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.

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

feat: add libpq interface to change a password #818

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

dlax
Copy link
Contributor

@dlax dlax commented May 14, 2024

Another addition in libpq 17.


See https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=a7be2a6c262d5352756d909b29c419ea5e5fa1d9:

drivers built on top of libpq should expose this function and its
use should be generally encouraged over doing ALTER USER directly for
password changes.

@dlax dlax force-pushed the libpq-change-password branch 2 times, most recently from cef9667 to e25b7a1 Compare May 14, 2024 10:35
@dlax dlax marked this pull request as ready for review May 14, 2024 11:23
Copy link
Member

@dvarrazzo dvarrazzo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks pretty ok, thank you for you this feature too. Just left some comments regards the tests.

psycopg/psycopg/pq/pq_ctypes.py Outdated Show resolved Hide resolved
@pytest.fixture
def role(pgconn: PGconn) -> Iterator[tuple[bytes, bytes]]:
user, passwd = "ashesh", "psycopg2"
r = pgconn.exec_(f"CREATE USER {user} LOGIN PASSWORD '{passwd}'".encode())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we want to drop the role in a transaction and ignore an error there?

You can even use the high level psycopg connection and exception for that (try: ... except UndefinedObject: ...) The reason why in the tests/pq tests they are not very use is mostly for historical evolution (most of those tests were written before the Connection object existed).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand what you suggest. It seems to me that we cannot really use a transaction here because we need the new role available in other connections in the actual test (the one from pgconn fixture and another one created).


pgconn.change_password(user, b"psycopg")
conninfo[b"password"] = b"psycopg"
conn = pq.PGconn.connect(b" ".join(b"%s='%s'" % item for item in conninfo.items()))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is pretty good as integration test, maybe it can happen that someone's pg_hba might make this connection fail. Maybe let's merge the test but then be prepared to fix it if it turns out to be not generic enough to run.

@dvarrazzo dvarrazzo added this to the 3.2 milestone May 18, 2024
See https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=a7be2a6c262d5352756d909b29c419ea5e5fa1d9:
> drivers built on top of libpq should expose this function and its
> use should be generally encouraged over doing ALTER USER directly for
> password changes.

The test case assumes that the role connected to postgres has CREATEROLE
rights. If this is not true, the test is skipped.
@dlax dlax requested a review from dvarrazzo May 21, 2024 07:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants