Skip to content
This repository has been archived by the owner on Jan 15, 2024. It is now read-only.

Refactor the auth to make it connection wise #230

Merged
merged 2 commits into from
Nov 16, 2013
Merged

Conversation

arthurnn
Copy link
Contributor

@arthurnn arthurnn commented Nov 9, 2013

Problem

login/logout were per node, as we have a connection pool, we can have multiple connections per node, which will break login/logout as the connections rotate depending the order of checkout/checkin

Solution

We need to make auth methods connection wise, so we can authenticate a specific connection after getting it from the pool, and before sending any command thought it.

[fixes #212]
[related mongoid/mongoid#3382]

@durran need your feedback on this one.
(all tests should be green, including the MongoHQ ones)

[fixes #212]
[related mongoid/mongoid#3382]
@arthurnn arthurnn mentioned this pull request Nov 9, 2013
@@ -259,6 +252,7 @@ def initialize(address, options = {})
@latency = nil
@primary = nil
@secondary = nil
@credentials = {}

Choose a reason for hiding this comment

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

Consider using Set to store credential sets on the client instance and to keep track of which credentials have been applied to each socket instance in the pool. This would allow you to just do a quick set difference between what's been applied to the socket and the authentications that have been applied to the client.

In most of the MongoDB drivers we take this approach and in while synchronizing in Pool#checkout we validate that each socket has applied the correct logins/logouts with a simple set comparison:

https://github.com/mongodb/mongo-ruby-driver/blob/c947c93c9b2d131dbb3162ea61cc743e06e82622/lib/mongo/connection/pool.rb#L314-L335

This also has the added benefit of making your DB#logout method very simple. Just remove the credential from the client set and let the pool issue the logout when the socket gets checked out.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe having a Hash with keys as database would make easier to find when a user/password has changed for the same database. Also we would not have 2+ credentials for the same database, AFIAK you only can have one credential per database right?

Choose a reason for hiding this comment

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

Yeah, only one credential per database. It actually doesn't matter if its a different user or not. In the Ruby driver we require the first user to manually logout of that database before another user can login on the same client instance.

I suppose you could go the route of trying to detect if the user is different and handle the logout for them, but in our case we decided that would introduce unnecessary complication into the auth flow. We wanted to keep it simple and explicit, with little or no magic happening behind the scenes.

Authenticable methods now run on a connection not on a node, so logout must write
directly to the socket
@coveralls
Copy link

Coverage Status

Coverage increased (+0.17%) when pulling 413c858 on auth_connection_pool into cdf3a26 on master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.17%) when pulling 413c858 on auth_connection_pool into cdf3a26 on master.

arthurnn added a commit that referenced this pull request Nov 16, 2013
Refactor the auth to make it connection wise
@arthurnn arthurnn merged commit 0f0cbcc into master Nov 16, 2013
@arthurnn arthurnn deleted the auth_connection_pool branch November 16, 2013 22:30
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Heroku Auth Problem
4 participants