Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add unit testing for tuf key verification. #146

Merged
merged 6 commits into from
Sep 14, 2022
Merged

Conversation

patflynn
Copy link
Collaborator

Required refactoring static verifiers factory method. I normally have a good idea how to handle DI on the server-side, but I'm just making it up as I go here. I'm very open to changes. Eventually it might make sense to collapse the two levels of indirection between verification supplier and verification.

Work towards #60

Signed-off-by: Patrick Flynn patrick@chainguard.dev

…c verifiers factory method

Signed-off-by: Patrick Flynn <patrick@chainguard.dev>
Patrick Flynn added 2 commits September 13, 2022 18:16
Signed-off-by: Patrick Flynn <patrick@chainguard.dev>
Signed-off-by: Patrick Flynn <patrick@chainguard.dev>
Copy link
Member

@loosebazooka loosebazooka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to start a discussion. I think I would just add the functional interface to Verifiers

  @FunctionalInterface
  public interface Supplier {
      Verifier newVerifier(PublicKey publicKey) throws NoSuchAlgorithmException;
  }

on a method signature like

/* package private */ TufClient(Verifiers.Supplier supplier)

you can inject with the method reference

new TufClient(Verifiers::newVerifier)

If we don't need to expose Verifiers.Supplier we can define that functional interface in the TUF client itself.

I might also make the TUF client have one public static factory method newTufClient and have the various constructors you need for testing be package private?

Patrick Flynn added 2 commits September 14, 2022 09:56
Signed-off-by: Patrick Flynn <patrick@chainguard.dev>
Signed-off-by: Patrick Flynn <patrick@chainguard.dev>
@patflynn
Copy link
Collaborator Author

Massive improvement! Now we're cooking with gas!

Comment on lines 61 to 68
public TufClient(Clock clock, Verifiers.Supplier verifiers) {
this.clock = clock;
this.verifiers = verifiers;
}

public TufClient(Verifiers.Supplier verifiers) {
this(Clock.systemUTC(), verifiers);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should these be public?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They can be package private and still be available for tests?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll remove public. FYI the TufClient class at the moment is really just a giant place holder for the update logic. It's going to be refactored, and the current API is not reflective in any way of what the client API will be.

loosebazooka
loosebazooka previously approved these changes Sep 14, 2022
Copy link
Member

@loosebazooka loosebazooka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor stuff but LGTM.

Signed-off-by: Patrick Flynn <patrick@chainguard.dev>
@patflynn patflynn merged commit e862713 into main Sep 14, 2022
@patflynn patflynn deleted the injectable-verifier branch September 14, 2022 16:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants