Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Content Security Policies - patch 4 - matomo stats #430

Merged
merged 5 commits into from
Sep 28, 2023

Conversation

vincentporte
Copy link
Contributor

@vincentporte vincentporte commented Sep 27, 2023

Description

🎸 rétablir l'envoi des stats d'activités vers matomo : correction des CSP
🎸 rendre paramétrables (url et token) l'envoi des stats vers l'instance matomo choisie

Type de changement

🪲 Correction de bug
🚧 technique

Points d'attention

🦺 mise à jour de la gestion du token d'authentification dans get_matomo_data
🦺 ajouter MATOMO_SITE_ID, MATOMO_BASE_URL et MATOMO_AUTH_TOKEN dans les variables d'env clevercloud
🦺 Suppression de la variable COMMU_ENVIRONMENT devenue inutile

@vincentporte vincentporte self-assigned this Sep 27, 2023
@vincentporte vincentporte added hot fix Pull requests that fix a bug to fix as soon as possible python Pull requests that update Python code labels Sep 27, 2023
@vincentporte vincentporte force-pushed the vincentporte/csp_connect_src branch 2 times, most recently from 5ab8a74 to 30c2412 Compare September 27, 2023 10:15
MATOMO_SITE_ID = os.getenv("MATOMO_SITE_ID", None)
MATOMO_BASE_URL = os.getenv("MATOMO_BASE_URL", None) # "https://stats.data.gouv.fr/"

# for `lacommunaute.utils.matomo.get_matomo_data` function needs
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

attention à ce genre de commentaire qui risque de mal vieillir, si ta fonction change de nom, si c'est utilisé ailleurs... je recommande de le retirer, les gens trouveront l'usage en fouillant !

MATOMO_SITE_ID = 268
MATOMO_URL = "https://stats.data.gouv.fr/index.php"
MATOMO_SITE_ID = os.getenv("MATOMO_SITE_ID", None)
MATOMO_BASE_URL = os.getenv("MATOMO_BASE_URL", None) # "https://stats.data.gouv.fr/"
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

commentaire inutile !

@@ -328,8 +328,17 @@

# MATOMO
# ---------------------------------------
MATOMO_SITE_ID = 268
MATOMO_URL = "https://stats.data.gouv.fr/index.php"
MATOMO_SITE_ID = os.getenv("MATOMO_SITE_ID", None)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

je dirais int(os.getenv(...)) pour etre sur de pas mettre des betises, si tu mets un truc non integer Matomo va crasher au chargement, et ce sera en prod du coup.

Mais du coup, ne le lire que si on a deja une BASE_URL me semble plus sain.


# for `lacommunaute.utils.matomo.get_matomo_data` function needs
if MATOMO_BASE_URL:
MATOMO_URL = os.path.join(MATOMO_BASE_URL, "index.php")
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Je ne suis pas certain de l'intérêt du MATOMO_URL dans les settings, on pourrait juste le mettre là où il est utilisé directement dans lacommunaute/utils/matomo.py ?

MATOMO_AUTH_TOKEN = os.getenv("MATOMO_AUTH_TOKEN", "set-matomo-auth-token")
else:
MATOMO_URL = None
MATOMO_AUTH_TOKEN = None
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Je dirais que si on a un BASE_URL, alors on exige un AUTH_TOKEN non ?

Pas de valeur par défaut => ça crashe si on n'en renseigne pas => ça permet d'éviter d'oublier.

@@ -4,14 +4,15 @@
from dateutil.relativedelta import relativedelta
from django.conf import settings

from config.settings.base import MATOMO_AUTH_TOKEN
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

attention: il faut utiliser from django.conf import settings sinon tu ignores complètement le module de settings choisi au démarrage par DJANGO_SETTINGS_MODULE (dev, test, etc)

from lacommunaute.forum_stats.models import Stat


def get_matomo_data(
period,
search_date,
method,
token_auth="anonymous",
token_auth=MATOMO_AUTH_TOKEN,
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hehe, mieux :D comment ça marchait auparavant ?

Copy link
Contributor Author

@vincentporte vincentporte Sep 27, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Le token pour stats.data.gouv est 'anonymous' d'où la valeur par défaut ;)

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow :)

@@ -12,4 +12,7 @@ def expose_settings(request):
"BASE_TEMPLATE": base_template,
"ALLOWED_HOSTS": settings.ALLOWED_HOSTS,
"COMMU_ENVIRONMENT": settings.COMMU_ENVIRONMENT,
"MATOMO_URL": settings.MATOMO_URL,
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pas sur que ce soit utilisé dans les templates, du coup peut etre pas la peine ?

@vincentporte vincentporte merged commit 1c8b1d4 into master Sep 28, 2023
4 checks passed
@vincentporte vincentporte deleted the vincentporte/csp_connect_src branch September 28, 2023 10:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hot fix Pull requests that fix a bug to fix as soon as possible python Pull requests that update Python code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants