Fix #971: Add column for signature metadata #985

jandusil · 2023-08-24T13:49:24Z

Add new column signature_metadata, remove merged columns signature_data_method and signature_data_uri_id, document it in liquibase
Edit entity class, introduce new converter

- Add new column signature_metadata, remove merged columns signature_data_method and signature_data_uri_id, document it in liquibase - Edit entity class, introduce new converter

- Propagate changes

- Add NoArgsConstructor

romanstrobl

Good job. Let's review the Liquibase script, because it will not work on other databases such as Oracle.

...ngelog/changesets/powerauth-java-server/1.5.x/20230824-update-signature-metadata-columns.xml

...tlime/security/powerauth/app/server/database/model/converter/SignatureMetadataConverter.java

- Make the change retrospective in terms of liquibase

petrdvorak

Please let's introduce abstraction on signature metadata types. We currently need to have only PowerAuth signature metadata with method and URI ID, but the reason we are making this change is that next release, we will also need to have FIDO2 signature metadata with completely different attributes.

As a result, we need to introduce facilities that allow serialization / deserialization of the specific classes.

...src/main/java/io/getlime/security/powerauth/app/server/database/model/SignatureMetadata.java

...io/getlime/security/powerauth/app/server/service/behavior/tasks/AuditingServiceBehavior.java

- Introduce abstraction for SignatureMetadata

- Add a test - Introduce more generic approach

jandusil · 2023-08-30T22:29:51Z

...io/getlime/security/powerauth/app/server/service/behavior/tasks/AuditingServiceBehavior.java

        signatureAuditRecord.setActivation(activationRepository.getReferenceById(activation.getActivationId()));
        signatureAuditRecord.setActivationCounter(activation.getCounter());
        signatureAuditRecord.setActivationCtrDataBase64(activation.getCtrDataBase64());
        signatureAuditRecord.setActivationStatus(activation.getActivationStatus());
        signatureAuditRecord.setAdditionalInfo(additionalInfo);
        signatureAuditRecord.setDataBase64(data);
        signatureAuditRecord.setSignature(signatureData.getSignature());
-        signatureAuditRecord.setSignatureDataMethod(signatureData.getRequestMethod());
-        signatureAuditRecord.setSignatureDataUriId(signatureData.getRequestUriId());
+        signatureAuditRecord.setSignatureMetadata(signatureMetadata);


Do we want to adjust the AuditDetail as well? Probably yes, right?

How do you mean it?

Yes, we need to store it in the audit record too. 👍

- Add a test - Introduce more generic approach

.../changelog/changesets/powerauth-java-server/1.5.x/20230323-add-column-signature-metadata.xml

...tlime/security/powerauth/app/server/database/model/converter/SignatureMetadataConverter.java

...java/io/getlime/security/powerauth/app/server/database/model/PowerAuthSignatureMetadata.java

...me/security/powerauth/app/server/service/model/signature/SignatureMetadataConverterTest.java

...java/io/getlime/security/powerauth/app/server/database/model/PowerAuthSignatureMetadata.java

...src/main/java/io/getlime/security/powerauth/app/server/database/model/SignatureMetadata.java

- Change type of signature_metadata to clob - Incorporate code review comments

- Incorporate code review comments

romanstrobl

It should work as expected, however I suggest we improve the types. In my opinion there is no need to use the ? (any type) in generics, or am I missing something? E.g. we know the resulting value in DB is a String.

.../getlime/security/powerauth/app/server/database/model/enumeration/SignatureMetadataType.java

romanstrobl · 2023-09-05T12:15:50Z

...tlime/security/powerauth/app/server/database/model/converter/SignatureMetadataConverter.java

+@Converter
+@Component
+@Slf4j
+public class SignatureMetadataConverter implements AttributeConverter<SignatureMetadata<?, ?>, String> {


Why do we use the <?, ?> type specification?

My general idea was that this converter could be used for different types of SignatureMetadata implementations not just the SignatureMetadata<String, String>. Since the props of implementations of SignatureMetadata could be different types, I thought the <?, ?> type specification shall be here.

What are these types which are specified instead of <?, ?> used for? I can't find it in the code. Do we really need to make the class SignatureMetadata generic? What happens if you remove <?, ?> type specification from the 3 places where it is used, will the code still work?

I think we can safely say that <String, Object> will work for us. This allows for a fairly generic way of serialization. Apologies for not specifying this clearer.

In current code the usage of generics is really strange. See the following message from IntelliJ IDEA:

My point is that we specify types for a generified class SignatureMetadata, however these types are never used, so they just make the code more complicated for no good reason. If you remove the <T1, T2> and <?, ?> types everywhere from the code for the SignatureMetadata, the tests are still green, so why do we need this generification in the first place?

I guess that it's just a leftover from the previous version, where there used to be such code

T1 getMetadataParam1();

.../getlime/security/powerauth/app/server/database/model/enumeration/SignatureMetadataType.java

- Incorporate code review comments

petrdvorak · 2023-09-06T06:56:39Z

...java/io/getlime/security/powerauth/app/server/database/model/PowerAuthSignatureMetadata.java

+@AllArgsConstructor
+@NoArgsConstructor
+@Data
+public class PowerAuthSignatureMetadata implements SignatureMetadata<String, String>, Serializable {


Apologies for the change, but just in case, let's make this <String, Object> - I can imagine we would like to encode some FIDO2 specific data and this could require structure to be comprehensive and easy to deserialize back...

petrdvorak · 2023-09-06T06:58:44Z

...tlime/security/powerauth/app/server/database/model/converter/SignatureMetadataConverter.java

+@Converter
+@Component
+@Slf4j
+public class SignatureMetadataConverter implements AttributeConverter<SignatureMetadata<?, ?>, String> {


I think we can safely say that <String, Object> will work for us. This allows for a fairly generic way of serialization. Apologies for not specifying this clearer.

petrdvorak · 2023-09-06T06:58:54Z

...tlime/security/powerauth/app/server/database/model/converter/SignatureMetadataConverter.java

+     * @return The JSON string representation of the object.
+     */
+    @Override
+    public String convertToDatabaseColumn(SignatureMetadata<?, ?> attribute) {


petrdvorak · 2023-09-06T06:59:00Z

...tlime/security/powerauth/app/server/database/model/converter/SignatureMetadataConverter.java

+     * @return The converted SignatureMetadata object.
+     */
+    @Override
+    public SignatureMetadata<?, ?> convertToEntityAttribute(String s) {


petrdvorak · 2023-09-06T07:02:31Z

.../getlime/security/powerauth/app/server/database/model/enumeration/SignatureMetadataType.java

+     * Represents PowerAuth signature metadata.
+     * This value is used for identifying the type of metadata in JSON serialization/deserialization.
+     */
+    public static final String POWERAUTH = "PowerAuthSignatureMetadata";


Since this will get serialized into the JSON payload, we should use just POWERAUTH. Also, are we sure this will get into the serialized JSON? We need to be able to deserialize it back...

petrdvorak · 2023-09-06T07:03:10Z

...io/getlime/security/powerauth/app/server/service/behavior/tasks/AuditingServiceBehavior.java

        signatureAuditRecord.setActivation(activationRepository.getReferenceById(activation.getActivationId()));
        signatureAuditRecord.setActivationCounter(activation.getCounter());
        signatureAuditRecord.setActivationCtrDataBase64(activation.getCtrDataBase64());
        signatureAuditRecord.setActivationStatus(activation.getActivationStatus());
        signatureAuditRecord.setAdditionalInfo(additionalInfo);
        signatureAuditRecord.setDataBase64(data);
        signatureAuditRecord.setSignature(signatureData.getSignature());
-        signatureAuditRecord.setSignatureDataMethod(signatureData.getRequestMethod());
-        signatureAuditRecord.setSignatureDataUriId(signatureData.getRequestUriId());
+        signatureAuditRecord.setSignatureMetadata(signatureMetadata);


Yes, we need to store it in the audit record too. 👍

petrdvorak · 2023-09-06T07:03:55Z

...src/main/java/io/getlime/security/powerauth/app/server/database/model/SignatureMetadata.java

+        @JsonSubTypes.Type(value = PowerAuthSignatureMetadata.class,
+                name = SignatureMetadataType.POWERAUTH)
+})
+public interface SignatureMetadata<T1, T2> extends Serializable {


Where do we keep the type property? We need to have it somewhere so that we are able to deserialize the value back...

I believe the type is dynamically added by the annotation during serialization
res: https://www.baeldung.com/jackson-inheritance#bd-1-conversion-between-subtypes

@JsonTypeInfo(use = JsonTypeInfo.Id.NAME, property = "type") @JsonSubTypes({ @JsonSubTypes.Type(value = PowerAuthSignatureMetadata.class, name = SignatureMetadataType.POWERAUTH) })

Works in the tests I wrote

@Test void convertToDatabaseColumnTest() { PowerAuthSignatureMetadata metadata = new PowerAuthSignatureMetadata("POST", "123"); String jsonStr = converter.convertToDatabaseColumn(metadata); assertNotNull(jsonStr); assertEquals("{\"type\":\"POWERAUTH\",\"signatureDataMethod\":\"POST\",\"signatureDataUriId\":\"123\"}", jsonStr); }

- Incorporate code review comments

- Remove generics

- Remove redundant extension

petrdvorak

Looks OK now

...java/io/getlime/security/powerauth/app/server/database/model/PowerAuthSignatureMetadata.java

romanstrobl

Much better now!

- Add serialVersionUID

Fix #971: Add column for signature metadata

0b357e8

- Add new column signature_metadata, remove merged columns signature_data_method and signature_data_uri_id, document it in liquibase - Edit entity class, introduce new converter

jandusil self-assigned this Aug 24, 2023

jandusil linked an issue Aug 24, 2023 that may be closed by this pull request

Add column for signature metadata #971

Closed

jandusil added 3 commits August 24, 2023 15:58

Fix #971: Add column for signature metadata

1f1c3ed

- Propagate changes

Merge branch 'develop' into issues/971-signature-metadata-col

225717b

Fix #971: Add column for signature metadata

4c19310

- Add NoArgsConstructor

jandusil requested a review from romanstrobl August 28, 2023 22:46

jandusil marked this pull request as ready for review August 28, 2023 22:46

romanstrobl approved these changes Aug 29, 2023

View reviewed changes

...ngelog/changesets/powerauth-java-server/1.5.x/20230824-update-signature-metadata-columns.xml Outdated Show resolved Hide resolved

romanstrobl reviewed Aug 29, 2023

View reviewed changes

...tlime/security/powerauth/app/server/database/model/converter/SignatureMetadataConverter.java Show resolved Hide resolved

Fix #971: Add column for signature metadata

fe2e837

- Make the change retrospective in terms of liquibase

jandusil requested a review from romanstrobl August 29, 2023 11:22

petrdvorak requested changes Aug 29, 2023

View reviewed changes

jandusil added 2 commits August 30, 2023 22:26

Fix #971: Add column for signature metadata

df53390

- Introduce abstraction for SignatureMetadata

Fix #971: Add column for signature metadata

a9a480f

- Add a test - Introduce more generic approach

jandusil requested a review from petrdvorak August 30, 2023 22:26

jandusil commented Aug 30, 2023

View reviewed changes

Fix #971: Add column for signature metadata

417d148

- Add a test - Introduce more generic approach

banterCZ reviewed Sep 4, 2023

View reviewed changes

.../changelog/changesets/powerauth-java-server/1.5.x/20230323-add-column-signature-metadata.xml Outdated Show resolved Hide resolved

banterCZ reviewed Sep 4, 2023

View reviewed changes

...tlime/security/powerauth/app/server/database/model/converter/SignatureMetadataConverter.java Outdated Show resolved Hide resolved

banterCZ reviewed Sep 4, 2023

View reviewed changes

jandusil added 2 commits September 5, 2023 11:26

Fix #971: Add column for signature metadata

afd9adc

- Change type of signature_metadata to clob - Incorporate code review comments

Fix #971: Add column for signature metadata

6f89a1b

- Incorporate code review comments

jandusil requested a review from banterCZ September 5, 2023 09:44

romanstrobl requested changes Sep 5, 2023

View reviewed changes

Fix #971: Add column for signature metadata

e2cca7a

- Incorporate code review comments

jandusil requested a review from romanstrobl September 5, 2023 14:53

petrdvorak requested changes Sep 6, 2023

View reviewed changes

Fix #971: Add column for signature metadata

2406b59

- Incorporate code review comments

jandusil requested a review from petrdvorak September 6, 2023 10:27

jandusil added 2 commits September 6, 2023 13:14

Fix #971: Add column for signature metadata

41941ac

- Remove generics

Fix #971: Add column for signature metadata

77979c9

- Remove redundant extension

petrdvorak approved these changes Sep 6, 2023

View reviewed changes

banterCZ reviewed Sep 6, 2023

View reviewed changes

...java/io/getlime/security/powerauth/app/server/database/model/PowerAuthSignatureMetadata.java Show resolved Hide resolved

romanstrobl approved these changes Sep 6, 2023

View reviewed changes

Fix #971: Add column for signature metadata

8775a9d

- Add serialVersionUID

jandusil merged commit 7596275 into develop Sep 6, 2023
5 checks passed

jandusil deleted the issues/971-signature-metadata-col branch September 6, 2023 11:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #971: Add column for signature metadata #985

Fix #971: Add column for signature metadata #985

jandusil commented Aug 24, 2023

romanstrobl left a comment

petrdvorak left a comment

jandusil Aug 30, 2023

romanstrobl Sep 5, 2023

petrdvorak Sep 6, 2023

romanstrobl left a comment

romanstrobl Sep 5, 2023

jandusil Sep 5, 2023 •

edited

Loading

romanstrobl Sep 5, 2023

petrdvorak Sep 6, 2023

romanstrobl Sep 6, 2023

banterCZ Sep 6, 2023

petrdvorak Sep 6, 2023

petrdvorak Sep 6, 2023

petrdvorak Sep 6, 2023

petrdvorak Sep 6, 2023

petrdvorak Sep 6, 2023

petrdvorak Sep 6, 2023

petrdvorak Sep 6, 2023

jandusil Sep 6, 2023 •

edited

Loading

petrdvorak left a comment

romanstrobl left a comment

Fix #971: Add column for signature metadata #985

Fix #971: Add column for signature metadata #985

Conversation

jandusil commented Aug 24, 2023

romanstrobl left a comment

Choose a reason for hiding this comment

petrdvorak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

romanstrobl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jandusil Sep 5, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jandusil Sep 6, 2023 • edited Loading

Choose a reason for hiding this comment

petrdvorak left a comment

Choose a reason for hiding this comment

romanstrobl left a comment

Choose a reason for hiding this comment

jandusil Sep 5, 2023 •

edited

Loading

jandusil Sep 6, 2023 •

edited

Loading