airavata-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Thejaka Amila Kanewala" <thejaka.am...@gmail.com>
Subject Re: Review Request 15789: Add credential store functionalities to the Airavata API
Date Fri, 22 Nov 2013 21:50:10 GMT

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15789/#review29314
-----------------------------------------------------------


Generally, try to add test cases where every there is significant logic.


https://svn.apache.org/repos/asf/airavata/trunk/modules/credential-store/src/main/java/org/apache/airavata/credential/store/credential/impl/ssh/SSHCredentialGenerator.java
<https://reviews.apache.org/r/15789/#comment56480>

    We decided to generate pass phrase based on DN and a random value. So the pass phrase
= HASH(DN + randomvalue).
    We only store randomvalue in credential store.



https://svn.apache.org/repos/asf/airavata/trunk/modules/rest/service/src/main/java/org/apache/airavata/services/registry/rest/resources/CredentialRegistryResource.java
<https://reviews.apache.org/r/15789/#comment56485>

    token should not be user id.



https://svn.apache.org/repos/asf/airavata/trunk/modules/airavata-client/src/main/java/org/apache/airavata/client/api/CredentialStoreManager.java
<https://reviews.apache.org/r/15789/#comment56476>

    I am not sure why do you want to get credentials through the API. I dont think its a good
idea to expose a function to get credentials through API.



https://svn.apache.org/repos/asf/airavata/trunk/modules/airavata-client/src/main/java/org/apache/airavata/client/api/CredentialStoreManager.java
<https://reviews.apache.org/r/15789/#comment56477>

    persistCredentials is a more appropriate name (?). To create credentials we need more
information. (Mainly when storing them in the credential store). 
    E.g :- admin username, admin email. 
    
    In addition to public key we also need to include "fromIp" like configuration. (To restrict
access resource)



https://svn.apache.org/repos/asf/airavata/trunk/modules/airavata-client/src/main/java/org/apache/airavata/client/api/CredentialStoreManager.java
<https://reviews.apache.org/r/15789/#comment56478>

    same comments apply here as previous



https://svn.apache.org/repos/asf/airavata/trunk/modules/airavata-client/src/main/java/org/apache/airavata/client/impl/CredentialStoreManagerImpl.java
<https://reviews.apache.org/r/15789/#comment56479>

    As mentioned in a previous comment we should not expose credentials through API.



https://svn.apache.org/repos/asf/airavata/trunk/modules/registry/airavata-jpa-registry/src/main/java/org/apache/airavata/persistance/registry/jpa/impl/AiravataJPARegistry.java
<https://reviews.apache.org/r/15789/#comment56481>

    Again not a fan of having this method.



https://svn.apache.org/repos/asf/airavata/trunk/modules/registry/airavata-jpa-registry/src/main/java/org/apache/airavata/persistance/registry/jpa/impl/AiravataJPARegistry.java
<https://reviews.apache.org/r/15789/#comment56482>

    token should not be username. There is a method to generate token id in credential store.



https://svn.apache.org/repos/asf/airavata/trunk/modules/registry/airavata-jpa-registry/src/main/java/org/apache/airavata/persistance/registry/jpa/impl/AiravataJPARegistry.java
<https://reviews.apache.org/r/15789/#comment56483>

    log and throw exception. Applies to other places also



https://svn.apache.org/repos/asf/airavata/trunk/modules/registry/registry-api/src/main/java/org/apache/airavata/registry/api/CredentialRegistry.java
<https://reviews.apache.org/r/15789/#comment56484>

    All comments mentioned earlier also apply here. Mainly credential retrieval.


- Thejaka Amila Kanewala


On Nov. 22, 2013, 7:43 p.m., Viknes Balasubramanee wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/15789/
> -----------------------------------------------------------
> 
> (Updated Nov. 22, 2013, 7:43 p.m.)
> 
> 
> Review request for Airavata.
> 
> 
> Bugs: Airavata-952
>     https://issues.apache.org/jira/browse/Airavata-952
> 
> 
> Repository: Airavata
> 
> 
> Description
> -------
> 
> Add credential store functionalities to the Airavata API
> 
> 
> Diffs
> -----
> 
>   https://svn.apache.org/repos/asf/airavata/trunk/modules/airavata-client/src/main/java/org/apache/airavata/client/AiravataClient.java
1541741 
>   https://svn.apache.org/repos/asf/airavata/trunk/modules/airavata-client/src/main/java/org/apache/airavata/client/api/AiravataAPI.java
1541741 
>   https://svn.apache.org/repos/asf/airavata/trunk/modules/airavata-client/src/main/java/org/apache/airavata/client/api/CredentialStoreManager.java
PRE-CREATION 
>   https://svn.apache.org/repos/asf/airavata/trunk/modules/airavata-client/src/main/java/org/apache/airavata/client/impl/CredentialStoreManagerImpl.java
PRE-CREATION 
>   https://svn.apache.org/repos/asf/airavata/trunk/modules/credential-store/pom.xml 1541741

>   https://svn.apache.org/repos/asf/airavata/trunk/modules/credential-store/src/main/java/org/apache/airavata/credential/store/credential/impl/ssh/SSHCredential.java
PRE-CREATION 
>   https://svn.apache.org/repos/asf/airavata/trunk/modules/credential-store/src/main/java/org/apache/airavata/credential/store/credential/impl/ssh/SSHCredentialGenerator.java
PRE-CREATION 
>   https://svn.apache.org/repos/asf/airavata/trunk/modules/credential-store/src/main/java/org/apache/airavata/credential/store/store/impl/SSHCredentialWriter.java
PRE-CREATION 
>   https://svn.apache.org/repos/asf/airavata/trunk/modules/credential-store/src/main/java/org/apache/airavata/credential/store/store/impl/db/CredentialsDAO.java
1541741 
>   https://svn.apache.org/repos/asf/airavata/trunk/modules/registry/airavata-jpa-registry/pom.xml
1541741 
>   https://svn.apache.org/repos/asf/airavata/trunk/modules/registry/airavata-jpa-registry/src/main/java/org/apache/airavata/persistance/registry/jpa/impl/AiravataJPARegistry.java
1541741 
>   https://svn.apache.org/repos/asf/airavata/trunk/modules/registry/registry-api/src/main/java/org/apache/airavata/registry/api/AiravataRegistry2.java
1541741 
>   https://svn.apache.org/repos/asf/airavata/trunk/modules/registry/registry-api/src/main/java/org/apache/airavata/registry/api/CredentialRegistry.java
PRE-CREATION 
>   https://svn.apache.org/repos/asf/airavata/trunk/modules/rest/client/src/main/java/org/apache/airavata/rest/client/CredentialStoreResourceClient.java
PRE-CREATION 
>   https://svn.apache.org/repos/asf/airavata/trunk/modules/rest/client/src/main/java/org/apache/airavata/rest/client/RegistryClient.java
1541741 
>   https://svn.apache.org/repos/asf/airavata/trunk/modules/rest/mappings/src/main/java/org/apache/airavata/rest/mappings/utils/ResourcePathConstants.java
1541741 
>   https://svn.apache.org/repos/asf/airavata/trunk/modules/rest/service/src/main/java/org/apache/airavata/services/registry/rest/resources/CredentialRegistryResource.java
PRE-CREATION 
>   https://svn.apache.org/repos/asf/airavata/trunk/modules/rest/service/src/main/java/org/apache/airavata/services/registry/rest/resources/UserRegistryResource.java
1541741 
> 
> Diff: https://reviews.apache.org/r/15789/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Viknes Balasubramanee
> 
>


Mime
View raw message