fedocal

#214 Add some regexp validation to fix #213
Merged by humaton. Opened by james.
james/fedocal master  into  master

Download 214.patch

Signed-off-by: James Antill james@and.org

Note that this isn't been tested in staging and the message to the UI has changed, so translations need to be updated.

Note that I tested the function against:

  • meeting-1@fedoraproject.org
  • #meeting-1:fedoraproject.org
  • https://matrix.to/#/#meeting-1:fedoraproject.org
Edited by james

We should add those to unit tests as well.

@james thanks for your contribution, lgtm.

As @zlopez said, test coverage would be nice.
I put something together, feel free to add the patch to the PR if you are happy with it.

0001-tests-Validate-meeting-location.patch

From ac12c748b5ecfc87ac4bf1c93361ba24e855e27b Mon Sep 17 00:00:00 2001
From: Dominik Wombacher <dominik@wombacher.cc>
Date: Tue, 7 May 2024 10:34:56 +0000
Subject: [PATCH] tests: Validate meeting location.
Relates to PR #214 and Issue #213
---
 tests/test_flask.py | 68 ++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 67 insertions(+), 1 deletion(-)
diff --git a/tests/test_flask.py b/tests/test_flask.py
index 2db5523..a171050 100644
--- a/tests/test_flask.py
+++ b/tests/test_flask.py
@@ -1562,7 +1562,9 @@ class Flasktests(Modeltests):
             self.assertIn(
                 '<title>Add meeting - Fedocal</title>', output_text)
-            # Invalid location
+            # Invalid meeting location: IRC Channel name without server
+            # Allowed: channel@irc.server.tld
+            # https://pagure.io/fedocal/issue/118
             data = {
                 'meeting_name': 'guess what?',
                 'meeting_date': TODAY,
@@ -1787,6 +1789,70 @@ class Flasktests(Modeltests):
                 self.assertNotIn(
                     'href="/meeting/20/?from_date=', output_text)
+            # Valid meeting location: IRC Channel with server
+            # https://pagure.io/fedocal/issue/118
+            data = {
+                'meeting_name': 'guess what?',
+                'meeting_date': TODAY,
+                'meeting_time_start': time(13, 0),
+                'meeting_time_stop': time(14, 0),
+                'meeting_timezone': 'Europe/Paris',
+                'meeting_location': 'meeting-1@fedoraproject.org',
+                'frequency': '',
+                'csrf_token': csrf_token,
+            }
+
+            with testing.mock_sends(schema.MeetingNewV1):
+                output = self.app.post('/test_calendar/add/', data=data,
+                                       follow_redirects=True)
+                self.assertEqual(output.status_code, 200)
+                output_text = output.get_data(as_text=True)
+                self.assertIn(
+                    '<li class="message">Meeting added</li>', output_text)
+
+            # Valid meeting location: Matrix Room
+            # https://pagure.io/fedocal/issue/213
+            data = {
+                'meeting_name': 'guess what?',
+                'meeting_date': TODAY,
+                'meeting_time_start': time(13, 0),
+                'meeting_time_stop': time(14, 0),
+                'meeting_timezone': 'Europe/Paris',
+                'meeting_location': '#meeting-1:fedoraproject.org',
+                'frequency': '',
+                'csrf_token': csrf_token,
+            }
+
+            with testing.mock_sends(schema.MeetingNewV1):
+                output = self.app.post('/test_calendar/add/', data=data,
+                                       follow_redirects=True)
+                self.assertEqual(output.status_code, 200)
+                output_text = output.get_data(as_text=True)
+                self.assertIn(
+                    '<li class="message">Meeting added</li>', output_text)
+
+            # Valid meeting location: Matrix Room URL
+            # https://pagure.io/fedocal/issue/213
+            data = {
+                'meeting_name': 'guess what?',
+                'meeting_date': TODAY,
+                'meeting_time_start': time(13, 0),
+                'meeting_time_stop': time(14, 0),
+                'meeting_timezone': 'Europe/Paris',
+                'meeting_location': 'https://matrix.to/#/#meeting-1:fedoraproject.org',
+                'frequency': '',
+                'csrf_token': csrf_token,
+            }
+
+            with testing.mock_sends(schema.MeetingNewV1):
+                output = self.app.post('/test_calendar/add/', data=data,
+                                       follow_redirects=True)
+                self.assertEqual(output.status_code, 200)
+                output_text = output.get_data(as_text=True)
+                self.assertIn(
+                    '<li class="message">Meeting added</li>', output_text)
+
+
     def test_edit_meeting(self):
         """ Test the edit_meeting function. """
         self.__setup_db()
-- 
2.44.0

To run the tests:

sudo dnf install gcc gcc-c++ rust cargo openssl-devel
tox -e py36 -- tests/test_flask.py::Flasktests

Try not to run against any newer python version, that's a whole different discussion to update the code-base...

I'll add the tests to the PR, however I also ran the validation against the JSON API for all the current meetings:

https://calendar.fedoraproject.org/api/meetings/

...here are the current locations that would fail the new validation:

  • CHAOSS monthly call => https://unomaha.zoom.us/j/609939109
  • CHAOSS weekly team meeting => https://hangouts.google.com/hangouts/_/67zc3kvkovelxctlilphwodlp4e
  • Docs Writing Workshop =>
  • EPEL Office Hours => https://meet.opensuse.org/epel
  • FZUG (Chinese) IRC Meeting => irc://irc.libera.chat/fedora-zh
  • Fedora DEI Team meeting (A/V) => https://meet.google.com/acq-pwxk-fhv
  • Fedora Linux 40 Final Go/No-Go =>
  • Fedora Source-git SIG => meet.google.com/mic-otnv-kse
  • Fedora Websites and Apps Team Weekly Meeting => https://meet.jit.si/fedora-websites-apps-meeting
  • KDE SIG => https://meet.kde.org/b/ale-swq-39j
  • Magazine Editorial Board => matrix.to-meeting-2@fedoraproject.org
  • OKD Community Development Meeting => https://umich.zoom.us/j/96648123924?pwd=RitQVVQvMFVSaXJhNkFBS08vWTk0Zz09
  • OKD Working Group Meeting => https://umich.zoom.us/j/99842244394?pwd=YVdkQjJTdnpBMUkySGVzK1kyTGoyZz09
  • Python Maint Fedora Bugs Triage => https://meet.google.com/xuj-jswy-hat
  • Release => podcast.fedoraproject.org
  • Workstation WG => https://meet.google.com/jod-dkmw-ibd
  • ankursinha on PTO =>
  • bittin - Fedora resting time =>
  • frostyx - Copr community person =>
  • gotmax23 on leave =>
  • hank - vacation =>
  • nikromen - Copr community person =>
  • nphilipp - vacation =>
  • praiskup - Copr community person =>
  • principis - Thesis =>
  • qulogic =>

The blanks can maybe be ignored because there are null entries (I assume the locations have a space in them?) ... but we could also allow empty strings, just in case.
google hangouts, google meet, kde meet, opensuse meet, meet.jit.si and zoom need to be added though (or a generalization). Not sure if we want to add the irc url form?
Edited by james

3 new commits added

  • tests: Validate meeting location using current values from calendar.
  • Add more validation for #213, meet/zoom/hangouts/etc.
  • tests: Validate meeting location.

Okay, I allowed the irc url format and somewhat generalized versions of meet/zoom/etc. (and allowed podcast) ... and merged Dominik's tests and then added a slight variant that tests all of the above failures.

Note that there are still two current meetings which fail this validation:

  • Fedora Source-git SIG => meet.google.com/mic-otnv-kse
  • Magazine Editorial Board => matrix.to-meeting-2@fedoraproject.org

...the first needs to add "https://" to the beginning and it'll be fine the second is a hack to workaround matrix not being valid before this PR ... we can allow the hack ('.' characters before the @ sign) or not.

4 new commits added

  • tests: Validate meeting location using current values from calendar.
  • Add more validation for #213, meet/zoom/hangouts/etc.
  • tests: Validate meeting location.
  • Add some regexp validation to fix #213

I like your thinking and all the additional tests you did, great job!

I just got a reminder for the EPEL Steering Committee meeting.
The meeting link that they use is: https://chat.fedoraproject.org/#/room/#meeting:fedoraproject.org

So it looks like there are a more Matrix cases that have to be covered.
The URL can be different and #/room/# and #/# seem both valid.

When I look at the "EPEL steering committee" meeting location (via. the JSON above) I get: fedora-meeting@chat.fedoraproject.org ... are they putting that link somewhere other than the location? Or am I missing something?

One simple thing we could do is allow the IRC and matrix short formats and then any https url, which would also cover the above chat.fp.org matrix url?

are they putting that link somewhere other than the location? Or am I missing something?

Yep, looks like it's in the description.

One simple thing we could do is allow the IRC and matrix short formats and then any https url, which would also cover the above chat.fp.org matrix url?

If you mean replacing matrix.to in your regex with a wildcard to put in whatever url someone like, I guess that would work.

3 new commits added

  • Allow all URLs for a meeting location. Issue #213
  • tests: Stupid hack workaround for mock/strftime failures.
  • tests: Fix message we check for, in test error output.

I need some help with the hack workaround for the tests ... I don't see how/why it fails in a loop (somewhere deep in the mock code that can't find strftime defined) but all the tests works fine otherwise when not in a loop.
I guess I could just paste everything N times, or maybe a function would help?

Also added the "any http/irc URL is fine" patch, so people can see what I meant and comment on it.

@james Could you point me to what loop do you mean?

It's the hack workaround commit:

https://pagure.io/fork/james/fedocal/c/9f687b62d1e30aa90615fe3ee4fcc73ea7f24d33

This is the before:

https://pagure.io/fork/james/fedocal/blob/601e8cc5c625bfc9c627c8ebeff690243ec91af7/f/tests/test_flask.py#_1855

And this is the current code:

https://pagure.io/fork/james/fedocal/blob/master/f/tests/test_flask.py#_1883

I will try to look at it and see what I can do with it.

1 new commit added

  • Make it more obvious what the weird testing problem is.

Just added a new commit, which hopefully makes it more obvious what/where the testing issue is ... I've still no idea how to fix it without randomly picking 4 tests to run (what it does now) but I'm pretty sure all the code that would run in production works and is tested.

This set of commits also includes the "allow all URLs" patch, so someone should decide on if we allow everything or be more strict and I can easily remove that before this is merged.
Edited by james

I will look at the testing issue this week. Didn't have time for it till now.

@james Were you able to even run the test on anything else than python3.6? If I try to just execute tox it will fail for everything on bad import. Only python36 is working.

And for me the Flasktests.test_upload_calendar is failing as well. Not sending any message.

Probably found the reason for it to fail, it has problem when multiple meetings are set for same time slot. I have some idea how to fix it, let me try it.

Unfortunately I'm not able to push the changes to your branch, so here is the diff you can use.

diff --git a/tests/test_flask.py b/tests/test_flask.py
index 0f63b76..0ed6751 100644
--- a/tests/test_flask.py
+++ b/tests/test_flask.py
@@ -1789,60 +1789,55 @@ class Flasktests(Modeltests):
                 self.assertNotIn(
                     'href="/meeting/20/?from_date=', output_text)
-            def _tst_add_meeting_loc(loc):
-              data = {
+            # Valid meeting location: List of current meeting locations:
+            meet_locations = [
+                'https://matrix.to/#/#meeting-1:fedoraproject.org',
+                '#meeting-1:fedoraproject.org',
+                'meeting-1@fedoraproject.org',
+                '',
+                'https://unomaha.zoom.us/j/609939109',
+                'https://hangouts.google.com/hangouts/_/67zc3kvkovelxctlilphwodlp4e',
+                'https://meet.opensuse.org/epel',
+                'irc://irc.libera.chat/fedora-zh',
+                'https://meet.google.com/acq-pwxk-fhv',
+                'https://meet.google.com/mic-otnv-kse',
+                'https://meet.jit.si/fedora-websites-apps-meeting',
+                'https://meet.kde.org/b/ale-swq-39j',
+                'https://umich.zoom.us/j/96648123924?pwd=RitQVVQvMFVSaXJhNkFBS08vWTk0Zz09',
+                'https://umich.zoom.us/j/99842244394?pwd=YVdkQjJTdnpBMUkySGVzK1kyTGoyZz09',
+                'https://meet.google.com/xuj-jswy-hat',
+                'podcast.fedoraproject.org',
+                'https://podcast.fedoraproject.org/',
+                'https://meet.google.com/jod-dkmw-ibd'
+            ]
+            # https://pagure.io/fedocal/issue/213
+            data = {
                 'meeting_name': 'guess what?',
                 'meeting_date': TODAY,
-                'meeting_time_start': time(13, 0),
-                'meeting_time_stop': time(14, 0),
+                'meeting_time_start': None,
+                'meeting_time_stop': None,
                 'meeting_timezone': 'Europe/Paris',
-                'meeting_location': loc,
+                'meeting_location': None,
                 'frequency': '',
                 'csrf_token': csrf_token,
-              }
-              with testing.mock_sends(schema.MeetingNewV1):
-                output = self.app.post('/test_calendar/add/', data=data,
-                                       follow_redirects=True)
-                self.assertEqual(output.status_code, 200)
-                output_text = output.get_data(as_text=True)
-                self.assertIn(
-                    '<li class="message">Meeting added</li>', output_text)
-
-            meeting_locs = set()
-            # Valid meeting location: IRC Channel with server
-            # https://pagure.io/fedocal/issue/118
-            meeting_locs.add('meeting-1@fedoraproject.org')
-
-            # Valid meeting location: Matrix Room
-            # https://pagure.io/fedocal/issue/213
-            meeting_locs.add('#meeting-1:fedoraproject.org')
-
-            # Valid meeting location: Matrix Room URL
-            # https://pagure.io/fedocal/issue/213
-            meeting_locs.add('https://matrix.to/#/#meeting-1:fedoraproject.org')
-
-            # Valid meeting location: List of current meeting locations:
-            meeting_locs.add('')
-            meeting_locs.add('https://unomaha.zoom.us/j/609939109')
-            meeting_locs.add('https://hangouts.google.com/hangouts/_/67zc3kvkovelxctlilphwodlp4e')
-            meeting_locs.add('https://meet.opensuse.org/epel')
-            meeting_locs.add('irc://irc.libera.chat/fedora-zh')
-            meeting_locs.add('https://meet.google.com/acq-pwxk-fhv')
-            meeting_locs.add('https://meet.google.com/mic-otnv-kse')
-            meeting_locs.add('https://meet.jit.si/fedora-websites-apps-meeting')
-            meeting_locs.add('https://meet.kde.org/b/ale-swq-39j')
-            meeting_locs.add('https://umich.zoom.us/j/96648123924?pwd=RitQVVQvMFVSaXJhNkFBS08vWTk0Zz09')
-            meeting_locs.add('https://umich.zoom.us/j/99842244394?pwd=YVdkQjJTdnpBMUkySGVzK1kyTGoyZz09')
-            meeting_locs.add('https://meet.google.com/xuj-jswy-hat')
-            meeting_locs.add('podcast.fedoraproject.org')
-            meeting_locs.add('https://podcast.fedoraproject.org/')
-            meeting_locs.add('https://meet.google.com/jod-dkmw-ibd')
-            import random
-            for i in range(4): # If this changes to 5 or higher, execption
-                meeting_loc = random.choice(list(meeting_locs))
-                _tst_add_meeting_loc(meeting_loc)
-                meeting_locs.remove(meeting_loc)
-
+            }
+ 
+            start_time = 0
+            end_time = 1
+            for meet_location in meet_locations:
+                with testing.mock_sends(schema.MeetingNewV1):
+                    data['meeting_location'] = meet_location
+                    start_time = start_time + 1
+                    end_time = end_time + 1
+                    data['meeting_time_start'] = time(start_time, 0)
+                    data['meeting_time_stop'] = time(end_time, 0)
+                    output = self.app.post('/test_calendar/add/', data=data,
+                                        follow_redirects=True)
+                    self.assertEqual(output.status_code, 200)
+                    output_text = output.get_data(as_text=True)
+                    self.assertIn(
+                        '<li class="message">Meeting added</li>', output_text)
+ 
     def test_edit_meeting(self):
         """ Test the edit_meeting function. """

You can apply it just saving it to file and doing git apply <filename> on your branch.

8 new commits added

  • test: Change the meeting time to not get the testing error, and test all urls.
  • Allow all URLs for a meeting location. Issue #213
  • tests: Stupid hack workaround for mock/strftime failures.
  • tests: Fix message we check for, in test error output.
  • tests: Validate meeting location using current values from calendar.
  • Add more validation for #213, meet/zoom/hangouts/etc.
  • tests: Validate meeting location.
  • Add some regexp validation to fix #213

Okay, thanks for the fix! ... I'm not sure if it should fail when you try to add a 5th meeting in a single timeslot, but using different times will work until we have more than 23 tests for that bit!

I integrated your change, and all the tests pass now ... either with the full url exception or not. Not sure who needs to do a final sign off before we merge?

I can merge it, but I'm not sure how the release is done for this one.

Trying to get this moving ... @pingou @bookwar

I assume you both know how to create/deploy a release?
Did you have any changes/problems with the merge?
Did you want to walk me through a release/deploy or do it yourself?
Also there's PR#212 which seems like it could also be merged.

let me merge this so we can move on with other PRs as well

Pull-Request has been merged by humaton
Metadata
None
No Tags
Static archive. pagure.io was sunset in 2026; this is a read-only snapshot.
Red Hat Logo

Fedora is sponsored by Red Hat. Learn more about the relationship between Red Hat and Fedora »

© Red Hat, Inc. and others.