From d18211c7d3def3f00909402f8fde56e2a048c8a1 Mon Sep 17 00:00:00 2001 From: Pavel Raiskup Date: Nov 16 2022 17:03:01 +0000 Subject: [PATCH 1/2] frontend: add flash message only for the web-ui log-ins Merges: #2368 --- diff --git a/frontend/coprs_frontend/coprs/views/apiv3_ns/apiv3_general.py b/frontend/coprs_frontend/coprs/views/apiv3_ns/apiv3_general.py index 833fc24..8587313 100644 --- a/frontend/coprs_frontend/coprs/views/apiv3_ns/apiv3_general.py +++ b/frontend/coprs_frontend/coprs/views/apiv3_ns/apiv3_general.py @@ -23,6 +23,7 @@ def gssapi_login_action(): expects. """ if "web-ui" in flask.request.full_path: + flask.flash("Welcome, {0}".format(flask.g.user.name), "success") return flask.redirect(oid.get_next_url()) return flask.jsonify(auth_check_response()) @@ -130,5 +131,4 @@ def gssapi_login(): "Admin" if user.admin else "User", user.name ) - flask.flash("Welcome, {0}".format(flask.g.user.name), "success") return gssapi_login_action() From c97259fa8710827e60de3ed1a18f3cfab06bb49f Mon Sep 17 00:00:00 2001 From: Pavel Raiskup Date: Nov 16 2022 21:15:04 +0000 Subject: [PATCH 2/2] frontend: simplify the GSSAPI/FAS login logic This fixes error 500 for the first login over GSSAPI, with disabled FAS and LDAP. In such case we newly set user.openid_groups to empty set. Drop the krb5_login table, it was originally meant to provide multiple Kerberos instances in one Copr (it was meant to map multiple kerbeoros names into one Copr username). Make the UserAuth.user_object() more self-standing, so it gets or creates a new user, and sets (when needed) the metadata. Merges: #2368 --- diff --git a/frontend/coprs_frontend/alembic/versions/bc29f080b915_drop_krb5_table.py b/frontend/coprs_frontend/alembic/versions/bc29f080b915_drop_krb5_table.py new file mode 100644 index 0000000..692b153 --- /dev/null +++ b/frontend/coprs_frontend/alembic/versions/bc29f080b915_drop_krb5_table.py @@ -0,0 +1,24 @@ +""" +drop krb5 table + +Revision ID: bc29f080b915 +Revises: 65a172e3f102 +Create Date: 2022-11-16 16:28:25.759413 +""" + +import sqlalchemy as sa +from alembic import op + + +revision = 'bc29f080b915' +down_revision = '65a172e3f102' + +def upgrade(): + op.drop_table('krb5_login') + +def downgrade(): + op.create_table('krb5_login', + sa.Column('user_id', sa.INTEGER(), autoincrement=False, nullable=False), + sa.Column('primary', sa.VARCHAR(length=80), autoincrement=False, nullable=False), + sa.ForeignKeyConstraint(['user_id'], ['user.id'], name='krb5_login_user_id_fkey') + ) diff --git a/frontend/coprs_frontend/coprs/auth.py b/frontend/coprs_frontend/coprs/auth.py index bb09c27..eb7b18f 100644 --- a/frontend/coprs_frontend/coprs/auth.py +++ b/frontend/coprs_frontend/coprs/auth.py @@ -43,18 +43,45 @@ class UserAuth: 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) + def user_object(oid_resp=None, username=None): + """ + Get or Create a `models.User` object based on the input parameters + """ + if oid_resp: + # All metadata for the user craation are in oid_resp. + return FedoraAccounts.user_from_response(oid_resp) + + if app.config["FAS_LOGIN"] and app.config["KRB5_LOGIN"]: + user = Kerberos.user_from_username(username) + if not user.mail: + # We can not continue (with perhaps freshly created user object) + # now because we don't have the necessary user metadata (e-mail + # and groups). TODO: obtain the info somehow on demand here! + 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) + ) + return user - if username and app.config["KRB5_LOGIN"]: - return Kerberos.user_from_username(username) + if app.config["KRB5_LOGIN"]: + return Kerberos.user_from_username(username, True) raise CoprHttpException("No auth method available") + @staticmethod + def get_or_create_user(username): + """ + Get the user from DB, or create a new one without any additional + metadata if it doesn't exist. + """ + user = UsersLogic.get(username).first() + if user: + return user + app.logger.info("Login for user '%s', " + "creating a database record", username) + return UsersLogic.create_user_wrapper(username) + class GroupAuth: """ @@ -65,31 +92,34 @@ class GroupAuth: """ @classmethod - def groups(cls, resp=None, username=None): + def update_user_groups(cls, user, oid_resp=None): """ - Return a `dict` that can be assigned to `models.User.openid_groups` + Upon a successful login, try to (a) load the list of groups from + authoritative source, and (b) (re)set the 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) + def _do_update(user, grouplist): + user.openid_groups = { + "fas_groups": grouplist, + } - # Fedora user via Kerberos - if app.config["FAS_LOGIN"] and username: - return None + if oid_resp: + _do_update(user, OpenIDGroups.group_names(oid_resp)) + return + # If we have a LDAP pre-configured, now is the right time to load the + # data, or fail. keys = ["LDAP_URL", "LDAP_SEARCH_STRING"] - if username and all(app.config[k] for k in keys): - return LDAPGroups.group_names(username) + if all(app.config[k] for k in keys): + _do_update(user, LDAPGroups.group_names(user.username)) + return - raise CoprHttpException("Nowhere to get user groups from") + # We only ever call update_user_groups() with oid_resp!= None with FAS + assert not app.config["FAS_LOGIN"] + + app.logger.warning("Nowhere to get groups from") + # This copr doesn't support groups. + _do_update(user, []) class FedoraAccounts: @@ -152,22 +182,16 @@ class FedoraAccounts: return oidname_parse.netloc.replace(".{0}".format(config_parse.netloc), "") @classmethod - def user_from_response(cls, resp): + def user_from_response(cls, oid_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) - + username = cls.fed_raw_name(oid_resp.identity_url) + user = UserAuth.get_or_create_user(username) # Update user attributes from FAS - user.mail = resp.email - user.timezone = resp.timezone + user.mail = oid_resp.email + user.timezone = oid_resp.timezone + GroupAuth.update_user_groups(user, oid_resp) return user @@ -200,27 +224,21 @@ class Kerberos: flask.session.pop("krb5_login", None) @staticmethod - def user_from_username(username): + def user_from_username(username, load_metadata=False): """ Create a `models.User` object from Kerberos username + When 'load_metadata' is True, we have to obtain and set the necessary + user metadata (groups, email). """ - user = UsersLogic.get(username).first() - if user: + user = UserAuth.get_or_create_user(username) + if not load_metadata: 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) + user.mail = username + "@" + krb_config['email_domain'] + GroupAuth.update_user_groups(user) + return user @staticmethod def _krb5_login_redirect(next_url=None): @@ -245,7 +263,7 @@ class OpenIDGroups: if "lp" in resp.extensions: # name space for the teams extension team_resp = resp.extensions['lp'] - return {"fas_groups": team_resp.teams} + return team_resp.teams return None diff --git a/frontend/coprs_frontend/coprs/logic/users_logic.py b/frontend/coprs_frontend/coprs/logic/users_logic.py index adc9abd..6f2d79a 100644 --- a/frontend/coprs_frontend/coprs/logic/users_logic.py +++ b/frontend/coprs_frontend/coprs/logic/users_logic.py @@ -144,7 +144,7 @@ class UsersLogic(object): app.logger.info("Deleting user '%s' data", user.name) @classmethod - def create_user_wrapper(cls, username, email, timezone=None): + def create_user_wrapper(cls, username, email=None, timezone=None): """ Initial creation of Copr user (creates the API token, too). Create user + token configuration. diff --git a/frontend/coprs_frontend/coprs/models.py b/frontend/coprs_frontend/coprs/models.py index 02447de..a1c294f 100644 --- a/frontend/coprs_frontend/coprs/models.py +++ b/frontend/coprs_frontend/coprs/models.py @@ -2143,22 +2143,6 @@ class Action(db.Model, helpers.Serializer): return DefaultActionPriorityEnum.vals.get(action_type_str, 0) -class Krb5Login(db.Model, helpers.Serializer): - """ - Represents additional user information for kerberos authentication. - """ - - __tablename__ = "krb5_login" - - # FK to User table - user_id = db.Column(db.Integer, db.ForeignKey("user.id"), nullable=False) - - # krb's primary, i.e. 'username' from 'username@EXAMPLE.COM' - primary = db.Column(db.String(80), nullable=False, primary_key=True) - - user = db.relationship("User", backref=db.backref("krb5_logins")) - - class CounterStat(db.Model, helpers.Serializer): """ Generic store for simple statistics. diff --git a/frontend/coprs_frontend/coprs/views/apiv3_ns/apiv3_general.py b/frontend/coprs_frontend/coprs/views/apiv3_ns/apiv3_general.py index 8587313..a084e77 100644 --- a/frontend/coprs_frontend/coprs/views/apiv3_ns/apiv3_general.py +++ b/frontend/coprs_frontend/coprs/views/apiv3_ns/apiv3_general.py @@ -3,11 +3,11 @@ import re import flask -from coprs import app, oid, db, models +from coprs import app, oid, db from coprs.views.apiv3_ns import apiv3_ns from coprs.exceptions import AccessRestricted from coprs.views.misc import api_login_required -from coprs.auth import UserAuth, GroupAuth +from coprs.auth import UserAuth def auth_check_response(): @@ -94,33 +94,7 @@ def gssapi_login(): if not username: return auth_403("invalid krb5 username: " + krb_username) - krb_login = ( - models.Krb5Login.query - .filter(models.Krb5Login.primary == username) - .first() - ) - - if 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 - + user = UserAuth.user_object(username=username) db.session.add(user) db.session.commit() diff --git a/frontend/coprs_frontend/coprs/views/misc.py b/frontend/coprs_frontend/coprs/views/misc.py index 21c3d84..26b7f12 100644 --- a/frontend/coprs_frontend/coprs/views/misc.py +++ b/frontend/coprs_frontend/coprs/views/misc.py @@ -16,7 +16,7 @@ from coprs.logic.complex_logic import ComplexLogic from coprs.logic.users_logic import UsersLogic from coprs.exceptions import ObjectNotFound from coprs.measure import checkpoint_start -from coprs.auth import FedoraAccounts, UserAuth, GroupAuth +from coprs.auth import FedoraAccounts, UserAuth @app.before_request @@ -98,9 +98,7 @@ def create_or_login(resp): 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) - + user = UserAuth.user_object(oid_resp=resp) db.session.add(user) db.session.commit() flask.flash(u"Welcome, {0}".format(user.name), "success") diff --git a/frontend/coprs_frontend/tests/test_auth.py b/frontend/coprs_frontend/tests/test_auth.py index feb4ad6..81ea396 100644 --- a/frontend/coprs_frontend/tests/test_auth.py +++ b/frontend/coprs_frontend/tests/test_auth.py @@ -28,5 +28,8 @@ class TestGroupAuth(CoprsTestCase): 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"] + user = mock.MagicMock() + GroupAuth.update_user_groups(user) + assert user.openid_groups == { + "fas_groups": ["group1", "group2", "another-group", + "another-group-2"]} diff --git a/frontend/coprs_frontend/tests/test_logging.py b/frontend/coprs_frontend/tests/test_logging.py index 4021945..1983e33 100644 --- a/frontend/coprs_frontend/tests/test_logging.py +++ b/frontend/coprs_frontend/tests/test_logging.py @@ -175,7 +175,7 @@ class TestLoggingAuth(CoprsTestCase): with app.test_request_context(): create_or_login(resp) - log.info.assert_any_call("First login for user '%s', creating " + log.info.assert_any_call("Login for user '%s', creating " "a database record", "somebody") log.info.assert_called_with("%s '%s' logged in", "User", "somebody")