Share user sessions through the database by whisperity · Pull Request #1172 · Ericsson/codechecker

whisperity · 2017-11-23T14:46:00Z

Closes #1145.

Along with the existing storage in the server's memory, user authentication session tokens will also be saved to the configuration database, in a new table. This way, if multiple servers are used, the user can authenticate through either and still receive the same token as they had before.

When a session cookie is presented, the following steps are taken to verify it:

Check if the token exists in the server's memory. If so, previous procedures apply.
Check if the token exists in the database. If the user is authenticated properly and the token exists, we create a local session by reusing the token in the database.

Session expiry times in the configuration file apply to how tokens in the database also expire. Expired tokens are not reused, at the attempt of reusal, they are deleted from the DB too.

Explicit logout also removes tokens from database.

csordasmarton · 2017-12-07T14:20:42Z

@whisperity The migration script has been merged. Please resolve the conflicts to review you pull request.

gyorb · 2017-12-07T14:54:33Z

config/version.json

    "product_db_version": {
        "major" : "6",
-        "minor" : "0"
+        "minor" : "1"


These versions can be removed later alembic schema versions should be used.

I'm adding this file back but without changing the version inside. Removing these dicts tumbles down into edits required that should not be part of this or any of my other PRs. See #1204.

gyorb · 2017-12-07T15:19:34Z

libcodechecker/server/database/config_db_model.py

        self.is_group = is_group

+
+class Session(Base):


Wouldn't we mix the current database sessions with the user login sessions? I'm just thinking about a different naming here.

You import them with different names using from ... import ... as .... The user login session, as an object, is an internal thing to session_manager.py, and should not be imported by code outside of it.

csordasmarton · 2017-12-07T15:39:39Z

libcodechecker/libclient/product_helper.py

        if session_token:
-            headers = {'Cookie': session_manager.SESSION_COOKIE_NAME +
-                       "=" + session_token}
+            headers = {'Cookie': SESSION_COOKIE_NAME + '=' + session_token}


You use this line of code in multiple places. Can we create a helper function for it?

I don't think there exists a good place where this helper function could be put.

csordasmarton · 2017-12-07T15:44:04Z

libcodechecker/server/session_manager.py

@@ -0,0 +1,541 @@
+# -------------------------------------------------------------------------


Did you modified this file or just changed the path? Why github doesn't recognize that the path was changed?

The first commit split the file into a serverside and a clientside code. The second commit modified the serverside file.

csordasmarton · 2017-12-07T15:50:26Z

libcodechecker/server/session_manager.py

+                        records = transaction.query(SessionRecord). \
+                            filter(and_(SessionRecord.auth_string ==
+                                        session.persistent_hash,
+                                        SessionRecord.token == token)).all()


Can't we just call the delete operation on the query?

.query().filter().delete()

csordasmarton · 2018-01-18T13:27:56Z

libcodechecker/server/server.py

    try:
-        manager = session_manager.SessionManager(root_sha, force_auth)
+        manager = session_manager.SessionManager(
+            server_cfg_file,


I am getting the following error when starting CodeChecker with an empty workspace:

UnboundLocalError: local variable 'server_cfg_file' referenced before assignment

This is fixed. Damn you IDE for pasting code after an else: like it was meant to be in the else's statement body.

gyorb · 2018-01-18T14:39:20Z

config_db_migrate/versions/150800b30447_share_sessions_through_the_database.py

+
+
+def upgrade():
+    ### commands auto generated by Alembic - please adjust! ###


Please remove these generated comments if the migration script is ok.

gyorb · 2018-01-18T15:05:13Z

libcodechecker/server/session_manager.py

-import portalocker
-
 from libcodechecker.logger import get_logger
+<<<<<<< HEAD:libcodechecker/session_manager.py


Could you remove these git marks from the source?

Done these too.

gyorb · 2018-01-18T15:05:56Z

libcodechecker/server/session_manager.py

                                           in SessionManager.__valid_sessions
                                           if s.still_reusable()]
        self.__logins_since_prune = 0
+<<<<<<< HEAD:libcodechecker/session_manager.py


git marks here too.

gyorb · 2018-01-19T07:35:25Z

@whisperity do we have a better place to share information between the server and the client? The name of version.py does not reflect this.

whisperity · 2018-01-19T08:30:56Z

@gyorb We don't. I was thinking on extending the API to tell the client what is the cookie's name, but then the command line client would need to be rewritten to store for each individual server not only the token but the cookie name too, essentially slowly turning the CLI into a terminal webbrowser...

These are sort of global constants that need to be shared between the faces in a particular build.

I have an idea for making the build script generate the relevant files that contain the constants, kinda like how configure generates a header used in an autotools project. Could do this as a later patch.

whisperity added enhancement 🌟 WIP 💣 Work In Progress CLI 💻 Related to the command-line interface, such as the cmd, store, etc. commands database 🗄️ Issues related to the database schema. server 🖥️ labels Nov 23, 2017

whisperity added this to the release 6.3 milestone Nov 23, 2017

whisperity requested review from dkrupp and gyorb November 23, 2017 14:46

whisperity added RDY-OnHold 🛑 Patch reviewed and ready, but don't merge due to having to merge a dependent patch first. and removed WIP 💣 Work In Progress labels Nov 24, 2017

whisperity force-pushed the share-sessions-through-db branch from b15db87 to 2cedd78 Compare November 24, 2017 12:55

whisperity force-pushed the share-sessions-through-db branch from 2cedd78 to b2c9d16 Compare December 7, 2017 14:48

whisperity requested a review from csordasmarton December 7, 2017 14:48

whisperity removed the RDY-OnHold 🛑 Patch reviewed and ready, but don't merge due to having to merge a dependent patch first. label Dec 7, 2017

whisperity force-pushed the share-sessions-through-db branch from b2c9d16 to b23faef Compare December 7, 2017 14:56

gyorb reviewed Dec 7, 2017

View reviewed changes

whisperity force-pushed the share-sessions-through-db branch from b23faef to f9bc20c Compare December 7, 2017 15:50

csordasmarton reviewed Dec 7, 2017

View reviewed changes

whisperity force-pushed the share-sessions-through-db branch 2 times, most recently from 33ac223 to b250973 Compare December 8, 2017 10:29

gyorb modified the milestones: release 6.3, release 6.3.1 Dec 8, 2017

gyorb modified the milestones: release 6.3.1, release 6.4 Jan 5, 2018

whisperity mentioned this pull request Jan 9, 2018

Server config #1249

Merged

whisperity mentioned this pull request Jan 17, 2018

Login window instead of WWW-Authenticate #1301

Merged

whisperity force-pushed the share-sessions-through-db branch 2 times, most recently from 016ca0d to 60db95e Compare January 18, 2018 13:06

csordasmarton suggested changes Jan 18, 2018

View reviewed changes

whisperity force-pushed the share-sessions-through-db branch from 60db95e to a5cf659 Compare January 18, 2018 13:33

csordasmarton approved these changes Jan 18, 2018

View reviewed changes

gyorb suggested changes Jan 18, 2018

View reviewed changes

whisperity force-pushed the share-sessions-through-db branch from a5cf659 to e98959b Compare January 18, 2018 15:09

whisperity requested a review from gyorb January 18, 2018 15:09

whisperity added 2 commits January 18, 2018 16:12

Split session_manager into a server- and a clientside module

9a5224a

Store the users' authenticated sessions in the database too

aedcc67

whisperity force-pushed the share-sessions-through-db branch from e98959b to aedcc67 Compare January 18, 2018 15:14

gyorb approved these changes Jan 19, 2018

View reviewed changes

gyorb merged commit 51de24d into Ericsson:master Jan 23, 2018

whisperity deleted the share-sessions-through-db branch January 30, 2018 18:35

		@@ -0,0 +1,541 @@
		# -------------------------------------------------------------------------



		def upgrade():
		### commands auto generated by Alembic - please adjust! ###

Conversation

whisperity commented Nov 23, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

csordasmarton commented Dec 7, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gyorb commented Jan 19, 2018

Uh oh!

whisperity commented Jan 19, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

whisperity commented Nov 23, 2017 •

edited

Loading