diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index d99918664..fde41868d 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -47,12 +47,12 @@ repos: ] files: ^requirements/.*\.txt$ - repo: https://github.com/asottile/pyupgrade - rev: v3.13.0 + rev: v3.15.0 hooks: - id: pyupgrade args: ['--keep-runtime-typing', '--py310-plus'] - repo: https://github.com/astral-sh/ruff-pre-commit - rev: v0.0.291 + rev: v0.0.292 hooks: - id: ruff args: ['--fix', '--exit-non-zero-on-fix'] @@ -127,7 +127,7 @@ repos: - id: flake8 additional_dependencies: *flake8deps - repo: https://github.com/PyCQA/pylint - rev: v3.0.0a7 + rev: v3.0.1 hooks: - id: pylint args: [ @@ -147,7 +147,7 @@ repos: additional_dependencies: - 'bandit[toml]' - repo: https://github.com/pre-commit/pre-commit-hooks - rev: v4.4.0 + rev: v4.5.0 hooks: - id: check-added-large-files - id: check-ast diff --git a/funnel/forms/organization.py b/funnel/forms/organization.py index cb505621b..077b96454 100644 --- a/funnel/forms/organization.py +++ b/funnel/forms/organization.py @@ -61,9 +61,12 @@ def validate_name(self, field: forms.Field) -> None: ) if reason == 'reserved': raise forms.validators.ValidationError(_("This name is reserved")) - if self.edit_obj and field.data.lower() == self.edit_obj.name.lower(): - # Name is not reserved or invalid under current rules. It's also not changed - # from existing name, or has only changed case. This is a validation pass. + if ( + self.edit_obj + and self.edit_obj.name + and field.data.lower() == self.edit_obj.name.lower() + ): + # Name has only changed case from previous name. This is a validation pass return if reason == 'user': if ( diff --git a/funnel/forms/sync_ticket.py b/funnel/forms/sync_ticket.py index fae507f95..1682820a5 100644 --- a/funnel/forms/sync_ticket.py +++ b/funnel/forms/sync_ticket.py @@ -175,8 +175,8 @@ class TicketParticipantForm(forms.Form): ) email = forms.EmailField( __("Email"), - validators=[forms.validators.DataRequired(), forms.validators.ValidEmail()], - filters=[forms.filters.strip()], + validators=[forms.validators.Optional(), forms.validators.ValidEmail()], + filters=[forms.filters.none_if_empty()], ) phone = forms.StringField( __("Phone number"), @@ -219,6 +219,9 @@ def set_queries(self) -> None: def validate(self, *args, **kwargs) -> bool: """Validate form.""" result = super().validate(*args, **kwargs) + if self.email.data is None: + self.user = None + return True with db.session.no_autoflush: accountemail = AccountEmail.get(email=self.email.data) if accountemail is not None: diff --git a/funnel/models/account.py b/funnel/models/account.py index b8c77428a..b594dd2d5 100644 --- a/funnel/models/account.py +++ b/funnel/models/account.py @@ -113,9 +113,14 @@ class PROFILE_STATE(LabeledEnum): # noqa: N801 class ZBase32Comparator(Comparator[str]): # pylint: disable=abstract-method """Comparator to allow lookup by Account.uuid_zbase32.""" - def __eq__(self, other: str) -> sa.ColumnElement[bool]: # type: ignore[override] + def __eq__(self, other: object) -> sa.ColumnElement[bool]: # type: ignore[override] """Return an expression for column == other.""" - return self.__clause_element__() == UUID(bytes=zbase32_decode(other)) + try: + return self.__clause_element__() == UUID( # type: ignore[return-value] + bytes=zbase32_decode(str(other)) + ) + except ValueError: # zbase32 call failed, so it's not a valid string + return sa.false() class Account(UuidMixin, BaseMixin, Model): diff --git a/funnel/models/project_membership.py b/funnel/models/project_membership.py index 7b50b15c5..fda29e6b3 100644 --- a/funnel/models/project_membership.py +++ b/funnel/models/project_membership.py @@ -12,7 +12,7 @@ from .membership_mixin import ImmutableUserMembershipMixin from .project import Project -__all__ = ['ProjectMembership', 'project_child_role_map'] +__all__ = ['ProjectMembership', 'project_child_role_map', 'project_child_role_set'] #: Roles in a project and their remapped names in objects attached to a project project_child_role_map: dict[str, str] = { @@ -24,6 +24,9 @@ 'reader': 'reader', } +#: A model that is indirectly under a project needs the role names without remapping +project_child_role_set: set[str] = set(project_child_role_map.values()) + #: ProjectMembership maps project's `account_admin` role to membership's `editor` #: role in addition to the recurring role grant map project_membership_role_map: dict[str, str | set[str]] = { diff --git a/funnel/models/sync_ticket.py b/funnel/models/sync_ticket.py index d51940425..29f99708d 100644 --- a/funnel/models/sync_ticket.py +++ b/funnel/models/sync_ticket.py @@ -207,7 +207,7 @@ class TicketParticipant(EmailAddressMixin, UuidMixin, BaseMixin, Model): """A participant in one or more events, synced from an external ticket source.""" __tablename__ = 'ticket_participant' - __email_optional__ = False + __email_optional__ = True __email_for__ = 'participant' fullname = with_roles( @@ -376,7 +376,9 @@ def checkin_list(cls, ticket_event: TicketEvent) -> list: # TODO: List type? TicketEventParticipant, TicketParticipant.id == TicketEventParticipant.ticket_participant_id, ) - .join(EmailAddress, EmailAddress.id == TicketParticipant.email_address_id) + .outerjoin( + EmailAddress, EmailAddress.id == TicketParticipant.email_address_id + ) .outerjoin( SyncTicket, TicketParticipant.id == SyncTicket.ticket_participant_id ) diff --git a/funnel/models/venue.py b/funnel/models/venue.py index 5f52e5443..620da0b01 100644 --- a/funnel/models/venue.py +++ b/funnel/models/venue.py @@ -2,8 +2,6 @@ from __future__ import annotations -import itertools - from sqlalchemy.ext.orderinglist import ordering_list from coaster.sqlalchemy import add_primary_relationship, with_roles @@ -19,7 +17,7 @@ ) from .helpers import MarkdownCompositeBasic, reopen from .project import Project -from .project_membership import project_child_role_map +from .project_membership import project_child_role_map, project_child_role_set __all__ = ['Venue', 'VenueRoom'] @@ -113,7 +111,7 @@ class VenueRoom(UuidMixin, BaseScopedNameMixin, Model): venue: Mapped[Venue] = with_roles( relationship(Venue, back_populates='rooms'), # Since Venue already remaps Project roles, we just want the remapped role names - grants_via={None: set(itertools.chain(*project_child_role_map.values()))}, + grants_via={None: project_child_role_set}, ) parent: Mapped[Venue] = sa.orm.synonym('venue') description, description_text, description_html = MarkdownCompositeBasic.create( diff --git a/funnel/templates/contacts.html.jinja2 b/funnel/templates/contacts.html.jinja2 index 10cfce8f1..1557acc31 100644 --- a/funnel/templates/contacts.html.jinja2 +++ b/funnel/templates/contacts.html.jinja2 @@ -67,7 +67,7 @@
- {{ useravatar(contact.ticket_participant.account) }} + {{ useravatar(contact.ticket_participant.participant) }}

{{ contact.ticket_participant.fullname }}

{{ contact.ticket_participant.email }}

diff --git a/funnel/views/api/oauth.py b/funnel/views/api/oauth.py index 5f73c7a7d..945ae619d 100644 --- a/funnel/views/api/oauth.py +++ b/funnel/views/api/oauth.py @@ -25,7 +25,7 @@ ) from ...registry import resource_registry from ...typing import ReturnView -from ...utils import abort_null, make_redirect_url +from ...utils import make_redirect_url from ..login_session import reload_for_cookies, requires_client_login, requires_login from .resource import get_userinfo @@ -415,20 +415,20 @@ def oauth_token_success(token: AuthToken, **params) -> ReturnView: def oauth_token() -> ReturnView: """Provide token endpoint for OAuth2 server.""" # Always required parameters - grant_type = cast(Optional[str], abort_null(request.form.get('grant_type'))) + grant_type = cast(Optional[str], request.form.get('grant_type')) auth_client = current_auth.auth_client # Provided by @requires_client_login - scope = abort_null(request.form.get('scope', '')).split(' ') + scope = request.form.get('scope', '').split(' ') # if grant_type == 'authorization_code' (POST) - code = cast(Optional[str], abort_null(request.form.get('code'))) - redirect_uri = cast(Optional[str], abort_null(request.form.get('redirect_uri'))) + code = cast(Optional[str], request.form.get('code')) + redirect_uri = cast(Optional[str], request.form.get('redirect_uri')) # if grant_type == 'password' (POST) - username = cast(Optional[str], abort_null(request.form.get('username'))) - password = cast(Optional[str], abort_null(request.form.get('password'))) + username = cast(Optional[str], request.form.get('username')) + password = cast(Optional[str], request.form.get('password')) # if grant_type == 'client_credentials' buid = cast( Optional[str], # XXX: Deprecated userid parameter - abort_null(request.form.get('buid') or request.form.get('userid')), + request.form.get('buid') or request.form.get('userid'), ) # Validations 1: Required parameters diff --git a/funnel/views/api/shortlink.py b/funnel/views/api/shortlink.py index f4968c7ce..68c657d41 100644 --- a/funnel/views/api/shortlink.py +++ b/funnel/views/api/shortlink.py @@ -10,13 +10,12 @@ from ... import app, shortlinkapp from ...models import Shortlink, db -from ...utils import abort_null from ..helpers import app_url_for, validate_is_app_url # Add future hasjobapp route here @app.route('/api/1/shortlink/create', methods=['POST']) -@requestform(('url', abort_null), ('shorter', getbool), ('name', abort_null)) +@requestform('url', ('shorter', getbool), 'name') def create_shortlink( url: str | furl, shorter: bool = True, name: str | None = None ) -> tuple[dict[str, str], int]: diff --git a/funnel/views/api/sms_events.py b/funnel/views/api/sms_events.py index a47420668..22053843d 100644 --- a/funnel/views/api/sms_events.py +++ b/funnel/views/api/sms_events.py @@ -19,7 +19,6 @@ ) from ...transports.sms import validate_exotel_token from ...typing import ReturnView -from ...utils import abort_null @app.route('/api/1/sms/twilio_event', methods=['POST']) @@ -116,7 +115,7 @@ def process_exotel_event(secret_token: str) -> ReturnView: # If there are too many rejects, then most likely a hack attempt. statsd.incr('phone_number.event', tags={'engine': 'exotel', 'stage': 'received'}) - exotel_to = abort_null(request.form.get('To', '')) + exotel_to = request.form.get('To', '') if not exotel_to: return {'status': 'eror', 'error': 'invalid_phone'}, 422 # Exotel sends back 0-prefixed phone numbers, not plus-prefixed intl. numbers diff --git a/funnel/views/contact.py b/funnel/views/contact.py index 4293ac62a..43064c7ac 100644 --- a/funnel/views/contact.py +++ b/funnel/views/contact.py @@ -17,7 +17,7 @@ from .. import app from ..models import ContactExchange, Project, TicketParticipant, db, sa from ..typing import ReturnRenderWith, ReturnView -from ..utils import abort_null, format_twitter_handle +from ..utils import format_twitter_handle from .login_session import requires_login @@ -141,7 +141,7 @@ def scan(self) -> ReturnView: @route('scan/connect', endpoint='scan_connect', methods=['POST']) @requires_login - @requestargs(('puk', abort_null), ('key', abort_null)) + @requestargs('puk', 'key') def connect(self, puk: str, key: str) -> ReturnView: """Verify a badge scan and create a contact.""" ticket_participant = TicketParticipant.query.filter_by(puk=puk, key=key).first() diff --git a/funnel/views/helpers.py b/funnel/views/helpers.py index 652bb7361..3602ad59c 100644 --- a/funnel/views/helpers.py +++ b/funnel/views/helpers.py @@ -580,6 +580,16 @@ def shortlink(url: str, actor: Account | None = None, shorter: bool = True) -> s # --- Request/response handlers -------------------------------------------------------- +@app.before_request +def no_null_in_form(): + """Disallow NULL characters in any form submit (but don't scan file attachments).""" + if request.method == 'POST': + for values in request.form.listvalues(): + for each in values: + if each is not None and '\x00' in each: + abort(400) + + @app.after_request def commit_db_session(response: ResponseType) -> ResponseType: """Commit database session at the end of a request if asked to.""" diff --git a/funnel/views/label.py b/funnel/views/label.py index 24070c281..962c468fe 100644 --- a/funnel/views/label.py +++ b/funnel/views/label.py @@ -13,7 +13,6 @@ from ..forms import LabelForm, LabelOptionForm from ..models import Account, Label, Project, db from ..typing import ReturnRenderWith, ReturnView -from ..utils import abort_null from .helpers import render_redirect from .login_session import requires_login, requires_sudo from .mixins import AccountCheckMixin, ProjectViewMixin @@ -29,7 +28,7 @@ class ProjectLabelView(ProjectViewMixin, UrlForView, ModelView): def labels(self) -> ReturnRenderWith: form = forms.Form() if form.validate_on_submit(): - namelist = [abort_null(x) for x in request.values.getlist('name')] + namelist = request.values.getlist('name') for idx, lname in enumerate(namelist, start=1): lbl = Label.query.filter_by(project=self.obj, name=lname).first() if lbl is not None: @@ -51,8 +50,8 @@ def new_label(self) -> ReturnRenderWith: # and those values are also available at `form.data`. # But in case there are options, the option values are in the list # in the order they appeared on the create form. - titlelist = [abort_null(x) for x in request.values.getlist('title')] - emojilist = [abort_null(x) for x in request.values.getlist('icon_emoji')] + titlelist = request.values.getlist('title') + emojilist = request.values.getlist('icon_emoji') # first values of both lists belong to the parent label titlelist.pop(0) emojilist.pop(0) @@ -143,9 +142,9 @@ def edit(self) -> ReturnRenderWith: return render_redirect(self.obj.project.url_for('labels')) if form.validate_on_submit(): - namelist = [abort_null(x) for x in request.values.getlist('name')] - titlelist = [abort_null(x) for x in request.values.getlist('title')] - emojilist = [abort_null(x) for x in request.values.getlist('icon_emoji')] + namelist = request.values.getlist('name') + titlelist = request.values.getlist('title') + emojilist = request.values.getlist('icon_emoji') namelist.pop(0) titlelist.pop(0) diff --git a/funnel/views/login.py b/funnel/views/login.py index 5ca20aaa4..5c4f5567b 100644 --- a/funnel/views/login.py +++ b/funnel/views/login.py @@ -153,7 +153,7 @@ def login() -> ReturnView: if request.method == 'GET': loginmethod = request.cookies.get('login') - formid = abort_null(request.form.get('form.id')) + formid = request.form.get('form.id') if request.method == 'POST' and formid == 'passwordlogin': try: success = loginform.validate() diff --git a/funnel/views/login_session.py b/funnel/views/login_session.py index 25a1dc85d..075712dd3 100644 --- a/funnel/views/login_session.py +++ b/funnel/views/login_session.py @@ -646,7 +646,7 @@ def wrapper(*args: P.args, **kwargs: P.kwargs) -> T | ReturnResponse: return render_redirect(url_for('change_password')) elif request.method == 'POST': - formid = abort_null(request.form.get('form.id')) + formid = request.form.get('form.id') if formid == FORMID_SUDO_OTP: try: otp_session = OtpSession.retrieve('sudo') diff --git a/funnel/views/siteadmin.py b/funnel/views/siteadmin.py index d929b8f08..0c89cf46c 100644 --- a/funnel/views/siteadmin.py +++ b/funnel/views/siteadmin.py @@ -453,8 +453,8 @@ def init_rq_dashboard(): """Register RQ Dashboard Blueprint if available for import.""" if rq_dashboard is not None: rq_dashboard.blueprint.before_request( - lambda: None - if current_auth and current_auth.user.is_sysadmin - else abort(403) + lambda: ( + None if current_auth and current_auth.user.is_sysadmin else abort(403) + ) ) app.register_blueprint(rq_dashboard.blueprint, url_prefix='/siteadmin/rq') diff --git a/funnel/views/ticket_participant.py b/funnel/views/ticket_participant.py index 126d300b6..3cd31d753 100644 --- a/funnel/views/ticket_participant.py +++ b/funnel/views/ticket_participant.py @@ -33,13 +33,7 @@ from ..proxies import request_wants from ..signals import user_data_changed, user_registered from ..typing import ReturnRenderWith, ReturnView -from ..utils import ( - abort_null, - format_twitter_handle, - make_qrcode, - mask_email, - split_name, -) +from ..utils import format_twitter_handle, make_qrcode, mask_email, split_name from .helpers import render_redirect from .login_session import requires_login from .mixins import AccountCheckMixin, ProjectViewMixin, TicketEventViewMixin @@ -96,7 +90,9 @@ def ticket_participant_checkin_data(ticket_participant, project, ticket_event): 'puuid_b58': puuid_b58, 'fullname': ticket_participant.fullname, 'company': ticket_participant.company, - 'email': mask_email(ticket_participant.email), + 'email': mask_email(ticket_participant.email) + if ticket_participant.email + else None, 'badge_printed': ticket_participant.badge_printed, 'checked_in': ticket_participant.checked_in, 'ticket_type_titles': ticket_participant.ticket_type_titles, @@ -269,9 +265,7 @@ def checkin(self) -> ReturnView: form = forms.Form() if form.validate_on_submit(): checked_in = getbool(request.form.get('checkin')) - ticket_participant_ids = [ - abort_null(x) for x in request.form.getlist('puuid_b58') - ] + ticket_participant_ids = request.form.getlist('puuid_b58') for ticket_participant_id in ticket_participant_ids: attendee = TicketEventParticipant.get(self.obj, ticket_participant_id) attendee.checked_in = checked_in diff --git a/migrations/versions/017c60414c03_make_ticket_participant_email_optional.py b/migrations/versions/017c60414c03_make_ticket_participant_email_optional.py new file mode 100644 index 000000000..e10c3250b --- /dev/null +++ b/migrations/versions/017c60414c03_make_ticket_participant_email_optional.py @@ -0,0 +1,44 @@ +"""Make ticket participant email optional. + +Revision ID: 017c60414c03 +Revises: 4f9ca10b7b9d +Create Date: 2023-10-05 15:08:34.540672 + +""" + +import sqlalchemy as sa +from alembic import op + +# revision identifiers, used by Alembic. +revision: str = '017c60414c03' +down_revision: str = '4f9ca10b7b9d' +branch_labels: str | tuple[str, ...] | None = None +depends_on: str | tuple[str, ...] | None = None + + +def upgrade(engine_name: str = '') -> None: + """Upgrade all databases.""" + # Do not modify. Edit `upgrade_` instead + globals().get(f'upgrade_{engine_name}', lambda: None)() + + +def downgrade(engine_name: str = '') -> None: + """Downgrade all databases.""" + # Do not modify. Edit `downgrade_` instead + globals().get(f'downgrade_{engine_name}', lambda: None)() + + +def upgrade_() -> None: + """Upgrade default database.""" + with op.batch_alter_table('ticket_participant', schema=None) as batch_op: + batch_op.alter_column( + 'email_address_id', existing_type=sa.INTEGER(), nullable=True + ) + + +def downgrade_() -> None: + """Downgrade default database.""" + with op.batch_alter_table('ticket_participant', schema=None) as batch_op: + batch_op.alter_column( + 'email_address_id', existing_type=sa.INTEGER(), nullable=False + ) diff --git a/package-lock.json b/package-lock.json index c5cc9e1df..589ea77ff 100644 --- a/package-lock.json +++ b/package-lock.json @@ -9165,9 +9165,9 @@ } }, "node_modules/postcss": { - "version": "8.4.28", - "resolved": "https://registry.npmjs.org/postcss/-/postcss-8.4.28.tgz", - "integrity": "sha512-Z7V5j0cq8oEKyejIKfpD8b4eBy9cwW2JWPk0+fB1HOAMsfHbnAXLLS+PfVWlzMSLQaWttKDt607I0XHmpE67Vw==", + "version": "8.4.31", + "resolved": "https://registry.npmjs.org/postcss/-/postcss-8.4.31.tgz", + "integrity": "sha512-PS08Iboia9mts/2ygV3eLpY5ghnUcfLV/EXTOW1E2qYxJKGGBUtNjN76FYHnMs36RmARn41bC0AZmn+rR0OVpQ==", "funding": [ { "type": "opencollective", diff --git a/tests/conftest.py b/tests/conftest.py index 57e1a6713..f5055d380 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -4,6 +4,7 @@ from __future__ import annotations import re +import time import typing as t import warnings from contextlib import ExitStack @@ -842,20 +843,50 @@ def event_after_transaction_end(_session, transaction): def _truncate_all_tables(engine: sa.Engine) -> None: """Truncate all tables in the given database engine.""" - with engine.begin() as transaction: - transaction.execute( - sa.text( - ''' - DO $$ - DECLARE tablenames text; - BEGIN - tablenames := string_agg( - quote_ident(schemaname) || '.' || quote_ident(tablename), ', ') - FROM pg_tables WHERE schemaname = 'public'; - EXECUTE 'TRUNCATE TABLE ' || tablenames || ' RESTART IDENTITY'; - END; $$''' - ) - ) + deadlock_retries = 0 + while True: + try: + with engine.begin() as transaction: + transaction.execute( + sa.text( + ''' + DO $$ + DECLARE tablenames text; + BEGIN + tablenames := string_agg( + quote_ident(schemaname) + || '.' + || quote_ident(tablename), ', ' + ) FROM pg_tables WHERE schemaname = 'public'; + EXECUTE + 'TRUNCATE TABLE ' || tablenames || ' RESTART IDENTITY'; + END; $$''' + ) + ) + break + except sa.exc.OperationalError: + # The TRUNCATE TABLE call will occasionally have a deadlock when the + # background server process has not finalised the transaction. SQLAlchemy + # recasts :exc:`psycopg.errors.DeadlockDetected` as + # :exc:`sqlalchemy.exc.OperationalError`. Pytest will show as:: + # + # ERROR - sqlalchemy.exc.OperationalError: + # (psycopg.errors.DeadlockDetected) deadlock detected + # DETAIL: Process waits for AccessExclusiveLock on relation + # of database ; blocked by process . Process + # waits for AccessShareLock on relation of database ; + # blocked by process . + # + # We overcome the deadlock by rolling back the transaction, sleeping a + # second and attempting to truncate again, retrying two more times. If the + # deadlock remains unresolved, we raise the error to pytest. We are not + # explicitly checking for OperationalError wrapping DeadlockDetected on the + # assumption that this retry is safe for all operational errors. Any new + # type of non-transient error will be reported by the final raise. + if (deadlock_retries := deadlock_retries + 1) > 3: + raise + transaction.rollback() + time.sleep(1) @pytest.fixture(scope='session')