entrouvert
/
authentic
16
0
Fork 0
Code Issues Pull Requests 32 Releases Activity

a2_rbac: allow management of subclasses by manager roles (#77366) #54

Closed
pmarillonnet wants to merge 1 commits from wip/77366-authn-manager-oidc-claim-edit into main
pull from: wip/77366-authn-manager-oidc-claim-edit
merge into: entrouvert:main
Conversation 16 Commits 1 Files Changed 7 +60 -4
pmarillonnet commented 2023-05-09 11:05:52 +02:00
Owner
Copy Link
No description provided.
bdauvergne commented 2023-05-09 11:40:35 +02:00
Owner
Copy Link

Je vois ce que tu fais mais je me demande pourquoi le code suivant :

    title = _('Edit OpenID Claim')
    form_class = forms.OIDCClaimForm
    permissions = ['authentic2.admin_service']
    permission_model = OIDCClient
    permission_pk_url_kwarg = 'service_pk'

n'a pas l'effet escompté, c'est à dire ne vérifier que la permission admin_service sur l'objet OIDCClient, comment se retrouve-t-on à vérifier une permission sur OIDCClaim ?

Pour les modèles déclarés dans Authentiator.related_models ça parait normal, les vues génériques dans src/authentic2/apps/authenticators/views.py ne déclare par de permission, permission_model ou permission_pk_url_kwarg.

Je vois ce que tu fais mais je me demande pourquoi le code suivant : ```class OIDCClaimEditView(BaseClaimView, views.BaseEditView): title = _('Edit OpenID Claim') form_class = forms.OIDCClaimForm permissions = ['authentic2.admin_service'] permission_model = OIDCClient permission_pk_url_kwarg = 'service_pk' ``` n'a pas l'effet escompté, c'est à dire ne vérifier que la permission admin_service sur l'objet OIDCClient, comment se retrouve-t-on à vérifier une permission sur OIDCClaim ? Pour les modèles déclarés dans Authentiator.related_models ça parait normal, les vues génériques dans src/authentic2/apps/authenticators/views.py ne déclare par de permission, permission_model ou permission_pk_url_kwarg.
pmarillonnet force-pushed wip/77366-authn-manager-oidc-claim-edit from af5079fc91 to bc2bd2f48c 2023-05-09 12:16:06 +02:00 Compare
pmarillonnet force-pushed wip/77366-authn-manager-oidc-claim-edit from bc2bd2f48c to 58b8e9e7e8 2023-05-09 14:31:33 +02:00 Compare
pmarillonnet force-pushed wip/77366-authn-manager-oidc-claim-edit from 58b8e9e7e8 to 73fab16733 2023-05-09 14:41:21 +02:00 Compare
pmarillonnet changed title from WIP: a2_rbac: allow management of subclasses by manager roles (#77366) to a2_rbac: allow management of subclasses by manager roles (#77366) 2023-05-09 14:42:02 +02:00
pmarillonnet commented 2023-05-09 14:44:04 +02:00
Author
Owner
Copy Link

Je vois ce que tu fais mais je me demande pourquoi le code suivant :

    title = _('Edit OpenID Claim')
    form_class = forms.OIDCClaimForm
    permissions = ['authentic2.admin_service']
    permission_model = OIDCClient
    permission_pk_url_kwarg = 'service_pk'

n'a pas l'effet escompté, c'est à dire ne vérifier que la permission admin_service sur l'objet OIDCClient, comment se retrouve-t-on à vérifier une permission sur OIDCClaim ?

Pour les modèles déclarés dans Authentiator.related_models ça parait normal, les vues génériques dans src/authentic2/apps/authenticators/views.py ne déclare par de permission, permission_model ou permission_pk_url_kwarg.

Arf, je n’ai pas reçu de mail relatif à ton intervention ci-dessus dans la PR, que je découvre maintenant :
Je regarde.

> Je vois ce que tu fais mais je me demande pourquoi le code suivant : > > ```class OIDCClaimEditView(BaseClaimView, views.BaseEditView): > title = _('Edit OpenID Claim') > form_class = forms.OIDCClaimForm > permissions = ['authentic2.admin_service'] > permission_model = OIDCClient > permission_pk_url_kwarg = 'service_pk' > ``` > > n'a pas l'effet escompté, c'est à dire ne vérifier que la permission admin_service sur l'objet OIDCClient, comment se retrouve-t-on à vérifier une permission sur OIDCClaim ? > > Pour les modèles déclarés dans Authentiator.related_models ça parait normal, les vues génériques dans src/authentic2/apps/authenticators/views.py ne déclare par de permission, permission_model ou permission_pk_url_kwarg. Arf, je n’ai pas reçu de mail relatif à ton intervention ci-dessus dans la PR, que je découvre maintenant : Je regarde.
pmarillonnet commented 2023-05-09 15:00:30 +02:00
Author
Owner
Copy Link

Je vois ce que tu fais mais je me demande pourquoi le code suivant :

    title = _('Edit OpenID Claim')
    form_class = forms.OIDCClaimForm
    permissions = ['authentic2.admin_service']
    permission_model = OIDCClient
    permission_pk_url_kwarg = 'service_pk'

n'a pas l'effet escompté, c'est à dire ne vérifier que la permission admin_service sur l'objet OIDCClient, comment se retrouve-t-on à vérifier une permission sur OIDCClaim ?

Attention c’est coté auth_oidc que le souci se pose ici, pas idp_oidc.

Pour les modèles déclarés dans Authentiator.related_models ça parait normal, les vues génériques dans src/authentic2/apps/authenticators/views.py ne déclare par de permission, permission_model ou permission_pk_url_kwarg.

Oui c’est justement l’objet de la PR, de faire en sorte que les MANAGED_CT puissent déclarer des dépendances sur lesquelles des permissions d’administration doivent aussi
etre définies.

> Je vois ce que tu fais mais je me demande pourquoi le code suivant : > > ```class OIDCClaimEditView(BaseClaimView, views.BaseEditView): > title = _('Edit OpenID Claim') > form_class = forms.OIDCClaimForm > permissions = ['authentic2.admin_service'] > permission_model = OIDCClient > permission_pk_url_kwarg = 'service_pk' > ``` > > n'a pas l'effet escompté, c'est à dire ne vérifier que la permission admin_service sur l'objet OIDCClient, comment se retrouve-t-on à vérifier une permission sur OIDCClaim ? Attention c’est coté auth_oidc que le souci se pose ici, pas idp_oidc. > Pour les modèles déclarés dans Authentiator.related_models ça parait normal, les vues génériques dans src/authentic2/apps/authenticators/views.py ne déclare par de permission, permission_model ou permission_pk_url_kwarg. Oui c’est justement l’objet de la PR, de faire en sorte que les MANAGED_CT puissent déclarer des dépendances sur lesquelles des permissions d’administration doivent aussi etre définies.
aberriot reviewed 2023-05-09 15:32:48 +02:00
aberriot left a comment
Owner
Copy Link

J'ai testé en local et ça fonctionne comme attendu à l'édition et à la suppression d'un claim. Le code et le test me paraissent tout bon. J'ai essayé l'onglet « Ajout d'un rôle » mais ça plante avec le forbidden par contre, est-ce attendu ?

J'ai testé en local et ça fonctionne comme attendu à l'édition et à la suppression d'un claim. Le code et le test me paraissent tout bon. J'ai essayé l'onglet « Ajout d'un rôle » mais ça plante avec le forbidden par contre, est-ce attendu ?
pmarillonnet commented 2023-05-09 15:37:08 +02:00
Author
Owner
Copy Link

J'ai testé en local et ça fonctionne comme attendu à l'édition et à la suppression d'un claim. Le code et le test me paraissent tout bon. J'ai essayé l'onglet « Ajout d'un rôle » mais ça plante avec le forbidden par contre, est-ce attendu ?

Ah zut, en testant en local je ne rencontrais aucun souci à l’ajout d’un rôle avant ou après application des changements, pour un usager qui est administrateur des moyens d’authentification. Je double-vérifie, pour voir si j’ai loupé un truc au premier essai, et j’ajoute le test qui correspond à cela.

> J'ai testé en local et ça fonctionne comme attendu à l'édition et à la suppression d'un claim. Le code et le test me paraissent tout bon. J'ai essayé l'onglet « Ajout d'un rôle » mais ça plante avec le forbidden par contre, est-ce attendu ? Ah zut, en testant en local je ne rencontrais aucun souci à l’ajout d’un rôle avant ou après application des changements, pour un usager qui est administrateur des moyens d’authentification. Je double-vérifie, pour voir si j’ai loupé un truc au premier essai, et j’ajoute le test qui correspond à cela.
pmarillonnet force-pushed wip/77366-authn-manager-oidc-claim-edit from 73fab16733 to baeaeed333 2023-05-09 15:41:13 +02:00 Compare
pmarillonnet commented 2023-05-09 15:42:04 +02:00
Author
Owner
Copy Link

Ah zut, en testant en local je ne rencontrais aucun souci à l’ajout d’un rôle avant ou après application des changements, pour un usager qui est administrateur des moyens d’authentification. Je double-vérifie, pour voir si j’ai loupé un truc au premier essai, et j’ajoute le test qui correspond à cela.

Je ne reproduis pas :/
Et j’ai rajouté deux lignes dans les tests qui vont de le sens de ce que je constate en local.

> Ah zut, en testant en local je ne rencontrais aucun souci à l’ajout d’un rôle avant ou après application des changements, pour un usager qui est administrateur des moyens d’authentification. Je double-vérifie, pour voir si j’ai loupé un truc au premier essai, et j’ajoute le test qui correspond à cela. Je ne reproduis pas :/ Et j’ai rajouté deux lignes dans les tests qui vont de le sens de ce que je constate en local.
bdauvergne commented 2023-05-09 15:43:03 +02:00
Owner
Copy Link

Oui je me suis mélangé les pinceaux, tout marche bien coté idp_oidc vis à vis du rôle "Administration des services"; mais donc la correction coté auth_oidc devrait ressembler à la correction coté idp_oidc, passer par les paramètres de classe permission_model et permission_pk_url_kwargs pour ne pas inventer une nouvelle solution pour le même problème. À tester mais ce serait :

    permissions = 'authenticators.admin_baseauthenticator'
    permission_model = BaseAuthenticator
    permission_pk_url_kwargs = 'authenticator_pk'
Oui je me suis mélangé les pinceaux, tout marche bien coté idp_oidc vis à vis du rôle "Administration des services"; mais donc la correction coté auth_oidc devrait ressembler à la correction coté idp_oidc, passer par les paramètres de classe permission_model et permission_pk_url_kwargs pour ne pas inventer une nouvelle solution pour le même problème. À tester mais ce serait : ``` permissions = 'authenticators.admin_baseauthenticator' permission_model = BaseAuthenticator permission_pk_url_kwargs = 'authenticator_pk' ```
bdauvergne commented 2023-05-09 15:58:29 +02:00
Owner
Copy Link

Je confirme que ce patch corrige le problème (testé à l'instant sur un utilisateur n'ayant que le rôle "Administrateur des moyens d'authentification") :

diff --git a/src/authentic2/apps/authenticators/views.py b/src/authentic2/apps/authenticators/views.py
index 9209ddb9..c38b9504 100644
--- a/src/authentic2/apps/authenticators/views.py
+++ b/src/authentic2/apps/authenticators/views.py
@@ -270,7 +270,9 @@ order = AuthenticatorsOrderView.as_view()
 
 
 class AuthenticatorRelatedObjectMixin(MediaMixin, TitleMixin, PermissionMixin):
-    permissions = ['authenticators.search_baseauthenticator']
+    permissions = ['authenticators.admin_baseauthenticator']
+    permission_model = BaseAuthenticator
+    permission_pk_url_kwarg = 'authenticator_pk'
 
     def dispatch(self, request, *args, **kwargs):
         self.authenticator = get_object_or_404(

J'ai changé la permission de search à admin parce que ça me parait plus conforme à ce qui est voulu (search n'a pas trop de sens ici, mais view pourra permettre de consulter sans modifier si jamais ça avait un sens un jour).

Je confirme que ce patch corrige le problème (testé à l'instant sur un utilisateur n'ayant que le rôle "Administrateur des moyens d'authentification") : ``` diff --git a/src/authentic2/apps/authenticators/views.py b/src/authentic2/apps/authenticators/views.py index 9209ddb9..c38b9504 100644 --- a/src/authentic2/apps/authenticators/views.py +++ b/src/authentic2/apps/authenticators/views.py @@ -270,7 +270,9 @@ order = AuthenticatorsOrderView.as_view() class AuthenticatorRelatedObjectMixin(MediaMixin, TitleMixin, PermissionMixin): - permissions = ['authenticators.search_baseauthenticator'] + permissions = ['authenticators.admin_baseauthenticator'] + permission_model = BaseAuthenticator + permission_pk_url_kwarg = 'authenticator_pk' def dispatch(self, request, *args, **kwargs): self.authenticator = get_object_or_404( ``` J'ai changé la permission de search à admin parce que ça me parait plus conforme à ce qui est voulu (search n'a pas trop de sens ici, mais view pourra permettre de consulter sans modifier si jamais ça avait un sens un jour).
pmarillonnet commented 2023-05-09 16:01:52 +02:00
Author
Owner
Copy Link

Je confirme que ce patch corrige le problème (testé à l'instant sur un utilisateur n'ayant que le rôle "Administrateur des moyens d'authentification") :

Bon, merci d’avoir regardé. L’application du patch en local ne plait pas au test de mon côté, et d’ailleurs j’avais déjà testé un truc du genre avant de proposer le contenu de la PR, sans succès.
Il y a un truc qui m’échappe, en local ça échoue parce qu’il semblerait que l’usager, qui endosse pourtant le role d’administration des moyens d’authentification, ne détienne pas la permission 'authenticators.admin_baseauthenticator' sur l’objet fournisseur OIDC en particulier.
Je vais dérouler le code à tete reposée, là je n’y arrive plus.

> Je confirme que ce patch corrige le problème (testé à l'instant sur un utilisateur n'ayant que le rôle "Administrateur des moyens d'authentification") : Bon, merci d’avoir regardé. L’application du patch en local ne plait pas au test de mon côté, et d’ailleurs j’avais déjà testé un truc du genre avant de proposer le contenu de la PR, sans succès. Il y a un truc qui m’échappe, en local ça échoue parce qu’il semblerait que l’usager, qui endosse pourtant le role d’administration des moyens d’authentification, ne détienne pas la permission 'authenticators.admin_baseauthenticator' sur l’objet fournisseur OIDC en particulier. Je vais dérouler le code à tete reposée, là je n’y arrive plus.
aberriot commented 2023-05-09 16:20:54 +02:00
Owner
Copy Link

Il y a un truc qui m’échappe, en local ça échoue parce qu’il semblerait que l’usager, qui endosse pourtant le role d’administration des moyens d’authentification, ne détienne pas la permission 'authenticators.admin_baseauthenticator' sur l’objet fournisseur OIDC en particulier.

Est-ce qu'il n'y a pas une logique qui se produit au moment de la création/assignation du role et du coup c'est ce qui explique le plantage en local (l'agent existe déjà avec son rôle), là ou pour les tests, tout est créé sur le moment et ça passe ?

> Il y a un truc qui m’échappe, en local ça échoue parce qu’il semblerait que l’usager, qui endosse pourtant le role d’administration des moyens d’authentification, ne détienne pas la permission 'authenticators.admin_baseauthenticator' sur l’objet fournisseur OIDC en particulier. Est-ce qu'il n'y a pas une logique qui se produit au moment de la création/assignation du role et du coup c'est ce qui explique le plantage en local (l'agent existe déjà avec son rôle), là ou pour les tests, tout est créé sur le moment et ça passe ?
pmarillonnet commented 2023-05-09 16:27:00 +02:00
Author
Owner
Copy Link

Est-ce qu'il n'y a pas une logique qui se produit au moment de la création/assignation du role et du coup c'est ce qui explique le plantage en local (l'agent existe déjà avec son rôle), là ou pour les tests, tout est créé sur le moment et ça passe ?

Je ne suis pas sur de comprendre de quoi tu parles, mais à ma connaissance rien de tel non.
Par contre la mise à jour des permissions liées aux roles d’administration ne se fait qu’en handler de signal post migrate django.

> Est-ce qu'il n'y a pas une logique qui se produit au moment de la création/assignation du role et du coup c'est ce qui explique le plantage en local (l'agent existe déjà avec son rôle), là ou pour les tests, tout est créé sur le moment et ça passe ? Je ne suis pas sur de comprendre de quoi tu parles, mais à ma connaissance rien de tel non. Par contre la mise à jour des permissions liées aux roles d’administration ne se fait qu’en handler de signal post migrate django.
👍 1
aberriot commented 2023-05-09 16:34:47 +02:00
Owner
Copy Link

Je ne suis pas sur de comprendre de quoi tu parles, mais à ma connaissance rien de tel non. Par contre la mise à jour des permissions liées aux roles d’administration ne se fait qu’en handler de signal post migrate django.

Ah ben voilà, c'est ce qui manquait je pense, je viens de tester avec un migrate et tout fonctionne chez moi, la gestion de rôle aussi bien que les claims dans l'interface. C'est tout bon au niveau fonctionnel de mon côté.

> Je ne suis pas sur de comprendre de quoi tu parles, mais à ma connaissance rien de tel non. Par contre la mise à jour des permissions liées aux roles d’administration ne se fait qu’en handler de signal post migrate django. Ah ben voilà, c'est ce qui manquait je pense, je viens de tester avec un migrate et tout fonctionne chez moi, la gestion de rôle aussi bien que les claims dans l'interface. C'est tout bon au niveau fonctionnel de mon côté.
pmarillonnet commented 2023-05-09 16:37:46 +02:00
Author
Owner
Copy Link

Je ne suis pas sur de comprendre de quoi tu parles, mais à ma connaissance rien de tel non. Par contre la mise à jour des permissions liées aux roles d’administration ne se fait qu’en handler de signal post migrate django.

Ah ben voilà, c'est ce qui manquait je pense, je viens de tester avec un migrate et tout fonctionne chez moi, la gestion de rôle aussi bien que les claims dans l'interface. C'est tout bon au niveau fonctionnel de mon côté.

Ok, j’avais oublié de préciser cela oui, mes excuses.
La version de Benj a l’air infiniment plus simple, je vais reprendre au calme demain et voir ce que je loupe dans son application correcte en local pour faire passer le test.

> > Je ne suis pas sur de comprendre de quoi tu parles, mais à ma connaissance rien de tel non. Par contre la mise à jour des permissions liées aux roles d’administration ne se fait qu’en handler de signal post migrate django. > > Ah ben voilà, c'est ce qui manquait je pense, je viens de tester avec un migrate et tout fonctionne chez moi, la gestion de rôle aussi bien que les claims dans l'interface. C'est tout bon au niveau fonctionnel de mon côté. Ok, j’avais oublié de préciser cela oui, mes excuses. La version de Benj a l’air infiniment plus simple, je vais reprendre au calme demain et voir ce que je loupe dans son application correcte en local pour faire passer le test.
bdauvergne commented 2023-05-10 07:43:05 +02:00
Owner
Copy Link

Chez moi ça a marché tout de suite (mais je fais des migrate_schemas régulièrement sur mon tenant authentic.dev.publik.love c'est peut-être ça).

Chez moi ça a marché tout de suite (mais je fais des migrate_schemas régulièrement sur mon tenant authentic.dev.publik.love c'est peut-être ça).
pmarillonnet commented 2023-05-10 08:24:23 +02:00
Author
Owner
Copy Link

Chez moi ça a marché tout de suite (mais je fais des migrate_schemas régulièrement sur mon tenant authentic.dev.publik.love c'est peut-être ça).

Est-ce le test écrit dans cette PR qui marche avec la modification que tu proposes ? Si oui, peux-tu pousser une autre branche stp ? De mon côté rien n’y fait, pas compris comment le test pourrait passer avec juste cette modif’.

> Chez moi ça a marché tout de suite (mais je fais des migrate_schemas régulièrement sur mon tenant authentic.dev.publik.love c'est peut-être ça). Est-ce le test écrit dans cette PR qui marche avec la modification que tu proposes ? Si oui, peux-tu pousser une autre branche stp ? De mon côté rien n’y fait, pas compris comment le test pourrait passer avec juste cette modif’.
bdauvergne commented 2023-06-05 11:35:42 +02:00
Owner
Copy Link

J'ai poussé mon patch avec le test de Paul dans la PR #69, on peut fermer celle-ci.

J'ai poussé mon patch avec le test de Paul dans la PR #69, on peut fermer celle-ci.
pmarillonnet commented 2023-06-05 11:38:03 +02:00
Author
Owner
Copy Link

J'ai poussé mon patch avec le test de Paul dans la PR #69, on peut fermer celle-ci.

Yes nickel, j’ai validé celle-là et je ferme celle-ci.

> J'ai poussé mon patch avec le test de Paul dans la PR #69, on peut fermer celle-ci. Yes nickel, j’ai validé celle-là et je ferme celle-ci.
pmarillonnet closed this pull request 2023-06-05 11:38:07 +02:00
All checks were successful
gitea/authentic/pipeline/head This commit looks good
Details

Pull request closed

Please reopen this pull request to perform a merge.
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels
No items
No Label
Milestone
Clear milestone
No items
No Milestone
Assignees
Clear assignees
No Assignees
3 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/authentic#54
No description provided.
Delete Branch "wip/77366-authn-manager-oidc-claim-edit"

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?