Produits Entr'ouvert » Authentic 2

Patch 0001:

Plutôt que request, je ne passerai que user à l'appel authorize pour l'instant (on pourra passer d'autres choses en paramètres supplémentaires si un jour on ajoute des autorisations d'un autre type, par IP par exemple).

Ça c'est un poil lent et faux aussi (ça ne prend pas en compte l'héritage), si tu enlèves le '+' au paramètre related_name de la propriété authorized_roles et que tu le renommes en 'authorized_services', on pourra écrire au lieu de :

        for role in self.authorized_roles.all():
            if request.user.roles.filter(uuid=role.uuid).exists():
                return True

plutôt

        if request.user.roles_and_parents().filter(authorized_services=self).exists():
            return True

Patch 0002:

Comme unauthorized_url peut-être nulle (ce qui est bien) plutôt prévoir un retour vers la homepage d'authentic dans ce cas. Ne pas appeler ça 'redirect_to_unautorized' puisque ça ne redirige rien, plutôt unauthorized_view().

(Je le place ici pour idée mais ça aurait été sympa de plutôt utiliser une exception comme ceci dans Service.authorize() raise PermissionDenied(service=service) et de gérer dans un middleware la conversation vers l'affichage d'une vue, ça simplifierait le code à juste un appel à Service.authorize(user=user) )

Patch 0003:

Virer les corrections PEP8 aux espaces et sauts de ligne.

Le test d'autorisation devrait être fait avance le validate_ticket() sinon on peut contourner l'autorisation en passant soi même le numéro de ticket au service.
Le test manque aussi ligne 117, cas passant du SSO CAS quand on est déjà connecté.
Le mettrai aussi un test ligne 364 dans la vue de proxy CAS (un fois qu'on a trouvé le target_service, on compare avec l'utilisateur dans pgt.user).

Patch 0004:

Ok, même tests pour OIDC et SAML à faire.

Patch 0005:

À ce stade je ne changerai pas de comportement pour OIDC, on ne renvoie pas d'erreur au service OIDC, on affiche la même page que pour tout le monde. Si jamais on nous demandait de retournait l'erreur au service, je préfèrerai qu'on ajoute ça comme un choix possible (unauthorized_behaviour=return_to_service/show_error_page), mais on fera ça plus tard si le besoin se présente.

Patch 0006:

Vite fait,

• du texte d'explication pour dire que la gestion des rôles sur l'écran d'un service, c'est pour de la gestion d'accès, et qu'aucun rôle = service ouvert
• quand on ajoute un rôle à un service, il apparait dans le tableau, quand on clique dessus, 404.
• crash au premier essai,
  • service.authorize(request) vs def authorize(self, user)
  • → 'WSGIRequest' object has no attribute 'roles_and_parents'
  • → ajouter des tests pour les services saml et oidc, pas uniquement cas.

(nouvelle branche poussée par Josué, prenant en compte les remarques, sauf)

du texte d'explication pour dire que la gestion des rôles sur l'écran d'un service, c'est pour de la gestion d'accès, et qu'aucun rôle = service ouvert

Besoin de suggestions là-dessus ?

• → ajouter des tests pour les services saml et oidc, pas uniquement cas.

et oidc.

Frédéric Péters a écrit :

(nouvelle branche poussée par Josué, prenant en compte les remarques, sauf)

du texte d'explication pour dire que la gestion des rôles sur l'écran d'un service, c'est pour de la gestion d'accès, et qu'aucun rôle = service ouvert

Besoin de suggestions là-dessus ?

Volontier.

• → ajouter des tests pour les services saml et oidc, pas uniquement cas.

et oidc.

Je suis en train de le faire et j'ai une AssetionError sur un response.form.submit().follow() et je cherche le pourquoi.
D'ou l'absence dans la branche.

Un message qui accompagne le AssertionError ou une trace?

Aussi, Authentic affiche sur sa page d'accueil la liste des services avec des boutons de connexion, il faudrait en exclure les services auxquels l'usager n'a pas accès. (cette page n'est pas affichée en mode hobo/publik).

Sur la page d'un service actuellement il y a déjà un tableau de rôles (qui serait les administrateurs du service); ce tableau a disparu.

Sur la page d'un service actuellement il y a déjà un tableau de rôles (qui serait les administrateurs du service); ce tableau a disparu.

Disparu/remplacé, par :

     def get_table_queryset(self):
-        return self.object.roles.all()
+        return self.object.authorized_services.all()

C'est peut-être compliqué avec la structure actuelle d'authentic d'avoir deux tableaux sur la même page, l'idée serait alors du coup d'avoir deux pages différentes, une avec les tableaux des rôles du service (object.roles) et l'autre les rôles autorisés à s'y connecter (object.authorized_services).

Au passage, vu que dans .authorized_services on n'a pas des services mais des rôles, c'est plutôt mal nommé.

Au passage, vu que dans .authorized_services on n'a pas des services mais des rôles, c'est plutôt mal nommé.

Reste d'actualité.

Le tableau vide affiche "aucun" alors que pour les rôles autorisés à se connecter, le tableau vide signifie "tout le monde", non ?

Maintenant qu'il y a deux tableaux sur la même page, il faudrait également ajouter un titre au-dessus du premier pour expliciter son contenu.

Le fonctionnement à deux tableaux, avec la recherche/filtre qui s'applique uniquement à un seul, c'est étrange. Ma préférence déjà exprimée de manière plus générale dans un autre ticket (#14452) ce serait une uniformisation des page du /manage/, page de vue qui affiche les infos, puis sous-pages pour d'autres aspects. À voir si on veut caler ça dans ce ticket aussi. (si ça n'est pas le cas il faudrait malgré tout arranger ça rapidement après).

Le message de la popup "Do you really want to remove role "XXX" from service "YYY" ?", il devrait être adapté au contrôle d'accès.

Aussi, Authentic affiche sur sa page d'accueil la liste des services avec des boutons de connexion, il faudrait en exclure les services auxquels l'usager n'a pas accès. (cette page n'est pas affichée en mode hobo/publik).

Toujours d'application.

"page d'accueil" → "/". Hobo pose A2_HOMEPAGE_URL pour une redirection donc on ne voit pas la page d'accueil; faut taper None dans ce paramètre de config pour la voir.

Frédéric Péters a écrit :

"page d'accueil" → "/". Hobo pose A2_HOMEPAGE_URL pour une redirection donc on ne voit pas la page d'accueil; faut taper None dans ce paramètre de config pour la voir.

Merci.
Avec ce commit http://git.entrouvert.org/authentic.git/commit/?h=wip/access-control-15456&id=54af7540c5e60776caa787234b4b2b7d92a0e53f ça devrait etre bon normalement.

(commentaire numéro 12 toujours d'application)

Sur le fond, aussi, je préférerais que ça soit tourné avec une fonction qui fasse la vérification d'accès, et dans les situations de contrôle d'accès que l'exception soit levée, plutôt que l'inverse avec l'exception tout le temps levée et une fonction de vérification d'accès basée dessus :

+def is_user_authorized(service, request):
+    try:
+        service.authorize(request.user)
+        return True
+    except ServiceAccessDenied:
+        return False

Et sur le style, il continue à y avoir quantité de chunks inutiles, notamment :

-    LibertySession, LibertyFederation, 
+    LibertySession, LibertyFederation,

     http_method_names = ['get']
-    
+
     def get(self, request):

+
+
 def test_invalid_request(oidc_settings, oidc_client, simple_user, app):

Josué Kouka a écrit :

Frédéric Péters a écrit :

"page d'accueil" → "/". Hobo pose A2_HOMEPAGE_URL pour une redirection donc on ne voit pas la page d'accueil; faut taper None dans ce paramètre de config pour la voir.

Merci.
Avec ce commit http://git.entrouvert.org/authentic.git/commit/?h=wip/access-control-15456&id=54af7540c5e60776caa787234b4b2b7d92a0e53f ça devrait etre bon normalement.

La homepage est déjà très lente pour le cas d'usage "Fédération Renater" (des centaines de SPs), ça ne va pas arranger les choses. Sachant que c'est 95% du cas d'usage de cette page actuellement (on la voit aussi chez Vinci, mais il n'y a que quelques dizaines de SPs).

Et donc aussi parce je ne dois pas uniquement faire mes remarques en Jabber: on va garder les deux tableaux pour l'instant et ne pas prolonger ce ticket trop longtemps, par contre je suis pour inverser leur order, celui des autorisation devenant bien plus important.

Le premier pourrait donc s'appeler 'Roles of users allowed on this service' (c'est long mais c'est explicite et ça se traduit donc assez bien) et le deuxième 'Roles solely visible from this service'.

• Les titres des tableaux ont été mis à jour
• Il n'y a plus de filtrage des SP

Branche mise à jour.

Sans prendre en compte mes commentaires (quick check sur .authorised_services).

Il n'y a plus de filtrage des SP

Je note mon regret.

Frédéric Péters a écrit :

Branche mise à jour.

Sans prendre en compte mes commentaires (quick check sur .authorised_services).

Il n'y a plus de filtrage des SP

Je note mon regret.

Je me suis mal exprimé, c'est lent comme c'est ici mais ça peut être plus rapide, il suffit d'intégrer le filtrage à la requête

qs = qs.filter(Q(authorized_services__isnull=True)|Q(authorized_services__in=user.roles_and_parents()))

idéalement plutôt que d'écrire ce code ici il faudrait une méthode authorized_for_user() sur le Manager du modèle Service, qu'on puisse modifier le code si de nouvelles conditions apparaissaient dans le futur.

Et je suis d'accord avec le fait que authorized_services c'est mal nommé.

La branches est mise à jour.

1. Typo: callback url when unathorized

2. context = {'callback_url': service.unauthorized_url or '/'} On est jamais sûr que '/' existe, utilise reverse('a2-homepage') à la place.

3. Merge raise ServiceAccessDenied when user not authorized (#15456) dans add authorized roles and unauthorized url field to Service et déplace add ServiceAccessMiddleware et add redirect to unauthorized page function (#15456) avant

4. correction de blancs/sauts de ligne inutiles dans cas: check if user is authorized through the client (#15456) (relire les patchs, git log -p), y en a d'autres, si tu peux repasser sur chaque commit (git rebase -i master), faire un git gui pour séparer ces modifications PEP8 et ensuite les rebaser toutes ensembles

Remarques assez cosmétiques, ça m'a l'air tout bon sinon.

Parce que Josué a perdu l'adresse du redmine, je note ici que la branche a été mise à jour pour suivre les remarques de Benjamin. (je n'ai pour ma part pas vériifé).

+    authorized_roles = models.ManyToManyField(
+        get_role_model_name(), verbose_name=_('authorized services'),

verbose_name pas modifié.

4. correction de blancs/sauts de ligne inutiles

Écrivais Benjamin et c'est toujours le cas.

Le tableau vide affiche "aucun" alors que pour les rôles autorisés à se connecter, le tableau vide signifie "tout le monde", non ?

Toujours d'application. S'il faut une suggestion, on pourrait par exemple mettre "Aucune restriction d'accès, tous les utilisateurs peuvent se connecter à ce service" ("No access restriction. All users are allowed to connect to this service.").

J'ai dit à Josué de pousser mais ta dernière remarque n'était pas traité il va poser un patch supplémentaire aussi.