entrouvert
/
combo
16
1
Fork 0
Code Issues Pull Requests 10 Releases Wiki Activity

WIP: public: add error403 view and make publish_page use it (#8122) #234

Draft
yweber wants to merge 1 commits from wip/8122-error403-view into main
pull from: wip/8122-error403-view
merge into: entrouvert:main
Conversation 4 Commits 1 Files Changed 7 +131 -80
yweber commented 2024-02-14 17:10:49 +01:00
Owner
Copy Link
No description provided.
yweber added 1 commit 2024-02-14 17:10:50 +01:00
gitea/combo/pipeline/head There was a failure building this commit Details
66a5ea7539 public: add error403 view and make publish_page use it (#8122)
yweber force-pushed wip/8122-error403-view from 66a5ea7539 to 2f0eea4624 2024-02-15 14:42:30 +01:00 Compare
yweber force-pushed wip/8122-error403-view from 2f0eea4624 to 6f10d2b8c7 2024-02-15 14:50:54 +01:00 Compare
yweber force-pushed wip/8122-error403-view from 6f10d2b8c7 to 5c0dfef841 2024-02-15 14:55:48 +01:00 Compare
bdauvergne requested changes 2024-04-11 00:23:08 +02:00
bdauvergne left a comment
Owner
Copy Link

Tout ça m'a l'air ok mais j'ai l'impression qu'on pourrait ne pas créer PagePermissionDenied, et seulement adapter ajax_page_cell et skeleton qui sont des vues JSON et devraient renvoyer une 403 avec un contenu JSON de toute façon, sans passer par l'exception PermissionDenied.

ImageAddView et DashboardAddTileView sont biens des vues HTML et donc le comportement générique dans error403 leur conviendrait.

Tout ça m'a l'air ok mais j'ai l'impression qu'on pourrait ne pas créer PagePermissionDenied, et seulement adapter ajax_page_cell et skeleton qui sont des vues JSON et devraient renvoyer une 403 avec un contenu JSON de toute façon, sans passer par l'exception PermissionDenied. ImageAddView et DashboardAddTileView sont biens des vues HTML et donc le comportement générique dans error403 leur conviendrait.
yweber commented 2024-04-11 10:16:04 +02:00
Author
Owner
Copy Link

Tout ça m'a l'air ok mais j'ai l'impression qu'on pourrait ne pas créer PagePermissionDenied, et seulement adapter ajax_page_cell et skeleton qui sont des vues JSON et devraient renvoyer une 403 avec un contenu JSON de toute façon, sans passer par l'exception PermissionDenied.

Ok ! Je vais essayer de faire ça :)

ImageAddView et DashboardAddTileView sont biens des vues HTML et donc le comportement générique dans error403 leur conviendrait.

C'est quoi ce que tu désigne par le comportement par défaut ? return permission_denied(request, *args, **kwargs) ?

> Tout ça m'a l'air ok mais j'ai l'impression qu'on pourrait ne pas créer PagePermissionDenied, et seulement adapter ajax_page_cell et skeleton qui sont des vues JSON et devraient renvoyer une 403 avec un contenu JSON de toute façon, sans passer par l'exception PermissionDenied. Ok ! Je vais essayer de faire ça :) > ImageAddView et DashboardAddTileView sont biens des vues HTML et donc le comportement générique dans error403 leur conviendrait. C'est quoi ce que tu désigne par le comportement par défaut ? `return permission_denied(request, *args, **kwargs)` ?
yweber commented 2024-04-11 10:49:43 +02:00
Author
Owner
Copy Link

Hmm, je pense avoir mieux compris l'idée (j'ai mis un peu de temps à me réveiller ;) ).

En gros, ce que tu propose :

  • utiliser PermissionDenied avec le comportement de error403 (rediriger si pas logué, sinon afficher la page dédiée)
  • utiliser autre chose pour les "vues JSON" ajax_cell_page & skeleton (genre return HttpResponse('"permission denied"', status=403) )

Ca me va, je peux faire ça :)

Ma seule réserver serait qu'après, un des comportement de PermissionDenied serait de renvoyer une 302.

Est-ce qu'une solution acceptable serait :

  • utiliser une exception PagePermissionDenied (ou TryAuthPermissionDenied) quand on veut utiliser l'implémentation proposée de error403
  • implémenter une exception JsonApiPermissionDenied (qui renvoie une 403 et du JSON)
  • laisser PermissionDenied renvoyer le permission_denied(request, ...) par défaut

De cette manière on assure que quand on utilise "simplement" PermissionDenied il ne se passe rien de fou : on a juste le comportement par défaut de django.

Hmm, je pense avoir mieux compris l'idée (j'ai mis un peu de temps à me réveiller ;) ). En gros, ce que tu propose : - utiliser PermissionDenied avec le comportement de error403 (rediriger si pas logué, sinon afficher la page dédiée) - utiliser autre chose pour les "vues JSON" ajax_cell_page & skeleton (genre `return HttpResponse('"permission denied"', status=403)` ) Ca me va, je peux faire ça :) Ma seule réserver serait qu'après, un des comportement de PermissionDenied serait de renvoyer une 302. Est-ce qu'une solution acceptable serait : - utiliser une exception PagePermissionDenied (ou TryAuthPermissionDenied) quand on veut utiliser l'implémentation proposée de error403 - implémenter une exception JsonApiPermissionDenied (qui renvoie une 403 et du JSON) - laisser PermissionDenied renvoyer le `permission_denied(request, ...)` par défaut De cette manière on assure que quand on utilise "simplement" PermissionDenied il ne se passe rien de fou : on a juste le comportement par défaut de django.
bdauvergne commented 2024-04-11 12:14:27 +02:00
Owner
Copy Link

Hmm, je pense avoir mieux compris l'idée (j'ai mis un peu de temps à me réveiller ;) ).

Tu as raison sur un point, les deux vues ImageAdd et AddTile sont appelés en ajax pour rendu dans des popups/dialogue et le fait de retourner une 302 quand on est pas connecté va éventuellement produire des choses bizarres, mais pas pire que ce que ça doit faire actuellement, pour AddTile ça ne doit rien faire je pense, parce que le JS attend un retour JSON et ne l'aura pas ces deux vues ne devraient juste jamais être appelées sans être connecté, ça n'arrive que si on est déjà connecté et que la session expire.

En gros, ce que tu propose :

  • utiliser PermissionDenied avec le comportement de error403 (rediriger si pas logué, sinon afficher la page dédiée)
  • utiliser autre chose pour les "vues JSON" ajax_cell_page & skeleton (genre return HttpResponse('"permission denied"', status=403) )

DashboardAddTileView est aussi une vue JSON maintenant que je re-regarde son code, même traitement. Il ne reste que ImageAddView qui est utilisé dans une popup dans le /manage/, on aura un souci si on est connecté, dans le manager et que la session expire et qu'on clique sur le bouton pour ajouter une image, la popup contiendra vraisemblablement la page de login d'authentic.

Ca me va, je peux faire ça :)

Ma seule réserver serait qu'après, un des comportement de PermissionDenied serait de renvoyer une 302.

Est-ce qu'une solution acceptable serait :

  • utiliser une exception PagePermissionDenied (ou TryAuthPermissionDenied) quand on veut utiliser l'implémentation proposée de error403
  • implémenter une exception JsonApiPermissionDenied (qui renvoie une 403 et du JSON)

Ce qui me gêne c'est que c'est déjà le comportement natif si une vue JSON est implémenté via DRF, tant qu'à faire quelque chose j'aurai utilisé DRF au pire dans son mode fonction avec le décorateur @api_view (https://www.django-rest-framework.org/api-guide/views/#api_view).

De cette manière on assure que quand on utilise "simplement" PermissionDenied il ne se passe rien de fou : on a juste le comportement par défaut de django.

Dans tous les cas d'un permission denied qu'on renvoie à un navigateur et qu'on est pas connecté on veut se logger, le souci ici c'est l'implémentation des popups qui ne permet pas ça et le fait de faire des vues JSON à la main.

Mais si tu veux faire simple, le besoin toulouse ne demande ce comportement que dans publish_page et rien d'autre, mais je trouvais bien que ce soit général et qu'on ait plus à y penser.

> Hmm, je pense avoir mieux compris l'idée (j'ai mis un peu de temps à me réveiller ;) ). Tu as raison sur un point, les deux vues ImageAdd et AddTile sont appelés en ajax pour rendu dans des popups/dialogue et le fait de retourner une 302 quand on est pas connecté va éventuellement produire des choses bizarres, mais pas pire que ce que ça doit faire actuellement, pour AddTile ça ne doit rien faire je pense, parce que le JS attend un retour JSON et ne l'aura pas ces deux vues ne devraient juste jamais être appelées sans être connecté, ça n'arrive que si on est déjà connecté et que la session expire. > > En gros, ce que tu propose : > - utiliser PermissionDenied avec le comportement de error403 (rediriger si pas logué, sinon afficher la page dédiée) > - utiliser autre chose pour les "vues JSON" ajax_cell_page & skeleton (genre `return HttpResponse('"permission denied"', status=403)` ) DashboardAddTileView est aussi une vue JSON maintenant que je re-regarde son code, même traitement. Il ne reste que ImageAddView qui est utilisé dans une popup dans le /manage/, on aura un souci si on est connecté, dans le manager et que la session expire et qu'on clique sur le bouton pour ajouter une image, la popup contiendra vraisemblablement la page de login d'authentic. > Ca me va, je peux faire ça :) > > Ma seule réserver serait qu'après, un des comportement de PermissionDenied serait de renvoyer une 302. > > Est-ce qu'une solution acceptable serait : > - utiliser une exception PagePermissionDenied (ou TryAuthPermissionDenied) quand on veut utiliser l'implémentation proposée de error403 > - implémenter une exception JsonApiPermissionDenied (qui renvoie une 403 et du JSON) Ce qui me gêne c'est que c'est déjà le comportement natif si une vue JSON est implémenté via DRF, tant qu'à faire quelque chose j'aurai utilisé DRF au pire dans son mode fonction avec le décorateur @api_view (https://www.django-rest-framework.org/api-guide/views/#api_view). > De cette manière on assure que quand on utilise "simplement" PermissionDenied il ne se passe rien de fou : on a juste le comportement par défaut de django. Dans tous les cas d'un permission denied qu'on renvoie à un navigateur et qu'on est pas connecté on veut se logger, le souci ici c'est l'implémentation des popups qui ne permet pas ça et le fait de faire des vues JSON à la main. Mais si tu veux faire simple, le besoin toulouse ne demande ce comportement que dans publish_page et rien d'autre, mais je trouvais bien que ce soit général et qu'on ait plus à y penser.
yweber commented 2024-04-11 16:03:27 +02:00
Author
Owner
Copy Link

Il ne reste que ImageAddView qui est utilisé dans une popup dans le /manage/, on aura un souci si on est connecté, dans le manager et que la session expire et qu'on clique sur le bouton pour ajouter une image, la popup contiendra vraisemblablement la page de login d'authentic.

Ok, ça n'a pas l'air très problématique.

Ce qui me gêne c'est que c'est déjà le comportement natif si une vue JSON est implémenté via DRF, tant qu'à faire quelque chose j'aurai utilisé DRF au pire dans son mode fonction avec le décorateur @api_view (https://www.django-rest-framework.org/api-guide/views/#api_view).

Ok ! J'ai pu utiliser DRF pour les vues DashboardAddTileView & DashboardRemoveTileView :)

Mais du coup en essayant de faire la même pour skeleton & ajax_cell_page je me suis rendu compte que les deux renvoies du HTML (et dans le commentaire de skeleton c'est écrit qu'elle renverra une 403 en cas de problème). Peut être que la solution simple dans ces deux cas est de remplacer les raise PermissionDenied() par des return permission_denied(request) ?

Dans tous les cas d'un permission denied qu'on renvoie à un navigateur et qu'on est pas connecté on veut se logger

D'accord, il me semble qu'il y a une série de tests a modifier du coup (qui attendent des 403 et qui vont prendre des 302 à la place)

le souci ici c'est [...] fait de faire des vues JSON à la main.

J'essayerai de jeter un oeil voir si j'en trouve d'autres à implémenter avec DRF :)

> Il ne reste que ImageAddView qui est utilisé dans une popup dans le /manage/, on aura un souci si on est connecté, dans le manager et que la session expire et qu'on clique sur le bouton pour ajouter une image, la popup contiendra vraisemblablement la page de login d'authentic. Ok, ça n'a pas l'air très problématique. > Ce qui me gêne c'est que c'est déjà le comportement natif si une vue JSON est implémenté via DRF, tant qu'à faire quelque chose j'aurai utilisé DRF au pire dans son mode fonction avec le décorateur @api_view (https://www.django-rest-framework.org/api-guide/views/#api_view). Ok ! J'ai pu utiliser DRF pour les vues DashboardAddTileView & DashboardRemoveTileView :) Mais du coup en essayant de faire la même pour skeleton & ajax_cell_page je me suis rendu compte que les deux renvoies du HTML (et dans le commentaire de skeleton c'est écrit qu'elle renverra une 403 en cas de problème). Peut être que la solution simple dans ces deux cas est de remplacer les `raise PermissionDenied()` par des `return permission_denied(request)` ? > Dans tous les cas d'un permission denied qu'on renvoie à un navigateur et qu'on est pas connecté on veut se logger D'accord, il me semble qu'il y a une série de tests a modifier du coup (qui attendent des 403 et qui vont prendre des 302 à la place) > le souci ici c'est [...] fait de faire des vues JSON à la main. J'essayerai de jeter un oeil voir si j'en trouve d'autres à implémenter avec DRF :)
yweber force-pushed wip/8122-error403-view from 5c0dfef841 to f81d9652be 2024-04-11 16:46:17 +02:00 Compare
yweber force-pushed wip/8122-error403-view from f81d9652be to f489b737de 2024-04-11 16:51:54 +02:00 Compare
yweber requested review from bdauvergne 2024-04-11 17:13:24 +02:00
All checks were successful
gitea/combo/pipeline/head This commit looks good
Details
This pull request has changes conflicting with the target branch.
  • combo/urls.py
Sign in to join this conversation.
Reviewers
No reviewers
bdauvergne
Labels
Clear labels
No items
No Label
Milestone
Clear milestone
No items
No Milestone
Assignees
Clear assignees
No Assignees
2 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#234
No description provided.
Delete Branch "wip/8122-error403-view"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?