Critical code review PR - Last sprint handover checklist of 0.10.0 release #147
Conversation
Earlier `/issuance/.well-known/openid-credential-issuer?version=v123` would return the latest versioned issuer metadata earlier if it was not valid, now returns an error. Signed-off-by: Harsh Vardhan <[email protected]>
Signed-off-by: Harsh Vardhan <[email protected]>
Signed-off-by: Challa Rao <[email protected]>
[DSD-5526] Update init_db.sh namespace and chart version
Signed-off-by: Harsh Vardhan <[email protected]>
[INJICERT-236] throw error when unsupported wellknown version requested
Signed-off-by: Challarao <[email protected]>
[INJICERT-37] rename certify database name
Signed-off-by: VSIVAKALYAN <[email protected]>
[DSD-5618] Update init_values.yaml
* [INJICERT-248] use well known config from spring from separate JSON file Signed-off-by: Harsh Vardhan <[email protected]> * [INJICERT-248] fixup JSON formatting of well-known config Signed-off-by: Harsh Vardhan <[email protected]> * [INJICERT-248] Fixup formatting & resource URL Signed-off-by: Harsh Vardhan <[email protected]> * [INJICERT-248] add missing config for mock-identity Signed-off-by: Harsh Vardhan <[email protected]> * [INJICERT-248] parse metadata when mediatype is text/plain Signed-off-by: Harsh Vardhan <[email protected]> * [INJICERT-248] rename openid4vci well known file Signed-off-by: Harsh Vardhan <[email protected]> --------- Signed-off-by: Harsh Vardhan <[email protected]>
Signed-off-by: ckm007 <[email protected]>
[MOSIP-34749] removed platform related DB's reference
Signed-off-by: Praful Rakhade <[email protected]>
[DSD-5853] updated Dockerfile
Signed-off-by: Nandhukumar <[email protected]>
#49)" (#59) This reverts commit c1c4e43. Signed-off-by: Harsh Vardhan <[email protected]>
Signed-off-by: Rakshitha650 <[email protected]>
Signed-off-by: Rakshitha650 <[email protected]>
[MOSIP-34818]Updated values.yaml
Signed-off-by: Nandhukumar <[email protected]>
Signed-off-by: Nandhukumar <[email protected]>
Signed-off-by: Nandhukumar <[email protected]>
Signed-off-by: Nandhukumar <[email protected]>
Signed-off-by: Nandhukumar <[email protected]>
Signed-off-by: Nandhukumar <[email protected]>
Signed-off-by: Nandhukumar <[email protected]>
MOSIP-33742 | Created component based API test rig for Inji-certify
Signed-off-by: Mohanachandran S <[email protected]>
Signed-off-by: Nandhukumar <[email protected]>
MOSIP-37807 | added default values
MOSIP-37793 - Updated the Readme file
Signed-off-by: Nandhukumar <[email protected]>
MOSIP-37942 | Updated delete query file name
There was a problem hiding this comment.
@vharsh @Piyush7034 I have reviewed the certify-core sub project in the PR and added comments.
Doing it in parts to handover to the dev team to parallelly work on the fixes.
certify-core/src/main/java/io/mosip/certify/core/constants/Constants.java
Show resolved
Hide resolved
certify-core/src/main/java/io/mosip/certify/core/constants/ErrorConstants.java
Outdated
Show resolved
Hide resolved
certify-core/src/main/java/io/mosip/certify/core/constants/SignatureAlg.java
Outdated
Show resolved
Hide resolved
certify-core/src/main/java/io/mosip/certify/core/entity/TemplateData.java
Outdated
Show resolved
Hide resolved
certify-core/src/main/java/io/mosip/certify/core/entity/TemplateData.java
Outdated
Show resolved
Hide resolved
| private String errorCode; | ||
|
|
||
| public TemplateException() { | ||
| super(ErrorConstants.UNKNOWN_ERROR); |
There was a problem hiding this comment.
this error says unknown, if we are having a separate exception class, that needs to have some base error message. Also in what all cases this exception is planned to be thrown ?
Also If this is related to Credential template, then we can also rename this as CredentialTemplateException to be more specific.
There was a problem hiding this comment.
Right now, we don't have proper error codes like how Keymanager or other MOSIP modules have had it for long, this can be implemented.
There was a problem hiding this comment.
ok lets do it then, for now we need only a generic error message that given a overall idea of what kind of error this is
There was a problem hiding this comment.
This constructor is not used anywhere, we've only used it with template_with_id_not_found error code.
There was a problem hiding this comment.
Contructor without param to be removed
certify-core/src/main/java/io/mosip/certify/core/repository/TemplateRepository.java
Outdated
Show resolved
Hide resolved
certify-core/src/main/java/io/mosip/certify/core/validators/CredentialRequestValidator.java
Outdated
Show resolved
Hide resolved
…ch (#148) * [INJICERT-499] added config docs for postgres dp plugin Signed-off-by: Piyush7034 <[email protected]> * Added upgrade sql scripts Signed-off-by: Piyush7034 <[email protected]> * Fixed template name Signed-off-by: Piyush7034 <[email protected]> * Fixed template name Signed-off-by: Piyush7034 <[email protected]> * Change in some configs w.r.t. data provider plugin Signed-off-by: Piyush7034 <[email protected]> --------- Signed-off-by: Piyush7034 <[email protected]> Signed-off-by: Vishwa <[email protected]>
There was a problem hiding this comment.
@vharsh @Piyush7034 Part 2 review done, reviewed till service layer of certify-service
certify-integration-api/src/main/java/io/mosip/certify/api/spi/DataProviderPlugin.java
Outdated
Show resolved
Hide resolved
certify-integration-api/src/main/java/io/mosip/certify/api/spi/VCFormatter.java
Outdated
Show resolved
Hide resolved
certify-integration-api/src/main/java/io/mosip/certify/api/spi/VCFormatter.java
Outdated
Show resolved
Hide resolved
| /** | ||
| * VCSigner can sign any VC provided a vcHash & Signer inputs | ||
| */ | ||
| public interface VCSigner { |
There was a problem hiding this comment.
Any special reason for this to be part of certify-integration-api and not certify-service ?
There was a problem hiding this comment.
There is spi folder in both certify-core and certify-integration-api. All the interfaces are placed in these folders. So, it was placed in certify-integration-api. This can be moved to service folder of the certify-service
There was a problem hiding this comment.
VCSigner and impl file to be moved to io.mosip.certify.vcsigners
certify-service/src/main/java/io/mosip/certify/CertifyServiceApplication.java
Outdated
Show resolved
Hide resolved
certify-service/src/main/java/io/mosip/certify/services/KeymanagerLibSigner.java
Outdated
Show resolved
Hide resolved
certify-service/src/main/java/io/mosip/certify/services/KeymanagerLibSigner.java
Outdated
Show resolved
Hide resolved
certify-service/src/main/java/io/mosip/certify/services/SVGRenderUtils.java
Outdated
Show resolved
Hide resolved
certify-service/src/main/java/io/mosip/certify/services/SvgTemplateServiceImpl.java
Outdated
Show resolved
Hide resolved
certify-service/src/main/java/io/mosip/certify/services/VCIssuanceServiceImpl.java
Show resolved
Hide resolved
There was a problem hiding this comment.
@vharsh @Piyush7034 Last set review done,
api test, test file and deployment script are not reviewed.
certify-service/src/main/java/io/mosip/certify/services/ldsigner/Ed25519ProofSignature2018.java
Outdated
Show resolved
Hide resolved
certify-service/src/main/java/io/mosip/certify/services/ldsigner/Ed25519ProofSignature2018.java
Outdated
Show resolved
Hide resolved
certify-service/src/main/java/io/mosip/certify/services/SvgTemplateServiceImpl.java
Outdated
Show resolved
Hide resolved
...-service/src/main/java/io/mosip/certify/services/templating/VelocityTemplatingConstants.java
Outdated
Show resolved
Hide resolved
| @Autowired | ||
| SvgTemplateService svgTemplateService; | ||
| @Value("${mosip.certify.vcformat.vc.expiry:true}") | ||
| boolean shouldHaveDates; |
There was a problem hiding this comment.
variable name and config name does not match
There was a problem hiding this comment.
Would it be a good time to consider this feature enhancement here? It wasn't discussed but implemented directly.
ref: #132
There was a problem hiding this comment.
Might need some changes in the config name, but largely ok.
There was a problem hiding this comment.
Lets rename to mosip.certify.data-provider-plugin.vc-expiry-duration and use the java standard library for parsing.
Add the comments in the properties with the format and point to the ISO 8601
| - SPRING_CONFIG_NAME=certify | ||
| - SPRING_CONFIG_LOCATION=/home/mosip/config/ | ||
| - enable_certify_artifactory=false | ||
| - download_hsm_client=false |
There was a problem hiding this comment.
We can add a small note to readme file to add the below environment variable to configure ngrok
mosipbox_public_url=https://nearly-notable-longhorn.ngrok-free.app
There was a problem hiding this comment.
docs to be updated later
| listen 80; | ||
|
|
||
| location / { | ||
| root /config/server; |
There was a problem hiding this comment.
why we have this config/server ?
There was a problem hiding this comment.
@Piyush7034 to remove this and find out if that will work for him
There was a problem hiding this comment.
@Piyush7034 Any update on this ?
There was a problem hiding this comment.
There were issues while running docker on my machine. Thats why I couldn't remove
Signed-off-by: Nandhukumar <[email protected]>
Signed-off-by: Nandhukumar <[email protected]>
MOSIP-38340 | Sync injicertify develop and release-0.10x branch
* [INJICERT-657] rename Svg -> SVG and other minor changes Signed-off-by: Harsh Vardhan <[email protected]> * [INJICERT-657] make mass renames, update DB Changes: * svg_template DB becomes template_registry & it's ID column becomes a String from UUID type Signed-off-by: Harsh Vardhan <[email protected]> * [INJICERT-657] move proofgenerator package Signed-off-by: Harsh Vardhan <[email protected]> * Docker and script code changes Signed-off-by: Piyush7034 <[email protected]> * [INJICERT-657] rename Certify keys in KeyManager Signed-off-by: Harsh Vardhan <[email protected]> * [INJICERT-657] rename Certify keys in KeyManager Signed-off-by: Harsh Vardhan <[email protected]> * [INJICERT-657] move VCFormatter & VCSigner to certify-service Signed-off-by: Harsh Vardhan <[email protected]> * [INJICERT-657] make Velocity template cacheable Signed-off-by: Harsh Vardhan <[email protected]> --------- Signed-off-by: Harsh Vardhan <[email protected]> Signed-off-by: Piyush7034 <[email protected]> Co-authored-by: Piyush7034 <[email protected]> Signed-off-by: Vishwa <[email protected]>
Signed-off-by: Piyush7034 <[email protected]> Signed-off-by: Vishwa <[email protected]>
…es and docker Signed-off-by: Piyush7034 <[email protected]>
[INJICERT-657] Added review changed related to rendering template names and docker
* [INJICERT-657] pass keyID from CertifyIssuanceImpl Other changes: * more renames, changes to scripts & SQL files Signed-off-by: Harsh Vardhan <[email protected]> * [INJICERT-657] use local CSV file for docker compose demo Signed-off-by: Harsh <[email protected]> * [INJICERT-657] remove & rename files Signed-off-by: Harsh Vardhan <[email protected]> * [INJICERT-657] add optional expiry of VC in template Signed-off-by: Harsh Vardhan <[email protected]> * [INJICERT-657] move packages around Signed-off-by: Harsh Vardhan <[email protected]> --------- Signed-off-by: Harsh Vardhan <[email protected]> Signed-off-by: Harsh <[email protected]> Signed-off-by: Vishwa <[email protected]>
Signed-off-by: Piyush7034 <[email protected]> Signed-off-by: Vishwa <[email protected]>
* [INJICERT-695] add mock mdoc VCI postman collection Signed-off-by: KiruthikaJeyashankar <[email protected]> * [INJICERT-695] modify property name of mdoc issuer key certificate mosip.certify.mock.vciplugin.issuer.key-cert changed to mosip.certify.mock.vciplugin.mdoc.issuer-key-cert Signed-off-by: KiruthikaJeyashankar <[email protected]> --------- Signed-off-by: KiruthikaJeyashankar <[email protected]> Signed-off-by: Vishwa <[email protected]>
* [INJICERT-657] Added some deployment changes Signed-off-by: Piyush7034 <[email protected]> * [INJICERT-657] Change in extraEnvVars yaml value Signed-off-by: Piyush7034 <[email protected]> --------- Signed-off-by: Piyush7034 <[email protected]> Signed-off-by: Vishwa <[email protected]>
Signed-off-by: Piyush7034 <[email protected]> Signed-off-by: Vishwa <[email protected]>
This reverts commit 443262e. Signed-off-by: Vishwa <[email protected]>
Signed-off-by: Harsh <[email protected]> Signed-off-by: Vishwa <[email protected]>
Signed-off-by: KiruthikaJeyashankar <[email protected]> Signed-off-by: Vishwa <[email protected]>
Signed-off-by: Harsh Vardhan <[email protected]> Signed-off-by: Vishwa <[email protected]>
This PR is created just to add the review comments as part of the critical code review task of last sprint handover checklist for release of 0.10.0 version.
** THIS PR SHOULD NOT BE MERGED **