WIP: public: change permission denied handling (#8122) #218

Closed
yweber wants to merge 1 commits from wip/8122-private-page-custom-message into main
Owner
No description provided.
yweber added 1 commit 2024-01-17 14:59:11 +01:00
gitea/combo/pipeline/head There was a failure building this commit Details
f89d091502
data: add custom error message when Page display is not allowed (#8122)
yweber force-pushed wip/8122-private-page-custom-message from f89d091502 to e486381e3e 2024-01-17 16:34:06 +01:00 Compare
yweber force-pushed wip/8122-private-page-custom-message from e486381e3e to 1fabd7a562 2024-01-17 16:50:05 +01:00 Compare
yweber force-pushed wip/8122-private-page-custom-message from 1fabd7a562 to 796feaf067 2024-01-17 17:11:45 +01:00 Compare
yweber changed title from WIP: data: add custom error message when Page display is not allowed (#8122) to data: add custom error message when Page display is not allowed (#8122) 2024-01-17 17:16:12 +01:00
lguerin reviewed 2024-01-18 09:07:06 +01:00
@ -218,6 +218,7 @@ class Page(models.Model):
public = models.BooleanField(_('Public'), default=True)
groups = models.ManyToManyField(Group, verbose_name=_('Roles'), blank=True)
private_error_message = models.TextField(_('Private error message'), blank=True)
Owner

quelque part, dans le label ou en help_text, je rajouterais que c'est un gabarit (en général on ajoute "(template)" à la fin du label)

quelque part, dans le label ou en help_text, je rajouterais que c'est un gabarit (en général on ajoute "(template)" à la fin du label)
yweber marked this conversation as resolved
@ -0,0 +15,4 @@
error_msg_p.removeAttribute('hidden')
}
}
page_visibility_form_update()
Owner

ça doit pouvoir se gérer en ajouter dans data attributes sur les champs, sur ce modèle (sur lingo, mais on a ce qu'il faut dans combo aussi), plutôt qu'ajouter du js:

class Meta:
....
        widgets = {
            'kind': forms.Select(attrs={'data-dynamic-display-parent': 'true'}),
            'reduction_rate': forms.TextInput(
                attrs={
                    'data-dynamic-display-child-of': 'kind',
                    'data-dynamic-display-value': 'reduction',
                }
            ),
            'effort_rate_target': forms.TextInput(
                attrs={
                    'data-dynamic-display-child-of': 'kind',
                    'data-dynamic-display-value': 'effort',
                }
            ),
        }
ça doit pouvoir se gérer en ajouter dans data attributes sur les champs, sur ce modèle (sur lingo, mais on a ce qu'il faut dans combo aussi), plutôt qu'ajouter du js: ```python class Meta: .... widgets = { 'kind': forms.Select(attrs={'data-dynamic-display-parent': 'true'}), 'reduction_rate': forms.TextInput( attrs={ 'data-dynamic-display-child-of': 'kind', 'data-dynamic-display-value': 'reduction', } ), 'effort_rate_target': forms.TextInput( attrs={ 'data-dynamic-display-child-of': 'kind', 'data-dynamic-display-value': 'effort', } ), } ```
Author
Owner

Merci pour tes commentaires !

Si j'ai pu appliquer les autres sans problème, celui-ci me pose soucis. Je n'ai pas l'impression que ça fonctionne dans mon cas :

  • ma checkbox a invariablement pour valeur (récupéré dans le JS avec .val()) "on" : il faudrait que le JS gérant les attributs "dynamic-display-*" gère le cas particulier des checkbox (en accédant a son attribut "checked" ?)
  • le code n'est pas rappelé après la construction/affichage du formulaire dans la popup : le code gérant les attributs ne "voit pas" le formulaire ajouté
  • seul le champ est caché, pas le label (dans les exemples fonctionnant que j'ai trouvé il me semble que le champ était contenu dans le label)

Je ne sais pas si je rate quelque chose dans l'utilisation de ces attributs ?

Sinon je ne vois pas trop d'autre choix que d'ajouter le JS au formulaire (sinon ajouter le cas particulier pour la gestion des checkbox avec "data-dynamic-display-*", mais j'ai l'impression que ça serait un ticket a part non ?)

Merci pour tes commentaires ! Si j'ai pu appliquer les autres sans problème, celui-ci me pose soucis. Je n'ai pas l'impression que ça fonctionne dans mon cas : * ma checkbox a invariablement pour valeur (récupéré dans le JS avec .val()) "on" : il faudrait que le JS gérant les attributs "dynamic-display-*" gère le cas particulier des checkbox (en accédant a son attribut "checked" ?) * le code n'est pas rappelé après la construction/affichage du formulaire dans la popup : le code gérant les attributs ne "voit pas" le formulaire ajouté * seul le champ est caché, pas le label (dans les exemples fonctionnant que j'ai trouvé il me semble que le champ était contenu dans le label) Je ne sais pas si je rate quelque chose dans l'utilisation de ces attributs ? Sinon je ne vois pas trop d'autre choix que d'ajouter le JS au formulaire (sinon ajouter le cas particulier pour la gestion des checkbox avec "data-dynamic-display-*", mais j'ai l'impression que ça serait un ticket a part non ?)
Owner

ok alors laisse tomber, garde le JS custom, effectivement je crois que quand je l'ai copié dans lingo j'ai corrigé un truc pour masquer le champ et le label; mais on ne vas pas modifier ça dans combo de suite parce que ça impacterait la custo de la cellule fiches.

ok alors laisse tomber, garde le JS custom, effectivement je crois que quand je l'ai copié dans lingo j'ai corrigé un truc pour masquer le champ et le label; mais on ne vas pas modifier ça dans combo de suite parce que ça impacterait la custo de la cellule fiches.
yweber marked this conversation as resolved
@ -547,0 +549,4 @@
msg_template = Template(page.private_error_message)
msg_render_ctx = {'page': page, 'request': request}
msg_render_ctx.update(getattr(request, 'extra_context_data', {}))
error_msg = msg_template.render(Context(msg_render_ctx))
Owner

ici je mettrais un RequestContext plutôt qu'un Context, sinon tu risques d'avoir plus tard un ticket qui dit qu'on n'a pas accès à la variable site_base, par exemple

ici je mettrais un RequestContext plutôt qu'un Context, sinon tu risques d'avoir plus tard un ticket qui dit qu'on n'a pas accès à la variable site_base, par exemple
yweber marked this conversation as resolved
@ -547,0 +562,4 @@
msg = _(
'%(open_a)s You can try to authenticate yourself %(close_a)s'
% {'open_a': f'<a href="{login_url}">', 'close_a': '</a>'}
Owner

les params de la chaîne à traduire sont à mettre à l'extérieur du _()

les params de la chaîne à traduire sont à mettre à l'extérieur du _()
yweber marked this conversation as resolved
yweber force-pushed wip/8122-private-page-custom-message from 796feaf067 to 0e1ebcc418 2024-01-18 12:04:23 +01:00 Compare
yweber force-pushed wip/8122-private-page-custom-message from 0e1ebcc418 to 896e484654 2024-01-18 13:48:59 +01:00 Compare
vdeniaud requested changes 2024-01-22 14:47:25 +01:00
@ -253,3 +253,4 @@
def __init__(self, *args, **kwargs):
super().__init__(*args, **kwargs)
self.fields['public'].widget.attrs.update({'onclick': 'page_visibility_form_update()'})
Owner

Je pense qu'on ne fait comme ça nulle part, ma préférence irait à tout faire dans le script, via un addeventlistener

Je pense qu'on ne fait comme ça nulle part, ma préférence irait à tout faire dans le script, via un addeventlistener
yweber marked this conversation as resolved
@ -547,0 +572,4 @@
ctx['page'] = error_page
template_name = settings.COMBO_PUBLIC_TEMPLATES['standard'].get('template')
response_content = loader.render_to_string(template_name, ctx.flatten(), request)
return HttpResponse(response_content, status=403)
Owner

La logique du code est bien mais je suggère de l'écrire tout autrement, en passant par un template dédié :

  • Après rendu du message d'erreur, le code ici devient juste return render(request, 'combo/private-page-error.html', context={'error_message': error_msg}, status=403)
  • Dans le template, gérer le cas non connecté et afficher le lien de connexion grâce un bloc {% if (ça devrait rendre mieux que de construire du html dans la vue)
    • À propos, la construction de l'URL de login peut se permettre d'être méga triviale, genre href="{% url "auth_login" %}?next={{ request.build_absolute_uri }}" (le gain en lisibilité outrepasse large le gain en robustesse de passer par urlparse, et le "next" est déjà hardcodé à plein endroits, pas besoin d'aller le chercher dans REDIRECT_FIELD_NAME)

N'hésite pas à me dire si tu trouves cette idée mauvaise ou qu'il y a une complexité que j'ai raté 😄

La logique du code est bien mais je suggère de l'écrire tout autrement, en passant par un template dédié : * Après rendu du message d'erreur, le code ici devient juste `return render(request, 'combo/private-page-error.html', context={'error_message': error_msg}, status=403)` * Dans le template, gérer le cas non connecté et afficher le lien de connexion grâce un bloc `{% if` (ça devrait rendre mieux que de construire du html dans la vue) * À propos, la construction de l'URL de login peut se permettre d'être méga triviale, genre `href="{% url "auth_login" %}?next={{ request.build_absolute_uri }}"` (le gain en lisibilité outrepasse large le gain en robustesse de passer par urlparse, et le "next" est déjà hardcodé à plein endroits, pas besoin d'aller le chercher dans REDIRECT_FIELD_NAME) N'hésite pas à me dire si tu trouves cette idée mauvaise ou qu'il y a une complexité que j'ai raté 😄
yweber marked this conversation as resolved
@ -345,0 +346,4 @@
page.save()
resp = app.get('/', status=403)
assert 'Access denied to Home' in resp.text
assert 'You can try to authenticate yourself' in resp.text
Owner

Ça vaut le coup de taper un resp.click sur le lien de login, et vite fait vérifier qu'on tombe bien sur la page de connexion

Ça vaut le coup de taper un resp.click sur le lien de login, et vite fait vérifier qu'on tombe bien sur la page de connexion
yweber marked this conversation as resolved
yweber force-pushed wip/8122-private-page-custom-message from 896e484654 to 65ceb1f40b 2024-01-22 15:34:17 +01:00 Compare
yweber force-pushed wip/8122-private-page-custom-message from 65ceb1f40b to 49b781feb5 2024-01-22 16:01:13 +01:00 Compare
yweber force-pushed wip/8122-private-page-custom-message from 49b781feb5 to 8150737373 2024-01-22 16:39:18 +01:00 Compare
yweber force-pushed wip/8122-private-page-custom-message from 8150737373 to 48d540c52f 2024-01-22 16:42:39 +01:00 Compare
yweber force-pushed wip/8122-private-page-custom-message from 48d540c52f to ec9dec01d8 2024-01-22 16:50:54 +01:00 Compare
yweber requested review from vdeniaud 2024-01-22 16:53:55 +01:00
yweber reviewed 2024-01-22 16:56:23 +01:00
@ -547,0 +553,4 @@
context = {
'custom_message': error_msg,
'page': Page(title=_('Error - access denied'), template_name=page.template_name),
Author
Owner

Je ne sais pas si c'est une bonne idée de faire que la fausse page essaye d'utiliser le même thème que la page non-accessible ?

Je ne sais pas si c'est une bonne idée de faire que la fausse page essaye d'utiliser le même thème que la page non-accessible ?
Owner

Je ne suis pas sûr, ici on touche à mes limites de compréhension de combo :) je pense que tu peux mettre template_name='combo/private-page-error.html', l'hypothèse étant que c'est sûrement plus safe de ne pas mixer deux template différents

Je ne suis pas sûr, ici on touche à mes limites de compréhension de combo :) je pense que tu peux mettre `template_name='combo/private-page-error.html'`, l'hypothèse étant que c'est sûrement plus safe de ne pas mixer deux template différents
Owner

Je serais à dire de regarder ce qui est fait dans error404 et de faire pareil.

Je serais à dire de regarder ce qui est fait dans error404 et de faire pareil.
Author
Owner

Merci à vous deux !

Ok, je force l'utilisation du template 'standard'

Merci à vous deux ! Ok, je force l'utilisation du template 'standard'
Owner

Ok, je force l'utilisation du template 'standard'

Ça n'est pas tout ce que error404 fait; il y a 1/ vérifier qu'il y a des pages du site accessible et si ça n'est pas le cas retourner un gabarit minimal, pour les mêmes raisons que sur la page 404 (c'est suite à un audit de sécurité, cf #41514); puis 2/ prendre une page qui aurait comme slug "403" et si elle n'existe pas, partir sur la base de qui est à l'accueil (cf #43851, c'est pour avoir le pied de page).

> Ok, je force l'utilisation du template 'standard' Ça n'est pas tout ce que error404 fait; il y a 1/ vérifier qu'il y a des pages du site accessible et si ça n'est pas le cas retourner un gabarit minimal, pour les mêmes raisons que sur la page 404 (c'est suite à un audit de sécurité, cf #41514); puis 2/ prendre une page qui aurait comme slug "403" et si elle n'existe pas, partir sur la base de qui est à l'accueil (cf #43851, c'est pour avoir le pied de page).
Author
Owner

J'ai l'impression qu'on comprend le ticket de deux manières différentes :

  • j'avais compris que le but du ticket était d'avoir la possibilité d'ajouter un message (par page) à afficher si la page n'est pas accessible
  • je crois comprendre que tu me guide vers le fait d'implémenter une page d'erreur PermissionDenied configurable pour tout le portail (le message personnalisable serait une page avec "403" comme slug)

C'est bien ça ?

J'ai l'impression qu'on comprend le ticket de deux manières différentes : - j'avais compris que le but du ticket était d'avoir la possibilité d'ajouter un message (par page) à afficher si la page n'est pas accessible - je crois comprendre que tu me guide vers le fait d'implémenter une page d'erreur PermissionDenied configurable pour tout le portail (le message personnalisable serait une page avec "403" comme slug) C'est bien ça ?
Owner

Il me semble en effet que la direction prise répète des erreurs et questions déjà traitées pour la page 404; le point 1/ est important pour ne pas prochain audit se taper le même commentaire, le point 2/ relève davantage de l'anticipation des besoins, ne pas devoir défaire/refaire quand quelqu'un viendra souhaiter du gras ou un lien dans le texte personnalisé.

Il me semble en effet que la direction prise répète des erreurs et questions déjà traitées pour la page 404; le point 1/ est important pour ne pas prochain audit se taper le même commentaire, le point 2/ relève davantage de l'anticipation des besoins, ne pas devoir défaire/refaire quand quelqu'un viendra souhaiter du gras ou un lien dans le texte personnalisé.
Owner

Alors oui je m'éloigne ainsi du ticket, mais il ne donne pas de cas d'usage qui justifierait que le message soit différent par page et Benjamin y a lié le ticket #84346 qui correspond davantage à ma description.

Alors oui je m'éloigne ainsi du ticket, mais il ne donne pas de cas d'usage qui justifierait que le message soit différent par page et Benjamin y a lié le ticket #84346 qui correspond davantage à ma description.
Author
Owner

Super, merci :) Je pars sur cette implémentation 404 like.

Super, merci :) Je pars sur cette implémentation 404 like.
yweber marked this conversation as resolved
yweber force-pushed wip/8122-private-page-custom-message from ec9dec01d8 to 64cf65310e 2024-01-22 17:16:06 +01:00 Compare
vdeniaud reviewed 2024-01-22 17:56:39 +01:00
@ -0,0 +1,20 @@
{% extends "combo/page_template.html" %}
{% load i18n %}
{% block messages %}
Owner

Comme discuté sur jabber, plutôt que de faire les choses dans le bloc messages je te suggère

{% block combo-content %}
  <div>
    <div class="errornotice">{{ custom_message|linebreaks }}</div>

    {% if request.user.is_authenticated %}
      <p><a class="pk-button" href="...">{% trans "Login" %}</a></p>
    {% endif %}
  </div>
{% endblock %}

Outre le markup ça intègre des remarques que j'aurais pu faire par ailleurs :

  • ajout de |linebreaks pour supporter d'éventuels sauts de lignes dans le message
  • utilisation directe de request.user.is_authenticated
  • le lien s'affiche comme un bouton, de là je trouve ça pas beau d'avoir un bouton avec un long libellé (très subjectif), donc j'ai mis juste « Login » mais tu peux aussi garder le libellé actuel
Comme discuté sur jabber, plutôt que de faire les choses dans le bloc messages je te suggère ``` {% block combo-content %} <div> <div class="errornotice">{{ custom_message|linebreaks }}</div> {% if request.user.is_authenticated %} <p><a class="pk-button" href="...">{% trans "Login" %}</a></p> {% endif %} </div> {% endblock %} ``` Outre le markup ça intègre des remarques que j'aurais pu faire par ailleurs : * ajout de |linebreaks pour supporter d'éventuels sauts de lignes dans le message * utilisation directe de request.user.is_authenticated * le lien s'affiche comme un bouton, de là je trouve ça pas beau d'avoir un bouton avec un long libellé (très subjectif), donc j'ai mis juste « Login » mais tu peux aussi garder le libellé actuel
yweber marked this conversation as resolved
yweber force-pushed wip/8122-private-page-custom-message from 64cf65310e to d90ebf7ad7 2024-01-22 18:21:16 +01:00 Compare
yweber force-pushed wip/8122-private-page-custom-message from d90ebf7ad7 to 548b931abd 2024-01-23 09:34:58 +01:00 Compare
yweber changed title from data: add custom error message when Page display is not allowed (#8122) to WIP: data: add custom error message when Page display is not allowed (#8122) 2024-01-23 11:48:33 +01:00
yweber force-pushed wip/8122-private-page-custom-message from 548b931abd to e1323063e8 2024-01-23 11:48:39 +01:00 Compare
yweber changed title from WIP: data: add custom error message when Page display is not allowed (#8122) to WIP: public: change permission denied handling (#8122) 2024-01-23 11:49:31 +01:00
yweber force-pushed wip/8122-private-page-custom-message from e1323063e8 to 46ecf07ff0 2024-01-23 11:52:22 +01:00 Compare
yweber changed title from WIP: public: change permission denied handling (#8122) to public: change permission denied handling (#8122) 2024-01-23 12:02:53 +01:00
yweber reviewed 2024-01-23 12:04:49 +01:00
@ -0,0 +12,4 @@
{% trans "authenticate yourself using another account" %}
</a>.
</p>
{% else %}
Author
Owner

On ne devrait jamais tomber dans ce "else", c'est utile de le laisser ?

On ne devrait jamais tomber dans ce "else", c'est utile de le laisser ?
fpeters reviewed 2024-01-31 12:22:49 +01:00
@ -549,2 +553,3 @@
# be viewed, display generic django Permission Denied error
raise PermissionDenied()
return redirect_to_login(request.build_absolute_uri())
Owner

Si j'arrive à suivre correctement, ça ne me semble pas approprié, ça voudrait dire qu'un premier accès sur le portail agent afficherait une page d'erreur, plutôt qu'envoyer l'usager se connecter.

J'ai l'impression qu'en fait tout est confus entre les différentes directions, la description un peu courte du ticket, le ticket lié toulouse qui demande une page 403 différente, mes commentaires, etc.

Il faut peut-être prendre du recul et refaire une description fonctionnelle un peu exhaustive de ce qu'on veut atteindre, avant de continuer sur le code.

Si j'arrive à suivre correctement, ça ne me semble pas approprié, ça voudrait dire qu'un premier accès sur le portail agent afficherait une page d'erreur, plutôt qu'envoyer l'usager se connecter. J'ai l'impression qu'en fait tout est confus entre les différentes directions, la description un peu courte du ticket, le ticket lié toulouse qui demande une page 403 différente, mes commentaires, etc. Il faut peut-être prendre du recul et refaire une description fonctionnelle un peu exhaustive de ce qu'on veut atteindre, avant de continuer sur le code.
Author
Owner

Si j'arrive à suivre correctement, ça ne me semble pas approprié, ça voudrait dire qu'un premier accès sur le portail agent afficherait une page d'erreur, plutôt qu'envoyer l'usager se connecter.

Dans le cas ou aucune page n'est accessible oui, j'avais (mal) compris que c'était ce qui était attendu ;)

J'ai l'impression qu'en fait tout est confus entre les différentes directions, la description un peu courte du ticket, le ticket lié toulouse qui demande une page 403 différente, mes commentaires, etc.
Il faut peut-être prendre du recul et refaire une description fonctionnelle un peu exhaustive de ce qu'on veut atteindre, avant de continuer sur le code.

Je pense qu'on peut fermer cette PR en attendant une description fonctionnelle plus détaillée de ce qui est attendu/voulu ?

> Si j'arrive à suivre correctement, ça ne me semble pas approprié, ça voudrait dire qu'un premier accès sur le portail agent afficherait une page d'erreur, plutôt qu'envoyer l'usager se connecter. Dans le cas ou aucune page n'est accessible oui, j'avais (mal) compris que c'était ce qui était attendu ;) > J'ai l'impression qu'en fait tout est confus entre les différentes directions, la description un peu courte du ticket, le ticket lié toulouse qui demande une page 403 différente, mes commentaires, etc. > Il faut peut-être prendre du recul et refaire une description fonctionnelle un peu exhaustive de ce qu'on veut atteindre, avant de continuer sur le code. Je pense qu'on peut fermer cette PR en attendant une description fonctionnelle plus détaillée de ce qui est attendu/voulu ?
yweber changed title from public: change permission denied handling (#8122) to WIP: public: change permission denied handling (#8122) 2024-02-12 09:27:42 +01:00
yweber closed this pull request 2024-02-14 17:06:57 +01:00
All checks were successful
gitea/combo/pipeline/head This commit looks good

Pull request closed

Sign in to join this conversation.
No reviewers
No Label
No Milestone
No Assignees
4 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: entrouvert/combo#218
No description provided.