#2343 Support for LDAP groups
Merged 2 years ago by praiskup. Opened 2 years ago by frostyx.
copr/ frostyx/copr ldap-groups  into  main

@@ -117,6 +117,7 @@ 

  BuildRequires: python3-sphinxcontrib-httpdomain

  BuildRequires: python3-whoosh

  BuildRequires: python3-wtforms >= 2.2.1

+ BuildRequires: python3-ldap

  BuildRequires: python3-yaml

  BuildRequires: redis

  BuildRequires: modulemd-tools >= 0.6
@@ -175,6 +176,7 @@ 

  Requires: python3-templated-dictionary

  Requires: python3-wtforms >= 2.2.1

  Requires: python3-zmq

+ Requires: python3-ldap

  Requires: xstatic-bootstrap-scss-common

  Requires: xstatic-datatables-common

  Requires: js-jquery-ui

@@ -0,0 +1,345 @@ 

+ """

+ Authentication-related code for communication with FAS, Kerberos, LDAP, etc.

+ """

+ 

+ import time

+ from urllib.parse import urlparse

+ import flask

+ import ldap

+ from openid_teams.teams import TeamsRequest

+ from coprs import oid

+ from coprs import app

+ from coprs.exceptions import CoprHttpException, AccessRestricted

+ from coprs.logic.users_logic import UsersLogic

+ 

+ 

+ class UserAuth:

+     """

+     Facade for choosing the correct authentication mechanism (FAS, Kerberos),

+     and interacting with it. All decision making based on

+     `app.config["FAS_LOGIN"]` and `app.config["KRB5_LOGIN"]` should be

+     encapsulated within this class.

+     """

+ 

+     @staticmethod

+     def logout():

+         """

+         Log out the current user

+         """

+         if flask.g.user:

+             app.logger.info("User '%s' logging out", flask.g.user.name)

+ 

+         FedoraAccounts.logout()

+         Kerberos.logout()

+ 

+         flask.flash("You were signed out")

+         return flask.redirect(oid.get_next_url())

+ 

+     @staticmethod

+     def current_username():

+         """

+         Is a user logged-in? Return their username

+         """

+         return FedoraAccounts.username() or Kerberos.username()

+ 

+     @staticmethod

+     def user_object(resp=None, username=None):

+         """

+         Create a `models.User` object based on the input parameters

+         """

+         if app.config["FAS_LOGIN"] and resp:

+             return FedoraAccounts.user_from_response(resp)

+ 

+         if username and app.config["KRB5_LOGIN"]:

+             return Kerberos.user_from_username(username)

+ 

+         raise CoprHttpException("No auth method available")

+ 

+ 

+ class GroupAuth:

+     """

+     Facade for choosing the correct user group authority (FAS, LDAP),

+     and interacting with it. All decision making based on

+     `app.config["FAS_LOGIN"]` and `app.config["KRB5_LOGIN"]` should be

+     encapsulated within this class.

+     """

+ 

+     @classmethod

+     def groups(cls, resp=None, username=None):

+         """

+         Return a `dict` that can be assigned to `models.User.openid_groups`

+         """

+         names = cls.group_names(resp=resp, username=username)

+         return {"fas_groups": names}

+ 

+     @staticmethod

+     def group_names(resp=None, username=None):

+         """

+         Return a list of group names that a user belongs to

+         """

+         # Fedora user via OpenID

+         if resp:

+             return OpenIDGroups.group_names(resp)

+ 

+         # Fedora user via Kerberos

+         if app.config["FAS_LOGIN"] and username:

+             return None

+ 

+         keys = ["LDAP_URL", "LDAP_SEARCH_STRING"]

+         if username and all(app.config[k] for k in keys):

+             return LDAPGroups.group_names(username)

+ 

+         raise CoprHttpException("Nowhere to get user groups from")

+ 

+ 

+ class FedoraAccounts:

+     """

+     Authentication via user accounts from

+     https://accounts.fedoraproject.org

+     """

+ 

+     @classmethod

+     def username(cls):

+         """

+         Is a user logged-in? Return their username

+         """

+         if "openid" in flask.session:

+             return cls.fed_raw_name(flask.session["openid"])

+         return None

+ 

+     @staticmethod

+     def login():

+         """

+         If not already logged-in, perform a log-in request

+         """

+         if flask.g.user is not None:

+             return flask.redirect(oid.get_next_url())

+ 

+         # If the login is successful, we are redirected to function decorated

+         # with the `@oid.after_login`

+         team_req = TeamsRequest(["_FAS_ALL_GROUPS_"])

+         return oid.try_login(app.config["OPENID_PROVIDER_URL"],

+                             ask_for=["email", "timezone"],

+                             extensions=[team_req])

+ 

+     @staticmethod

+     def logout():

+         """

+         Log out the current user

+         """

+         flask.session.pop("openid", None)

+ 

+     @staticmethod

+     def is_user_allowed(username):

+         """

+         Is this user allowed to log in?

+         """

+         if not username:

+             return False

+         if not app.config["USE_ALLOWED_USERS"]:

+             return True

+         return username in app.config["ALLOWED_USERS"]

+ 

+     @staticmethod

+     def fed_raw_name(oidname):

+         """

+         Convert the full `oidname` to username

+         """

+         oidname_parse = urlparse(oidname)

+         if not oidname_parse.netloc:

+             return oidname

+         config_parse = urlparse(app.config["OPENID_PROVIDER_URL"])

+         return oidname_parse.netloc.replace(".{0}".format(config_parse.netloc), "")

+ 

+     @classmethod

+     def user_from_response(cls, resp):

+         """

+         Create a `models.User` object from FAS response

+         """

+         username = cls.fed_raw_name(resp.identity_url)

+         user = UsersLogic.get(username).first()

+ 

+         # Create if not created already

+         if not user:

+             app.logger.info("First login for user '%s', "

+                             "creating a database record", username)

+             user = UsersLogic.create_user_wrapper(username, resp.email, resp.timezone)

+ 

+         # Update user attributes from FAS

+         user.mail = resp.email

+         user.timezone = resp.timezone

+         return user

+ 

+ 

+ class Kerberos:

+     """

+     Authentication via Kerberos / GSSAPI

+     """

+ 

+     @staticmethod

+     def username():

+         """

+         Is a user logged-in? Return their username

+         """

+         if "krb5_login" in flask.session:

+             return flask.session["krb5_login"]

+         return None

+ 

+     @classmethod

+     def login(cls):

+         """

+         If not already logged-in, perform a log-in request

+         """

+         return cls._krb5_login_redirect(next_url=oid.get_next_url())

+ 

+     @staticmethod

+     def logout():

+         """

+         Log out the current user

+         """

+         flask.session.pop("krb5_login", None)

+ 

+     @staticmethod

+     def user_from_username(username):

+         """

+         Create a `models.User` object from Kerberos username

+         """

+         user = UsersLogic.get(username).first()

+         if user:

+             return user

+ 

+         # We can not create a new user now because we don't have the necessary

+         # e-mail and groups info.

+         if app.config["FAS_LOGIN"] is True:

+             raise AccessRestricted(

+                 "Valid GSSAPI authentication supplied for user '{}', but this "

+                 "user doesn't exist in the Copr build system.  Please log-in "

+                 "using the web-UI (without GSSAPI) first.".format(username)

+             )

+ 

+         # Create a new user object

+         krb_config = app.config['KRB5_LOGIN']

+         email = username + "@" + krb_config['email_domain']

+         return UsersLogic.create_user_wrapper(username, email)

+ 

+     @staticmethod

+     def _krb5_login_redirect(next_url=None):

+         if app.config['KRB5_LOGIN']:

+             # Pick the first one for now.

+             return flask.redirect(flask.url_for("apiv3_ns.gssapi_login",

+                                                 next=next_url))

+         flask.flash("Unable to pick krb5 login page", "error")

+         return flask.redirect(flask.url_for("coprs_ns.coprs_show"))

+ 

+ 

+ class OpenIDGroups:

+     """

+     User groups from FAS (and OpenID in general)

+     """

+ 

+     @staticmethod

+     def group_names(resp):

+         """

+         Return a list of group names (that a user belongs to) from FAS response

+         """

+         if "lp" in resp.extensions:

+             # name space for the teams extension

+             team_resp = resp.extensions['lp']

+             return {"fas_groups": team_resp.teams}

+         return None

+ 

+ 

+ class LDAPGroups:

+     """

+     User groups from LDAP

+     """

+ 

+     @staticmethod

+     def group_names(username):

+         """

+         Return a list of group names that a user belongs to

+         """

+         ldap_client = LDAP(app.config["LDAP_URL"],

+                            app.config["LDAP_SEARCH_STRING"])

+         groups = []

+         for group in ldap_client.get_user_groups(username):

+             group = group.decode("utf-8")

+             attrs = dict([tuple(x.split("=")) for x in group.split(",")])

+             groups.append(attrs["cn"])

+         return groups

+ 

+ 

+ class LDAP:

+     """

+     High-level facade for interacting with LDAP server

+     """

+ 

+     def __init__(self, url, search_string):

+         self.url = url

+         self.search_string = search_string

+ 

+     def send_request(self, ou, attrs, ffilter):

+         """

+         Send a /safe/ request to a LDAP server

+         """

+         return self._send_request_repeatedly(ou, attrs, ffilter)

+ 

+     def _send_request_repeatedly(self, ou, attrs, ffilter):

+         i = 0

+         while True:

+             i += 1

+             try:

+                 return self._send_request(ou, attrs, ffilter)

+             except ldap.SERVER_DOWN as ex:

+                 print(str(ex))

+                 time.sleep(0.5)

+ 

+     def _send_request(self, ou, attrs, ffilter):

+         """

+         Send a single request to a LDAP server

+         """

+         try:

+             connect = ldap.initialize(self.url)

+             return connect.search_s(ou, ldap.SCOPE_ONELEVEL,

+                                     ffilter, attrs)

+         except ldap.SERVER_DOWN as ex:

+             msg = ex.args[0]["desc"]

+             raise CoprHttpException(msg) from ex

+ 

+     def query_one(self, attrs, filters=None):

+         """

+         Query one object from LDAP

+         """

+         ffilter = self._build_filter(filters)

+         return self.send_request(self.search_string, attrs, ffilter)[0]

+ 

+     def get_user(self, username):

+         """

+         Return an LDAP user

+         """

+         attrs = [

+             "cn",

+             "uid",

+             "memberOf",

+             "mail",

+         ]

+         filters = {

+             "objectclass": "*",

+             "uid": username,

+         }

+         return self.query_one(attrs, filters)

+ 

+     def get_user_groups(self, username):

+         """

+         Return a list of groups that a user belongs to

+         """

+         user = self.get_user(username)

+         if not user:

+             return None

+         return user[1]["memberOf"]

+ 

+     def _build_filter(self, filters):

+         # pylint: disable=no-self-use

+         filters = filters or {"objectclass": "*"}

+         ffilter = ["({0}={1})".format(k, v) for k, v in filters.items()]

+         return "(&{0})".format("".join(ffilter))

@@ -149,6 +149,12 @@ 

  

      HIDE_IMPORT_LOG_AFTER_DAYS = 14

  

+     # LDAP server URL, e.g. ldap://ldap.foo.company.com

+     LDAP_URL = None

+ 

+     # e.g. ou=users,dc=company,dc=com

+     LDAP_SEARCH_STRING = None

+ 

  

  class ProductionConfig(Config):

      DEBUG = False

@@ -53,7 +53,7 @@ 

      else:

          if config['FAS_LOGIN']:

              menu.append({

-                 'link': flask.url_for('misc.login'),

+                 'link': flask.url_for('misc.oid_login'),

                  'desc': 'log in',

              })

  

@@ -62,7 +62,11 @@ 

      # is this user admin of the system?

      admin = db.Column(db.Boolean, default=False)

  

-     # list of groups as retrieved from openid

+     # List of groups as retrieved from openid.

+     # The name `openid_groups` is misleading because we now support more

+     # group authorities (e.g. LDAP) than just OpenID. Whatever group authority

+     # is used, the `openid_groups` variable needs to be in the  following format

+     #     openid_groups = {"fas_groups": ["foo", "bar", "baz"]}

      openid_groups = db.Column(JSONEncodedDict)

  

  

@@ -302,7 +302,7 @@ 

          <span class="badge">{{ g.user.coprs_count }}</span>

          My projects

        </a>

-     {% if config.FAS_LOGIN %}

+     {% if config.FAS_LOGIN or config.LDAP_URL %}

        <a href="{{url_for('groups_ns.list_user_groups') }}" class="list-group-item">

          <span class="badge"> {{ user.user_groups|length }} </span>

          My groups

@@ -7,7 +7,7 @@ 

  from coprs.views.apiv3_ns import apiv3_ns

  from coprs.exceptions import AccessRestricted

  from coprs.views.misc import api_login_required

- from coprs.logic.users_logic import UsersLogic

+ from coprs.auth import UserAuth, GroupAuth

  

  

  def auth_check_response():
@@ -79,8 +79,6 @@ 

      if flask.g.user is not None:

          return gssapi_login_action()

  

-     krb_config = app.config['KRB5_LOGIN']

- 

      if app.config["DEBUG"] and 'TEST_REMOTE_USER' in os.environ:

          # For local testing (without krb5 keytab and other configuration)

          flask.request.environ['REMOTE_USER'] = os.environ['TEST_REMOTE_USER']
@@ -102,39 +100,35 @@ 

      )

  

      if krb_login:

-         flask.g.user = krb_login.user

-         flask.session['krb5_login'] = krb_login.user.name

-         app.logger.info(

-             "%s '%s' logged in",

-             "Admin" if krb_login.user.admin else "User",

-             krb_login.user.name

-         )

-         flask.flash("Welcome, {0}".format(flask.g.user.name), "success")

-         return gssapi_login_action()

- 

-     # We need to create row in 'krb5_login' table

-     user = models.User.query.filter(models.User.username == username).first()

-     if not user:

-         if app.config["FAS_LOGIN"] is True:

-             # We can not create a new user now because we wouldn't get the necessary

-             # e-mail and groups info.

-             return auth_403(

-                 "Valid GSSAPI authentication supplied for user '{}', but this "

-                 "user doesn't exist in the Copr build system.  Please log-in "

-                 "using the web-UI (without GSSAPI) first.".format(username)

-             )

-         # Create the user in the database

-         email = username + "@" + krb_config['email_domain']

-         user = UsersLogic.create_user_wrapper(username, email)

-         db.session.add(user)

- 

-     krb_login = models.Krb5Login(user=user, primary=username)

-     db.session.add(krb_login)

+         user = krb_login.user

+ 

+     else:

+         # First GSSAPI login for this user

+         try:

+             user = UserAuth.user_object(username=username)

+         except AccessRestricted as ex:

+             return auth_403(str(ex))

+ 

+         # We need to create row in 'krb5_login' table

+         app.logger.info("First krb5 login for user '%s', "

+                         "creating a database record", username)

+         krb_login = models.Krb5Login(user=user, primary=username)

+         db.session.add(krb_login)

+         db.session.commit()

+ 

+     # Groups could have changed since the last log-in, update our DB

+     groups = GroupAuth.groups(username=username)

+     user.openid_groups = groups

+ 

+     db.session.add(user)

      db.session.commit()

  

-     app.logger.info("First krb5 login for user '%s', "

-                     "creating a database record", username)

-     flask.flash("Welcome, {0}".format(user.name), "success")

      flask.g.user = user

      flask.session['krb5_login'] = user.name

+     app.logger.info(

+         "%s '%s' logged in",

+         "Admin" if user.admin else "User",

+         user.name

+     )

+     flask.flash("Welcome, {0}".format(flask.g.user.name), "success")

      return gssapi_login_action()

@@ -85,8 +85,8 @@ 

  @groups_ns.route("/list/my")

  @login_required

  def list_user_groups():

-     if not app.config['FAS_LOGIN']:

-         raise ObjectNotFound("Logging via Fedora Accounts not enabled")

+     if not (app.config['FAS_LOGIN'] or app.config['LDAP_URL']):

+         raise ObjectNotFound("Fedora Accounts or LDAP groups not enabled")

  

      teams = flask.g.user.user_teams

      active_map = {

@@ -2,13 +2,10 @@ 

  import datetime

  import functools

  from functools import wraps

- from urllib.parse import urlparse

  import flask

  

  from flask import send_file

  

- from openid_teams.teams import TeamsRequest

- 

  from copr_common.enums import RoleEnum

  from coprs import app

  from coprs import db
@@ -19,14 +16,7 @@ 

  from coprs.logic.users_logic import UsersLogic

  from coprs.exceptions import ObjectNotFound

  from coprs.measure import checkpoint_start

- 

- 

- def fed_raw_name(oidname):

-     oidname_parse = urlparse(oidname)

-     if not oidname_parse.netloc:

-         return oidname

-     config_parse = urlparse(app.config["OPENID_PROVIDER_URL"])

-     return oidname_parse.netloc.replace(".{0}".format(config_parse.netloc), "")

+ from coprs.auth import FedoraAccounts, UserAuth, GroupAuth

  

  

  @app.before_request
@@ -42,11 +32,8 @@ 

      # https://github.com/PyCQA/pylint/issues/3793

      # pylint: disable=assigning-non-slot

      flask.g.user = username = None

-     if "openid" in flask.session:

-         username = fed_raw_name(flask.session["openid"])

-     elif "krb5_login" in flask.session:

-         username = flask.session["krb5_login"]

  

+     username = UserAuth.current_username()

      if username:

          flask.g.user = models.User.query.filter(

              models.User.username == username).first()
@@ -93,76 +80,45 @@ 

  @misc.route("/login/", methods=["GET"])

  @workaround_ipsilon_email_login_bug_handler

  @oid.loginhandler

- def login():

-     if not app.config['FAS_LOGIN']:

-         if app.config['KRB5_LOGIN']:

-             return krb5_login_redirect(next=oid.get_next_url())

-         flask.flash("No auth method available", "error")

-         return flask.redirect(flask.url_for("coprs_ns.coprs_show"))

- 

-     if flask.g.user is not None:

-         return flask.redirect(oid.get_next_url())

-     else:

-         # a bit of magic

-         team_req = TeamsRequest(["_FAS_ALL_GROUPS_"])

-         return oid.try_login(app.config["OPENID_PROVIDER_URL"],

-                              ask_for=["email", "timezone"],

-                              extensions=[team_req])

+ def oid_login():

+     """

+     Entry-point for OpenID login

+     """

+     # After a successful FAS login, we are redirected to the `@oid.after_login`

+     # function

+     return FedoraAccounts.login()

  

  

  @oid.after_login

  def create_or_login(resp):

      flask.session["openid"] = resp.identity_url

-     fasusername = fed_raw_name(resp.identity_url)

- 

-     # kidding me.. or not

-     if fasusername and (

-             (

-                 app.config["USE_ALLOWED_USERS"] and

-                 fasusername in app.config["ALLOWED_USERS"]

-             ) or not app.config["USE_ALLOWED_USERS"]):

+     fasusername = FedoraAccounts.fed_raw_name(resp.identity_url)

  

-         username = fed_raw_name(resp.identity_url)

-         user = models.User.query.filter(

-             models.User.username == username).first()

-         if not user:  # create if not created already

-             app.logger.info("First login for user '%s', "

-                             "creating a database record", username)

-             user = UsersLogic.create_user_wrapper(username, resp.email,

-                                                   resp.timezone)

-         else:

-             user.mail = resp.email

-             user.timezone = resp.timezone

-         if "lp" in resp.extensions:

-             team_resp = resp.extensions['lp']  # name space for the teams extension

-             user.openid_groups = {"fas_groups": team_resp.teams}

- 

-         db.session.add(user)

-         db.session.commit()

-         flask.flash(u"Welcome, {0}".format(user.name), "success")

-         flask.g.user = user

- 

-         app.logger.info("%s '%s' logged in",

-                         "Admin" if user.admin else "User",

-                         user.name)

- 

-         if flask.request.url_root == oid.get_next_url():

-             return flask.redirect(flask.url_for("coprs_ns.coprs_by_user",

-                                                 username=user.name))

-         return flask.redirect(oid.get_next_url())

-     else:

+     if not FedoraAccounts.is_user_allowed(fasusername):

          flask.flash("User '{0}' is not allowed".format(fasusername))

          return flask.redirect(oid.get_next_url())

  

+     user = UserAuth.user_object(resp=resp)

+     user.openid_groups = GroupAuth.groups(resp=resp)

+ 

+     db.session.add(user)

+     db.session.commit()

+     flask.flash(u"Welcome, {0}".format(user.name), "success")

+     flask.g.user = user

+ 

+     app.logger.info("%s '%s' logged in",

+                     "Admin" if user.admin else "User",

+                     user.name)

+ 

+     if flask.request.url_root == oid.get_next_url():

+         return flask.redirect(flask.url_for("coprs_ns.coprs_by_user",

+                                             username=user.name))

+     return flask.redirect(oid.get_next_url())

+ 

  

  @misc.route("/logout/")

  def logout():

-     if flask.g.user:

-         app.logger.info("User '%s' logging out", flask.g.user.name)

-     flask.session.pop("openid", None)

-     flask.session.pop("krb5_login", None)

-     flask.flash(u"You were signed out")

-     return flask.redirect(oid.get_next_url())

+     return UserAuth.logout()

  

  

  def api_login_required(f):
@@ -204,21 +160,12 @@ 

      return decorated_function

  

  

- def krb5_login_redirect(next=None):

-     if app.config['KRB5_LOGIN']:

-         # Pick the first one for now.

-         return flask.redirect(flask.url_for("apiv3_ns.krb5_login",

-                                             next=next))

-     flask.flash("Unable to pick krb5 login page", "error")

-     return flask.redirect(flask.url_for("coprs_ns.coprs_show"))

- 

- 

  def login_required(role=RoleEnum("user")):

      def view_wrapper(f):

          @functools.wraps(f)

          def decorated_function(*args, **kwargs):

              if flask.g.user is None:

-                 return flask.redirect(flask.url_for("misc.login",

+                 return flask.redirect(flask.url_for("misc.oid_login",

                                                      next=flask.request.url))

  

              if role == RoleEnum("admin") and not flask.g.user.admin:

@@ -0,0 +1,32 @@ 

+ # pylint: disable=no-self-use

+ 

+ from unittest import mock

+ from tests.coprs_test_case import CoprsTestCase

+ from coprs import app

+ from coprs.auth import GroupAuth

+ 

+ 

+ class TestGroupAuth(CoprsTestCase):

+ 

+     @mock.patch("coprs.auth.LDAP.get_user_groups")

+     def test_group_names_ldap(self, get_user_groups):

+         """

+         Test that we can parse LDAP response containing user groups and return

+         just their names

+         """

+ 

+         app.config["FAS_LOGIN"] = False

+         app.config["LDAP_URL"] = "not-important"

+         app.config["LDAP_SEARCH_STRING"] = "not-important"

+ 

+         # We expect `LDAP.get_user_groups` to return something like this.

+         # Some internal values were redacted but otherwise it's a copy-pasted

+         # response

+         get_user_groups.return_value = [

+             b'cn=group1,ou=foo,dc=company,dc=com',

+             b'cn=group2,ou=bar,dc=company,dc=com',

+             b'cn=another-group,ou=baz,ou=qux,dc=company,dc=com',

+             b'cn=another-group-2,ou=foo,ou=bar,dc=company,dc=com'

+         ]

+         names = GroupAuth.group_names(username="user1")

+         assert names == ["group1", "group2", "another-group", "another-group-2"]

@@ -1,5 +1,5 @@ 

  from coprs import app

- from coprs.views.misc import fed_raw_name

+ from coprs.auth import FedoraAccounts

  from tests.coprs_test_case import CoprsTestCase

  

  
@@ -11,11 +11,13 @@ 

          ]

          for provider in providers:

              app.config["OPENID_PROVIDER_URL"] = provider

-             assert fed_raw_name("https://someuser.id.fedoraproject.org/") == "someuser"

+             fullname = "https://someuser.id.fedoraproject.org/"

+             assert FedoraAccounts.fed_raw_name(fullname) == "someuser"

  

      def test_fed_raw_name_scheme(self):

          app.config["OPENID_PROVIDER_URL"] = "foo://id.fedoraproject.org/"

-         assert fed_raw_name("bar://someuser.id.fedoraproject.org/") == "someuser"

+         fullname = "bar://someuser.id.fedoraproject.org/"

+         assert FedoraAccounts.fed_raw_name(fullname) == "someuser"

  

      def test_fed_raw_name_without_oid_url(self):

-         assert fed_raw_name("someuser") == "someuser"

+         assert FedoraAccounts.fed_raw_name("someuser") == "someuser"

I am working on the LDAP groups support for the internal Copr.

Metadata Update from @frostyx:
- Pull-request tagged with: wip

2 years ago

Build failed. More information on how to proceed and troubleshoot errors available at https://fedoraproject.org/wiki/Zuul-based-ci

In the first commit, I attempted to refactor our auth code. My motivation was:

  • The auth code is fairly complex and I wanted to put all the decision-making about what auth service should be used (We have FAS user with FAS groups; Kerberos with FAS groups where it matters if it is the first login or not; Kerberos with LDAP groups; and possibly more combinations in the future)
  • I dislike having auth code in misc.py
  • There are similarities between all the FAS and Kerberos code and I wanted to unify them

I am thinking of throwing away the first commit (or some of its parts) because I wasn't able to do it in a way that I would be satisfied with.

However, the second commit does what we want and can be easily used with or without the refactoring ^^. It needs to be a little polished and tested though.

What do you think? Is this an expected solution for interacting with LDAP for you, or would you expect some magic with mod_auth?

I think this deserves refactoring, thanks. Overall it looks good, I'd just prefer a little bit less-RH centric solution.

I still think that LDAP is a bit temporary solution, and we should add IPA support in the future.

I'd prefer to keep these arguments and the LDAP url in configuration.

2 new commits added

  • add groups for kerberos
  • refactor auth code
2 years ago

Build failed. More information on how to proceed and troubleshoot errors available at https://fedoraproject.org/wiki/Zuul-based-ci

rebased onto 303aa219e6a9907a28e39990c6328f0ef692a3c5

2 years ago

Build failed. More information on how to proceed and troubleshoot errors available at https://fedoraproject.org/wiki/Zuul-based-ci

2 new commits added

  • frontend: support LDAP groups for Kerberos users
  • frontend: refactor auth code
2 years ago

Merge Failed.

This change or one of its cross-repo dependencies was unable to be automatically merged with the current state of its repository. Please rebase the change and upload a new patchset.
Warning:
Error merging pagure.io/copr/copr for 2343,58da8c7e3e08ad36149e815d00030d126c61cf03

rebased onto 0c5c6de

2 years ago

Merge Failed.

This change or one of its cross-repo dependencies was unable to be automatically merged with the current state of its repository. Please rebase the change and upload a new patchset.
Warning:
Error merging pagure.io/copr/copr for 2343,ba56dd5

Metadata Update from @frostyx:
- Pull-request untagged with: wip

2 years ago

Can you just document what krb_login != None means?

Does fedora have LDAP? Could it (in the future) if not? That would make our logic much easier...

I somewhat can't understand this check ... we ar in the kerberos logic, and asking for fas_login? I'd probably setup a meeting and discuss if you don't mind.

1 new commit added

  • frontend: encapsulate all auth-mechanism decision making at one place
2 years ago

Build succeeded.

3 new commits added

  • frontend: encapsulate all auth-mechanism decision making at one place
  • frontend: support LDAP groups for Kerberos users
  • frontend: refactor auth code
2 years ago

Build succeeded.

I made some changes towards the idea that we discussed with @praiskup privately.

It would be nice to document that this leads to @oid.after_login ... so it is trivial to follow the flow; could we move that method to auth.py as well?

Why not to move those two statements as well? A note like "oid.get_next_url() is just correctly handling the HTTP_REFERRER, ... so we use it even though the particular Copr instance doesn't support FAS/OpenID"

This is a bit ugly name :-( would you mind dumping somewhere that this is not necessarily openid anymore, but can be LDAP?

Can we fix the groups() method so this additional 'is not None' check isn't needed?

This still feels a bit weird. It does some checks, e.g. for FAS config existence ... but we have to do the check below, too ....
This behaves differently for Red Hat Copr and Fedora Copr, could this be responsible for calling GroupAuth.groups()?

This FAS_LOGIN check e.g. looks weird. Check for just resp would sound more logical (LDAP could work for FAS systems in the futuáre, too)

Cut/paste error, we are trying to obtain groups user belongs to

Checking for "KRB5_LOGIN" doesn't sound logical here either.

Sorry for being too verbose. I think the PR is basically fine, we can resolve the notes
in a follow-up PR (it should work as is, according to my testing and "static" analysis).

Thank you for moving this forward!

Additional note: Is this condition actually ever used? The /login/ url that only calls classmethod is wrapped with '@oid.login_handler' ....?

I am not sure, I will try if I can get into that condition.
But the original code did the same thing
https://pagure.io/copr/copr/blob/main/f/frontend/coprs_frontend/coprs/views/misc.py#_99

could this be responsible for calling GroupAuth.groups()?

I am not sure. This part of the code is called only when there is no models.User database row for this user. But we want to get user groups at every login.

We could probably change the logic a bit more and don't ask for

    krb_login = (
        models.Krb5Login.query
        .filter(models.Krb5Login.primary == username)
        .first()
    )

    if krb_login:
        user = krb_login.user

but instead, simply do UserAuth.user_object(username=username) which would query or create a models.User object. Then we could update the groups as well because the code wouldn't be specific for the first-time login. For the first-time login, we would only create models.Krb5Login row.

What do you think?

4 new commits added

  • fix group names
  • move more code to logout
  • document openid_groups
  • document after_login
2 years ago

Sorry for being too verbose. I think the PR is basically fine, we can resolve the notes
in a follow-up PR (it should work as is, according to my testing and "static" analysis).

No problem, and I don't think you are being too nitpicky or too verbose here. The code is not easy to navigate and I would rather discover issues sooner than later.

I pushed some new commits. I intend to squash them to the older ones but for the sake of easier review, I temporarily pushed them separately.

Build failed. More information on how to proceed and troubleshoot errors available at https://fedoraproject.org/wiki/Zuul-based-ci

4 new commits added

  • login changes
  • groups
  • login docstring
  • don't check LDAP config
2 years ago

1 new commit added

  • logout
2 years ago

Build failed. More information on how to proceed and troubleshoot errors available at https://fedoraproject.org/wiki/Zuul-based-ci

Some changes based on our 1on1 with @praiskup.

-        if "krb5_login" in flask.session:
-            return flask.session["krb5_login"]
-        return None
+        return flask.g.user.username if flask.g.user else None

We probably can't do this because when this is called and
flask.session["krb5_login"] has some value, the flask.g.user is
still None. In fact, we are using this method to get the username
and based on that, we then query models.User object from the
database and set flask.g.user.

We probably can't do this because when this is called and flask.session["krb5_login"] has some value, the flask.g.user is still None.

ACK, it just seems weird :) it seems we are setting the flask.g.user first (is that actually a session?).

Could we have a simple unit test for 'GroupAuth.group_names()' output with faked ldap_client.get_user_groups(?

4 new commits added

  • squash test
  • oid_login doc
  • remove u
  • add test
2 years ago

Build succeeded.

6 new commits added

  • login changes
  • login docstring
  • document after_login
  • frontend: encapsulate all auth-mechanism decision making at one place
  • frontend: support LDAP groups for Kerberos users
  • frontend: refactor auth code
2 years ago

Build succeeded.

rebased onto 3fa4dec

2 years ago

Build succeeded.

4 new commits added

  • frontend: rename login route to oid_login
  • frontend: encapsulate all auth-mechanism decision making at one place
  • frontend: support LDAP groups for Kerberos users
  • frontend: refactor auth code
2 years ago

Build succeeded.

I added some pylint fixes, added the GroupAuth.group_names() test, and rebased the PR.

Commit 44ced72 fixes this pull-request

Pull-Request has been merged by praiskup

2 years ago

Commit d0155ae fixes this pull-request

Pull-Request has been merged by praiskup

2 years ago

Commit 37cdc5f fixes this pull-request

Pull-Request has been merged by praiskup

2 years ago

Commit 4089459 fixes this pull-request

Pull-Request has been merged by praiskup

2 years ago