CADC-8776 check the doiadmin cert owns the DOI parent node #7
Conversation
| params.put("doiMetadata", fileContent); | ||
| } | ||
| if (StringUtil.hasText(journalRef)) { | ||
| params.put("journalref", journalRef == null ? "" : journalRef); |
There was a problem hiding this comment.
Can journalRef == null happen inside if
There was a problem hiding this comment.
There is one test where the journalRef is null, but the IF condition negates journalRef from being null in the IF. Still that check and set of the journalRef isn't right. It's okay for the current int tests, but you should be able to post a journalRef with an empty value, which translates in the PostAction to deleting the existing journalRef. You can't post an empty journalReg with that test method. Checking that the journalRef is not null before adding it as a parameter is all that's needed. Fixed.
| URI vospaceResourceID = URI.create(config.getFirstPropertyValue(DoiInitAction.VAULT_RESOURCE_ID_KEY)); | ||
| String parentPath = config.getFirstPropertyValue(PARENT_PATH_KEY); | ||
|
|
||
| Subject adminSubject = SSLUtil.createSubject(new File("/config/doiadmin.pem")); |
There was a problem hiding this comment.
Maybe the README should mention the exact location where the certs are expected?
There was a problem hiding this comment.
That would be helpful, fixed.
| VOSpaceClient vosClient = new VOSpaceClient(vospaceResourceID); | ||
| String currentPath = ""; | ||
| String[] paths = parentPath.split("/"); | ||
| for (String path : paths) { |
There was a problem hiding this comment.
Is this recursive checking required? It is performed on the server side as well and reported back to the client.
There was a problem hiding this comment.
That's a good question. We use two container nodes as the root for the DOI's: /AstroDataCitationDOI/CISTI.CANFAR, but the owner and permissions only matter on the second node. I do remember something about why two parent nodes, and why owner and permissions matter for both, but I can't find any reference. Tempted to change that check for only the second parent node, but I'll leave it until I'm more sure. Added a TODO there so I don't forget.
| } | ||
| } | ||
|
|
||
| private static String getUsername(Subject subject) { |
There was a problem hiding this comment.
Hmm. This is almost like AuthenticationUtil.getUseridsFromSubject() but not quite...
There was a problem hiding this comment.
Yeah close, but just not quite.
| @@ -127,7 +126,7 @@ private Title getTitle(Resource resource) { | |||
| Title title = null; | |||
| List<Title> titles = resource.getTitles(); | |||
| for (Title t : titles) { | |||
| if (t.titleType == null) { | |||
| if (StringUtil.hasText(t.getValue())) { | |||
There was a problem hiding this comment.
titleType is optional, but the title value went from empty not allowed in the old schema, to empty allowed in the new schema. We don't want empty titles, hence the check. Don't know why it was checking for an optional attribute previously, doesn't make sense.
| log.debug("content: " + content); | ||
| InputStream inputStream = new ByteArrayInputStream(content.getBytes()); | ||
|
|
||
| // upload | ||
| HttpUpload put = new HttpUpload(inputStream, doiURL); | ||
| put.setRequestProperty("Authorization", "Basic " + Base64.encodeString(getCredentials())); | ||
| put.setBufferSize(64 * 1024); | ||
| put.setContentType("text/plain;charset=UTF-8"); | ||
| put.run(); | ||
| put.setRequestProperty("Content-Type", "text/plain;charset=UTF-8"); |
There was a problem hiding this comment.
HttpTransfer.CONTENT_TYPE?
| try { | ||
| groupsURI = localAuthority.getServiceURI(Standards.GMS_SEARCH_10.toString()); | ||
| Set<URI> groupsURIs = localAuthority.getServiceURIs(Standards.GMS_SEARCH_10); |
There was a problem hiding this comment.
Not sure why you need the GMS configured in the doi.properties when you use the one in cadc-registry.properties. Am I missing something?
There was a problem hiding this comment.
Removed the GMS configuration, no longer needed. Removed the int test properties file, and added a intTest README to explain the int test setup.
No description provided.