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

Dissociate credentials from connection parameters #77

Open
chanmix51 opened this issue Sep 28, 2016 · 9 comments
Open

Dissociate credentials from connection parameters #77

chanmix51 opened this issue Sep 28, 2016 · 9 comments
Milestone

Comments

@chanmix51
Copy link
Member

chanmix51 commented Sep 28, 2016

It is often annoying to declare several sessions that share the same connection parameters with different users. They must declare the same session builder every time. What would be nice would be a list of profiles (login, passwords etc.) and a list of session configurations (host, db name, connection settings etc).

$pomm = new Pomm(
    [
        'session_1' =>
            [
                'dsn' => 'pgsql://host:5432/db_name',
                'pomm:default' => true,
                'class:session_builder' => '\Some\Class',
                'default_profile' => 'my_profile',
            ],
        'session_2' => …
    ],
    [
        'my_profile' => ['username' => 'unpriv_user', 'password' => 'p4ßw0rD'],
        'administrator' => ['username' => 'postgres', 'password' => 'p4ßw0rD'],
    ]
    );

$session = $pomm->getDefaultSession(); // Default session (1) with default profile
$session = $pomm['session_1']; // Session 1 with default profile
$session = $pomm->getSession('session_1'); // same as above
$session = $pomm->getSession('session_1', 'administrator'); // session 1 with administrator privileges.

Of course the old form of DSN would still be available and inline the default profile to use.

$pomm = new Pomm(
    [
        'session_1' =>
            [
                'dsn' => 'pgsql://user:p4sS@host:5432/db_name',
            ]
    ],
    [
        'administrator' => ['username' => 'postgres', 'password' => 'p4ßw0rD'],
    ]
);

$session = $pomm->getSession('session_1', 'administrator');

Any thoughts ?

@chanmix51 chanmix51 added this to the 2.1 milestone Sep 28, 2016
@stof
Copy link

stof commented Sep 28, 2016

The issue with this API is that getSession() returns the same instance each time you call it for a given name. Your proposal can lead to several behaviors:

  • ignoring the profile being requested in case we already have an existing session for this name, and return the existing session => WTF moment about not having admin access
  • using session+profile as identifier for session. This is much harder in case you use the Pomm instance as a service locator for the session, as you have to provide the same profile each time
  • drop the existing session and create a new one in case there is a profile mismatch => can lead to weird issues as the old session might still be used by other code, but the Pomm registry will not know it anymore

@chanmix51
Copy link
Member Author

Thank you for helping 👍

Imho, the good answer is the number 2. To ensure backward compatibility we need to keep the way the sessions used to be returned by the Pomm instance. In case of a profile enforcement, it must be explicit in the session call every time hence it must be cached the same way.

$pomm->getSession('my_session'); // create and return the session using default profile.
$pomm->getSession('my_session'); // return the same instance.
$pomm->getSession('my_session', 'administrator'); // creates a new instance with administrator profile.
$pomm->getSession('my_session'); // return the instance cached with default profile.
$pomm->getSession('my_session', 'administrator'); // returns the previously created session with administrator profile.
$pomm['my_session']; // return the cached session with default profile.

The idea behind this logic is to make developers not to bother about creating or caching sessions. They explicitly ask the service locator what they need. Whatever the session exists or not, it is returned if possible. This way, $pomm['my_session'] returns a predictable session using its default profile which is what you would generally expect.

There is a need for different kind of sessions in tests where administrator rights are required to build database objects or in CLI commands where the database user may not have the same rights granted on database objects than the default one used in the web application.

@webaaz
Copy link

webaaz commented Sep 28, 2016

👍

@sanpii
Copy link
Member

sanpii commented Sep 28, 2016

A simple ->grantTo($user, $password) is not enough?

@stof
Copy link

stof commented Sep 28, 2016

@sanpii mutating the existing session is a bad idea. It can impact other code already using this Session object (and even worse in case there is a running transaction, as disconnecting and reconnecting with a different user cannot keep the transaction)

@sanpii
Copy link
Member

sanpii commented Sep 28, 2016

@stof I thought cloning the session.

@stof
Copy link

stof commented Sep 29, 2016

@sanpii but then, you end up with a new Session now know by your Pomm registry.
Thus, cloning might be hard (what about all clients registered on the session, should they be forced to be cloneable, or should they be registered without being aware of the clone ?)

@stood
Copy link
Member

stood commented Oct 1, 2016

For me, the configuration for profiles will create confusion with application profiles.
I think each session equals a profile, and this is the application who will consume those sessions.
You only need to create and use variables in order to setup configuration's redundancy and dsn

@chanmix51
Copy link
Member Author

The idea is to be able in a test suite to

  1. Create structure & import fixtures
  2. Eventually launch CLI commands using a specific database user
  3. Launch the tests with an unprivileged database user

The database session must use the same configuration (session builder, session parameters etc.) but with a different user. Once the config file is parsed and variables expanded it is not possible for now to ask Pomm to do this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants