Produits Entr'ouvert » w.c.s.

C'est tout simple.

Le plus dur les tests : j'étais parti sur de grande ambitions de factorisation (via des fixture) des tests existants sur l'authentification de l'API, dans l'idée qu'ensuite je n'aurais qu'a ajouter quelques lignes de code là dedans, pour qu'une multitude de tests tournent sur l'authentification via api client.

Mais marche trop haute, tout à la poubelle, et écriture de tests a part.

Je ne relis pas encore, j'attendrai une réponse aux questions de Fred.

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

J'aimerais ne pas voir l'introduction d'une troisième méthode. (le module "responses")

On peut négocier ?

Je trouve qu'il y a vraiment pas photo en termes de facilité d'usage et de lisibilité. Ci-dessous le premier tests avec et sans responses pour comparaison :

@responses.activate
def test_idp_http_error(app, pub, role):
    # No authentication : 403
    app.get('/api/forms/', status=403)
    app.set_authorization(('Basic', ('test-client', '12345')))
    # check-api-client returns 403
    responses.add(
        responses.POST,
        'https://authentic.example.invalid/api/check-api-client/',
        status=403,
    )
    app.get('/api/forms/', status=403)
    # check api call to authentic
    assert len(responses.calls) == 1
    request = responses.calls[0].request
    assert request.headers['Accept'] == 'application/json'
    assert request.headers['Content-Type'] == 'application/json'
    assert json.loads(request.body) == {'identifier': 'test-client', 'password': '12345'}

def test_idp_http_error(app, pub, role):
    # No authentication : 403
    app.get('/api/forms/', status=403)
    app.set_authorization(('Basic', ('test-client', '12345')))
    # check-api-client returns 403
    with mock.patch('wcs.qommon.misc._http_request') as http_request:
        http_request.return_value = None, 403, None, None
        app.get('/api/forms/', status=403)
        # check api call to authentic
        assert http_request.call_count == 1
        assert http_request.call_args[0][0].startswith(
            'https://authentic.example.invalid/api/check-api-client/'
        )
        assert http_request.call_args[0][1] == 'POST'
        assert http_request.call_args[1]['headers']['Accept'] == 'application/json'
        assert http_request.call_args[1]['headers']['Content-type'] == 'application/json'
        assert json.loads(http_request.call_args[0][2]) == {'identifier': 'test-client', 'password': '12345'}

Pour info c'est déjà introduit dans passerelle.

Plus fondamentalement, je suis un peu gêné par cette introduction APIClientUser

Voilà.

(oui ça demande d'aussi gérer has_anonymised_data_api_restriction)

Pas bien compris, tu vois des ajustements en plus à faire par rapport à cette deuxième version du patch ?

(mais ça a été introduit avec une justification (assert quelque chose) qui ne tient pas ici).

J'ai tenté de respecter le dogme que je ne partage pas, (...)

Le "dogme" c'est ne pas multiplier les manières de faire, je pense ça critique pour garder quelque chose de maintenable et accessible à tout le monde.

J'entends que tu ne penses pas ça et aussi qu'aller modifier des dizaines d'occurence de with mock.patch('wcs.qommon.misc.urlopen') n'est pas super fun.

Avec l'utilisation de with responses.RequestsMock() as rsps: plut@t que @responses.activate pour être raccord avec la forme commune.

Ok, une fois qu'on a le tout correct (ui côté authentic etc.) il faudra aussi un ticket pour ne plus exposer la création d'accès API via wcs, qu'une seule méthode soit exposée.

commit 1214963e728d9387488e0cfb82aef69181a20084
Author: Emmanuel Cazenave <ecazenave@entrouvert.com>
Date:   Mon Aug 29 14:22:08 2022 +0200

    api: extend api accesses with idp (#68337)