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

Fix #971: Add column for signature metadata #985

Merged
merged 15 commits into from
Sep 6, 2023
Merged
Show file tree
Hide file tree
Changes from 10 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
<?xml version="1.0" encoding="UTF-8"?>
<databaseChangeLog xmlns="http://www.liquibase.org/xml/ns/dbchangelog"
xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
xsi:schemaLocation="http://www.liquibase.org/xml/ns/dbchangelog http://www.liquibase.org/xml/ns/dbchangelog/dbchangelog-4.9.xsd">

<changeSet id="1" logicalFilePath="powerauth-java-server/1.5.x/20230323-add-column-signature-metadata" author="Lubos Racansky">
<preConditions onFail="MARK_RAN">
<not>
<columnExists tableName="pa_signature_audit" columnName="signature_metadata"/>
</not>
</preConditions>
<comment>Add signature_metadata column of type Clob</comment>
<addColumn tableName="pa_signature_audit">
<column name="signature_metadata" type="clob"/>
</addColumn>
</changeSet>

</databaseChangeLog>

This file was deleted.

Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
xsi:schemaLocation="http://www.liquibase.org/xml/ns/dbchangelog http://www.liquibase.org/xml/ns/dbchangelog/dbchangelog-4.9.xsd">

<include file="20230323-add-columns-signature-data.xml" relativeToChangelogFile="true" />
<include file="20230323-add-column-signature-metadata.xml" relativeToChangelogFile="true" />
<include file="20230426-add-column-totp-seed.xml" relativeToChangelogFile="true" />
<include file="20230723-add-table-unique-value.xml" relativeToChangelogFile="true" />

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,59 @@
/*
* PowerAuth Server and related software components
* Copyright (C) 2023 Wultra s.r.o.
*
* This program is free software: you can redistribute it and/or modify
* it under the terms of the GNU Affero General Public License as published
* by the Free Software Foundation, either version 3 of the License, or
* (at your option) any later version.
*
* This program is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
* GNU Affero General Public License for more details.
*
* You should have received a copy of the GNU Affero General Public License
* along with this program. If not, see <http://www.gnu.org/licenses/>.
*/

package io.getlime.security.powerauth.app.server.database.model;

import lombok.AllArgsConstructor;
import com.fasterxml.jackson.annotation.JsonProperty;
import lombok.Data;
import lombok.NoArgsConstructor;

import java.io.Serializable;
import java.util.Objects;


/**
* Concrete implementation of the {@link SignatureMetadata} interface for PowerAuth.
* Contains metadata parameters specific to PowerAuth Signature.
*
* @author Jan Dusil
*/
@AllArgsConstructor
@NoArgsConstructor
@Data
public class PowerAuthSignatureMetadata implements SignatureMetadata<String, String>, Serializable {
Copy link
Member

Choose a reason for hiding this comment

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

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 marked this conversation as resolved.
Show resolved Hide resolved
@JsonProperty("signatureDataMethod")
private String signatureDataMethod;
@JsonProperty("signatureDataUriId")
private String signatureDataUriId;

@Override
public boolean equals(Object o) {
if (this == o) return true;
if (o == null || getClass() != o.getClass()) return false;
PowerAuthSignatureMetadata that = (PowerAuthSignatureMetadata) o;
return Objects.equals(signatureDataMethod, that.signatureDataMethod) &&
Objects.equals(signatureDataUriId, that.signatureDataUriId);
}

@Override
public int hashCode() {
return Objects.hash(signatureDataMethod, signatureDataUriId);
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
/*
* PowerAuth Server and related software components
* Copyright (C) 2023 Wultra s.r.o.
*
* This program is free software: you can redistribute it and/or modify
* it under the terms of the GNU Affero General Public License as published
* by the Free Software Foundation, either version 3 of the License, or
* (at your option) any later version.
*
* This program is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
* GNU Affero General Public License for more details.
*
* You should have received a copy of the GNU Affero General Public License
* along with this program. If not, see <http://www.gnu.org/licenses/>.
*/

package io.getlime.security.powerauth.app.server.database.model;

import com.fasterxml.jackson.annotation.JsonSubTypes;
import com.fasterxml.jackson.annotation.JsonTypeInfo;
import io.getlime.security.powerauth.app.server.database.model.enumeration.SignatureMetadataType;

import java.io.Serializable;

/**
* Represents a generic interface for metadata related to different types of signatures.
* The interface allows for defining various metadata attributes as type parameters T1 and T2.
*
* @param <T1> The type of the first metadata parameter.
* @param <T2> The type of the second metadata parameter.
* @author Jan Dusil
*/
@JsonTypeInfo(use = JsonTypeInfo.Id.NAME, property = "type")
@JsonSubTypes({
@JsonSubTypes.Type(value = PowerAuthSignatureMetadata.class,
name = SignatureMetadataType.POWER_AUTH)
})
public interface SignatureMetadata<T1, T2> extends Serializable {
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@jandusil jandusil Sep 6, 2023

Choose a reason for hiding this comment

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

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);
    }


}

Original file line number Diff line number Diff line change
@@ -0,0 +1,93 @@
/*
* PowerAuth Server and related software components
* Copyright (C) 2023 Wultra s.r.o.
*
* This program is free software: you can redistribute it and/or modify
* it under the terms of the GNU Affero General Public License as published
* by the Free Software Foundation, either version 3 of the License, or
* (at your option) any later version.
*
* This program is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
* GNU Affero General Public License for more details.
*
* You should have received a copy of the GNU Affero General Public License
* along with this program. If not, see <http://www.gnu.org/licenses/>.
*/

package io.getlime.security.powerauth.app.server.database.model.converter;

import com.fasterxml.jackson.core.JsonProcessingException;
import com.fasterxml.jackson.core.type.TypeReference;
import com.fasterxml.jackson.databind.ObjectMapper;
import io.getlime.security.powerauth.app.server.database.model.SignatureMetadata;
import jakarta.persistence.AttributeConverter;
import jakarta.persistence.Converter;
import lombok.extern.slf4j.Slf4j;
import org.apache.commons.lang3.StringUtils;
import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.stereotype.Component;

/**
* A JPA attribute converter for converting SignatureMetadata objects to and from JSON representations.
* This class enables storing SignatureMetadata in the database as a JSON column.
*
* @author Jan Dusil
*/
@Converter
@Component
@Slf4j
public class SignatureMetadataConverter implements AttributeConverter<SignatureMetadata<?, ?>, String> {
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@jandusil jandusil Sep 5, 2023

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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

T1 getMetadataParam1();


private final ObjectMapper objectMapper;

/**
* Constructor that initializes the ObjectMapper.
*
* @param objectMapper The Jackson ObjectMapper.
*/
@Autowired
public SignatureMetadataConverter(ObjectMapper objectMapper) {
jandusil marked this conversation as resolved.
Show resolved Hide resolved
this.objectMapper = objectMapper;
}

/**
* Converts a SignatureMetadata object to its JSON string representation.
*
* @param attribute The SignatureMetadata object to convert.
* @return The JSON string representation of the object.
*/
@Override
public String convertToDatabaseColumn(SignatureMetadata<?, ?> attribute) {
Copy link
Member

Choose a reason for hiding this comment

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

dtto

if (attribute == null) {
return "{}";
}
try {
return objectMapper.writeValueAsString(attribute);
} catch (JsonProcessingException ex) {
logger.warn("JSON writing error", ex);
return "{}";
}
}

/**
* Converts a JSON string representation to a SignatureMetadata object.
*
* @param s The JSON string to convert.
* @return The converted SignatureMetadata object.
*/
@Override
public SignatureMetadata<?, ?> convertToEntityAttribute(String s) {
Copy link
Member

Choose a reason for hiding this comment

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

dtto

if (StringUtils.isBlank(s)) {
return null;
}
try {
return objectMapper.readValue(s, new TypeReference<>() {
});
} catch (JsonProcessingException ex) {
logger.warn("Conversion failed for SignatureMetadata, error: " + ex.getMessage(), ex);
return null;
}
}
}
Loading