shred / acme4j

Java client for ACME (Let's Encrypt)
https://acme4j.shredzone.org
Apache License 2.0
510 stars 94 forks source link

a more mockable api would be a nice to have #64

Open deanhiller opened 6 years ago

deanhiller commented 6 years ago

This may be worth reading before my below comment https://blog.twitter.com/engineering/en_us/topics/insights/2017/the-testing-renaissance.html

One gold standard for making an api that is testable is following these rules for an api

I know it is a long shot, BUT it would rock if you released an api like that on top of your existing api.

Instead for now, I had to create an AcmeClientProxy which has a ton of untestable code due to the api of acme4j :(.

There are some things that can be considered just data like X509Cert, but there are way too many things like Order that have connect methods and such in them making it really confusing as to know when the library itself is going remote.

In fact, all twitter clients(100%) that go remote are

"public Dto method(Dto request)" making them testable AND very clear as to when it goes remote(ie. if I call the method, it goes remote).

thanks, Dean

shred commented 6 years ago

Thank you for your feedback, Dean!

The DTO/DAO pattern is my favorite one for these kind of clients, too. And actually, acme4j before version 0.6 was designed like that.

However, the ACME protocol is somewhat special. There is a nonce that comes from the response of the previous request, and must be presented in the next one. Also the account's keypair must be present all the time, to sign the requests. So in every service method, besides the resource DTO, also a session and account object had to be passed in. Due to the nature of DTOs, it was also unclear what properties needed to be set, and what properties held an actual result when the service returned a DTO.

At the bottom line, the old API required too much background knowledge of the ACME protocol. It was difficult to use, felt very low level, and was prone to bugs. The only advantage probably was that it was easy to write unit tests for it. :smile:

One major goal of acme4j is to present a concise API that helps the user to get a certificate, without needing to worry about all the ACME stuff happening in the background. This is why I decided to move to a classic domain model design in version 0.6.

But I agree that it is very difficult to write unit tests now. It shouldn't be like that.

I think putting a DTO/DAO API on top is not the right approach. As mentioned above, I have been there, and I didn't like the result at all.

The resource objects use the org.shredzone.acme4j.connector.Connection interface for communicating with the ACME server. So one way would be to write a MockConnection that emulates the communication with the server. This could be done by a MockProvider that is especially made for unit testing. One advantage is that, since you are using the actual acme4j service methods, you would get results from acme4j in your unit tests that are close to real world results.

What do you think about it?

shred commented 5 years ago

Now that the ACME protocol is finalized, I am actively working on this issue. There will be a mock framework available with the next acme4j version.

shred commented 4 years ago

Current status: The mock framework can already be used for simple tests that end in successful issuance of a certificate. I am currently working on extending the framework so it can also simulate all kind of error situations and "in progress" states. However, it will take some more time. :disappointed:

I have removed the mock module from the master branch for now, and relocated it to the feature/mock branch. It was a mistake to add the module to the master branch in an early stage, as it was blocking continuous acme4j updates for too long now.

I will rebase the feature branch frequently, so the mock module won't fall behind the master branch. Feel free to use the mock module if it's okay for you that it is experimental.

stefanofornari commented 1 week ago

I have ran into this thread just now. I had a look at the mock branch, it may be some good stuff, but I think the current unit tests have an easier approach that I like more. At the end is just about stubbing the provider and the connection objects. It can be generalized giving AcmeProviderStub a queue of responses for more complex scenarios like a full renew round-trip. I am doing it in my separate project but it would be great to create an artifact in here as per #162

shred commented 1 week ago

The mock branch grew very complex, as I tried to fully simulate a full mock CA. I also doubt that it can be rebased to the current master without having to solve many merge conflicts.

I think it would be a good approach to start with a simple acme4j-test module that just helps to build unit tests with a simple unit test test provider. The CA behavior could then be mocked by the tester, e.g. with the Mockito framework. Would that be helpful?

stefanofornari commented 1 week ago

yes very helpful. with a couple of flexible stubs (for the provider and connection) you do not even need to use mokito at all. The way I am doing it right now looks like the following:

Session session = new Session("acmetest:renew://cacert1.com");

based on the given uri the stub takes ownership, checks the subschema (renew above) and looks up for some configuration in the accept() method:

public boolean accepts(final URI uri) {
        if (uri == null) {
            return false;
        }
        final String scheme = uri.getScheme();

        if ((scheme == null) || !scheme.equals(SCHEME)) {
            return false;
        }

        //
        // Scheme is here accepted. If a subscheme is provided, it is used to
        // load this provider configuration from the resources
        //
        final String schemeSpecific = uri.getSchemeSpecificPart();
        final int pos = schemeSpecific.indexOf("://");
        if (pos > 0) {
            //
            // we have a subscheme
            //
            JSON stubs = TestUtils.getJSON("stubs/" + schemeSpecific.substring(0, pos));
                stubs.get("responseQueue").asArray().forEach((stub) -> {
                    responseQueue.offer(stub.asString());
            });
        }

        return true;
    }

The configuration is something like:

{
    "responseQueue": [
        "updateOrderResponse", "updateOrderResponse",
        "authorizationResponse1", "authorizationResponse2",
        "updateOrderResponse", "updateOrderResponseValid"
    ]
}

As you can see, for now it is just a queue of responses the provider will get from the connection.

Such configuration is then used to create a proper connection stub:

@Override
public Connection connect(URI serverUri, NetworkSettings networkSettings) {
    return new AcmeConnectionStub(resolve(serverUri), responseQueue.poll());
}

of course, this is very basic, it can be improved in many ways, but it should be enough to get the idea.