Merge pull request #877 from uc-cdis/fix/populate-idp-for-user-info

(PXP-7539): Fix/populate idp for user info

fence/auth.py

@@ -60,27 +60,56 @@ def build_redirect_url(hostname, path):
     return redirect_base + path
 
 
-def login_user(request, username, provider):
-    user = query_for_user(session=current_session, username=username)
+def login_user(username, provider):
+    """
+    Login a user with the given username and provider. Set values in Flask
+    session to indicate the user being logged in. In addition, commit the user
+    and associated idp information to the db.
 
-    if not user:
+    Args:
+        username (str): specific username of user to be logged in
+        provider (str): specfic idp of user to be logged in
+
+    """
+
+    def set_flask_session_values(user):
+        """
+        Helper fuction to set user values in the session.
+
+        Args:
+            user (User): User object
+        """
+        flask.session["username"] = user.username
+        flask.session["provider"] = user.identity_provider.name
+        flask.session["user_id"] = str(user.id)
+        flask.g.user = user
+        flask.g.scopes = ["_all"]
+        flask.g.token = None
+
+    user = query_for_user(session=current_session, username=username)
+    if user:
+        #  This expression is relevant to those users who already have user and
+        #  idp info persisted to the database. We return early to avoid
+        #  unnecessarily re-saving that user and idp info.
+        if user.identity_provider and user.identity_provider.name == provider:
+            set_flask_session_values(user)
+            return
+    else:
         user = User(username=username)
-        idp = (
-            current_session.query(IdentityProvider)
-            .filter(IdentityProvider.name == provider)
-            .first()
-        )
-        if not idp:
-            idp = IdentityProvider(name=provider)
-        user.identity_provider = idp
-        current_session.add(user)
-        current_session.commit()
-    flask.session["username"] = username
-    flask.session["provider"] = provider
-    flask.session["user_id"] = str(user.id)
-    flask.g.user = user
-    flask.g.scopes = ["_all"]
-    flask.g.token = None
+
+    idp = (
+        current_session.query(IdentityProvider)
+        .filter(IdentityProvider.name == provider)
+        .first()
+    )
+    if not idp:
+        idp = IdentityProvider(name=provider)
+
+    user.identity_provider = idp
+    current_session.add(user)
+    current_session.commit()
+
+    set_flask_session_values(user)
 
 
 def logout(next_url, force_era_global_logout=False):
@@ -138,9 +167,7 @@ def decorator(f):
         @wraps(f)
         def wrapper(*args, **kwargs):
             if flask.session.get("username"):
-                login_user(
-                    flask.request, flask.session["username"], flask.session["provider"]
-                )
+                login_user(flask.session["username"], flask.session["provider"])
                 return f(*args, **kwargs)
 
             eppn = None
@@ -169,7 +196,7 @@ def wrapper(*args, **kwargs):
                 username = eppn.split("!")[-1]
                 flask.session["username"] = username
                 flask.session["provider"] = IdentityProvider.itrust
-                login_user(flask.request, username, flask.session["provider"])
+                login_user(username, flask.session["provider"])
                 return f(*args, **kwargs)
             else:
                 raise Unauthorized("Please login")
@@ -181,7 +208,7 @@ def wrapper(*args, **kwargs):
 
 def handle_login(scope):
     if flask.session.get("username"):
-        login_user(flask.request, flask.session["username"], flask.session["provider"])
+        login_user(flask.session["username"], flask.session["provider"])
 
     eppn = flask.request.headers.get(config["SHIBBOLETH_HEADER"])
 
@@ -196,7 +223,7 @@ def handle_login(scope):
         username = eppn.split("!")[-1]
         flask.session["username"] = username
         flask.session["provider"] = IdentityProvider.itrust
-        login_user(flask.request, username, flask.session["provider"])
+        login_user(username, flask.session["provider"])
     else:
         raise Unauthorized("Please login")
 

fence/blueprints/login/base.py

@@ -110,7 +110,7 @@ def _login(username, idp_name):
     Login user with given username, then redirect if session has a saved
     redirect.
     """
-    login_user(flask.request, username, idp_name)
+    login_user(username, idp_name)
     if flask.session.get("redirect"):
         return flask.redirect(flask.session.get("redirect"))
     return flask.jsonify({"username": username})

fence/blueprints/login/fence_login.py

@@ -81,7 +81,7 @@ def get(self):
             tokens["id_token"], aud={"openid"}, purpose="id", attempt_refresh=True
         )
         username = id_token_claims["context"]["user"]["name"]
-        login_user(flask.request, username, IdentityProvider.fence)
+        login_user(username, IdentityProvider.fence)
 
         if "redirect" in flask.session:
             return flask.redirect(flask.session.get("redirect"))

fence/blueprints/login/shib.py

@@ -73,7 +73,7 @@ def get(self):
         idp = IdentityProvider.itrust
         if entityID:
             idp = entityID
-        login_user(flask.request, username, idp)
+        login_user(username, idp)
 
         if flask.session.get("redirect"):
             return flask.redirect(flask.session.get("redirect"))

tests/login/test_login_user.py

@@ -17,7 +17,34 @@ def test_login_user_already_in_db(db_session):
     db_session.commit()
     user_id = str(test_user.id)
 
-    login_user(flask.request, email, provider)
+    login_user(email, provider)
+
+    assert test_user.identity_provider.name == provider
+    assert flask.session["username"] == email
+    assert flask.session["provider"] == provider
+    assert flask.session["user_id"] == user_id
+    assert flask.g.user == test_user
+
+
+def test_login_user_with_idp_already_in_db(db_session):
+    """
+    Test that if a user is already in the database, has identity_provider
+    configured, and logs in, the session will contain the user's information.
+    """
+    email = "testuser@gmail.com"
+    provider = "Test Provider"
+
+    test_user = User(username=email, is_admin=False)
+    test_idp = IdentityProvider(name=provider)
+    test_user.identity_provider = test_idp
+
+    db_session.add(test_user)
+    db_session.commit()
+    user_id = str(test_user.id)
+
+    login_user(email, provider)
+
+    assert test_user.identity_provider.name == provider
     assert flask.session["username"] == email
     assert flask.session["provider"] == provider
     assert flask.session["user_id"] == user_id
@@ -32,9 +59,11 @@ def test_login_new_user(db_session):
     email = "testuser@gmail.com"
     provider = "Test Provider"
 
-    login_user(flask.request, email, provider)
+    login_user(email, provider)
+
     test_user = db_session.query(User).filter(User.username == email.lower()).first()
 
+    assert test_user.identity_provider.name == provider
     assert flask.session["username"] == email
     assert flask.session["provider"] == provider
     assert flask.session["user_id"] == str(test_user.id)