SDK-2771: Add new maven module for supporting the creation of Yoti au… #505
Conversation
MrBurtyyy
force-pushed
the
SDK-2771
branch
2 times, most recently
from
9d1586b to
d994ecd
Compare
pn-santos
left a comment
pn-santos
left a comment
There was a problem hiding this comment.
I'm not that familiar with the current practices in the SDK nor any stylistic conventions in place so apologies in advance if any of the comments are out-of-sync with how stuff is done in the SDKs.
Also, given I'm not really involved with the SDKs don't take the comments as blockers. I'm not going to explicitly approve, I'll leave that to someone more attuned to the SDK code base to take that responsibility, but I don't see anything functional that would be a reason to hold it up.
yoti-sdk-auth/src/main/java/com/yoti/auth/AuthenticationTokenGenerator.java
Outdated
Show resolved
Hide resolved
| private final SignedJwtGenerator signedJwtGenerator; | ||
|
|
||
| private final String authApiUrl; | ||
| private final ObjectMapper objectMapper; |
There was a problem hiding this comment.
Why is the ObjectMapper being injected/exposed?
The CreateAuthenticationTokenResponse is defined by this module, shouldn't the ObjectMapper (and it's config) also be owned/defined by this module? What would be the point of the client providing their own instance config if they do not control the annotations/layout of CreateAuthenticationTokenResponse?
There was a problem hiding this comment.
We don't expose the ObjectMapper to the Relying Business, but it's done this way (and in other places in the SDK) to make testing with mocks easier in the AuthenticationTokenGenerator
yoti-sdk-auth/src/main/java/com/yoti/auth/AuthenticationTokenGenerator.java
Outdated
Show resolved
Hide resolved
| ObjectMapper objectMapper = new ObjectMapper() | ||
| .setPropertyNamingStrategy(PropertyNamingStrategies.SNAKE_CASE) | ||
| .configure(DeserializationFeature.FAIL_ON_UNKNOWN_PROPERTIES, false); |
There was a problem hiding this comment.
If we do not expose the ObjectMapper then there's no point in making it an instance var. It should be declared as a static final var of AuthenticationTokenGenerator
yoti-sdk-auth/src/main/java/com/yoti/auth/AuthenticationTokenGenerator.java
Outdated
Show resolved
Hide resolved
yoti-sdk-auth/src/main/java/com/yoti/auth/FormRequestClient.java
Outdated
Show resolved
Hide resolved
yoti-sdk-auth/src/main/java/com/yoti/auth/AuthenticationTokenGenerator.java
Outdated
Show resolved
Hide resolved
yoti-sdk-auth/src/main/java/com/yoti/auth/FormRequestClient.java
Outdated
Show resolved
Hide resolved
| this.signedJwtGenerator = signedJwtGenerator; | ||
| this.objectMapper = objectMapper; | ||
|
|
||
| authApiUrl = System.getProperty(Properties.PROPERTY_YOTI_AUTH_URL, Properties.DEFAULT_YOTI_AUTH_URL); |
There was a problem hiding this comment.
This class is the sole user of this Properties class... why not simply declare those values as constants here? i.e.
private static final String YOTI_AUTH_URL_KEY = "yoti.auth.url";
(...)
authApiUrl = System.getProperty(YOTI_AUTH_URL_KEY, "https://auth.api.yoti.com/v1/oauth/token");and get rid of Properties altogether?
There was a problem hiding this comment.
We have public properties in here, so the YOTI_AUTH_URL_KEY property can be accessed if we want to override the URL without knowing the actual key needed (like we do in the other modules).
Not sure if we want to change this, @bucky-boy what do you think?
yoti-sdk-auth/src/main/java/com/yoti/auth/SignedJwtGenerator.java
Outdated
Show resolved
Hide resolved
| String jwts = signedJwtGenerator.create(sdkId, keyPair, jwtIdSupplier, authApiUrl); | ||
|
|
||
| Map<String, String> formParams = new HashMap<>(DEFAULT_FORM_VALUES); | ||
| formParams.put("scope", String.join(" ", scopes)); |
There was a problem hiding this comment.
Should this list be checked for null or empty string?
| * @throws IOException | ||
| */ | ||
| public CreateAuthenticationTokenResponse generate(List<String> scopes) throws ResourceException, IOException { | ||
| String jwts = signedJwtGenerator.create(sdkId, keyPair, jwtIdSupplier, authApiUrl); |
| try { | ||
| return encode(entry.getKey()) + "=" + encode(entry.getValue()); | ||
| } catch (UnsupportedEncodingException e) { | ||
| throw new RuntimeException(e); | ||
| } |
There was a problem hiding this comment.
If doing the try/catch to turn it into a runtime exception it would probably make more sense to do it within the encode method.
irotech
left a comment
irotech
left a comment
There was a problem hiding this comment.
only not blocking minor comments
| formParams.put("grant_type", "client_credentials"); | ||
| formParams.put("client_assertion_type", "urn:ietf:params:oauth:client-assertion-type:jwt-bearer"); | ||
| formParams.put("scope", String.join(" ", scopes)); | ||
| formParams.put("client_assertion", jwts); |
There was a problem hiding this comment.
why having a var if used once?
| formParams.put("client_assertion", jwts); | |
| formParams.put("client_assertion", createSignedJwt(sdkId, keyPair, jwtIdSupplier, authApiUrl)); |
| connection.setRequestMethod(method); | ||
| connection.setRequestProperty("Content-Type", "application/x-www-form-urlencoded"); | ||
| connection.setRequestProperty("Content-Length", Integer.toString(postData.length)); | ||
| connection.setRequestProperty("charset", "utf-8"); |
There was a problem hiding this comment.
| connection.setRequestProperty("charset", "utf-8"); | |
| connection.setRequestProperty("charset", StandardCharsets.UTF_8); |
| + "vd67J8cdh1v6d3m0xrT639BIh7FIZjVIg3B8ERBmIH1oFn05BQFYlCEUG7Cl1KV2\n" | ||
| + "/VIBAoGAYin6eHFtBXpPRMddTt4lLOFtpBb9gGp9DxKGAsH0pdfQYgZHz0xKtxrq\n" | ||
| + "6l+d2M9sRhtisxw8YevPFWEVtcUWxz+qESM+wzUMrmWSuwwxXyVgjR4rC8untfPM\n" | ||
| + "laDBX8dDBwBQ3i0FhvuGR/LEjHWr9hj00faKROHOLFim6wbRIkg=\n" // |
There was a problem hiding this comment.
is this empty comment, "//", in there for any reason?
…thentication tokens via API
|
…thentication tokens via API