fcrepo-exts / fcrepo-camel-toolbox

A collection of ready-to-use messaging applications with fcrepo-camel
Apache License 2.0
13 stars 26 forks source link

FCREPO-3770: Allow to configure basic authentication for a triple store #167

Closed andyundso closed 2 years ago

andyundso commented 2 years ago

JIRA: https://fedora-repository.atlassian.net/browse/FCREPO-3770

What does this Pull Request do?

With this PR, the toolbox will be able to talk with triple stores that require basic authentication.

What's new?

How should this be tested?

Setup a Fedora as per README:

docker run -p8080:8080 --rm -p61616:61616  -p8181:8181 --name=my_fcrepo6  fcrepo/fcrepo:6.0.0

Run Zazuko's Fuseki:

docker run --name fuseki -p3030:3030 --rm -e ADMIN_PASSWORD=fuseki -e OTEL_JAVAAGENT_ENABLED=false zazuko/jena-fuseki:4.1.0 --mem /test

Create a properties file with the following properties:

triplestore.indexer.enabled=true
solr.indexer.enabled=true
triplestore.baseUrl=http://localhost:3030/test
triplestore.authUsername=admin
triplestore.authPassword=fuseki
solr.baseUrl=http://localhost:8983/solr/

Build the project (note that the tests currently don't work, so -DskipTests -DskipITs should be set) and run it:

mvn clean install
java -jar fcrepo-camel-toolbox-app/target/fcrepo-camel-toolbox-app-6.0.0-SNAPSHOT-driver.jar -c ./application.properties

Post some data to Fedora:

curl -X POST -u fedoraAdmin:fedoraAdmin -d "$(cat fcrepo-indexing-triplestore/src/test/resources/indexable.ttl)" http://localhost:8080/fcrepo/rest

Afterwards, check the logs of all components. The toolbox should log a successful request to Fedora and Fuseki. You can also do a SparQL query to verify that the data has been inserted.

Additional Notes:

I tried to copy the integration test which tests the update, but for whatever reason, the fcrepoMockEndpoint doesn't get its two required calls. I don't see why it shouldn't do that (I guess some magic in some property or some naming?). Additionally, running the integration test against Docker works normally. So I need some guidance where to find the error.

dbernstein commented 2 years ago

@andyundso : Thanks - I'll take a look at this today.

andyundso commented 2 years ago

@dbernstein thanks for the review, will implement this.

I think it would sense to hold this PR until #165 is merged. This way we can avoid merge-conflicts plus I could add an integration test there as well.

demiankatz commented 2 years ago

@tomcbe, regarding whether it's enough to use AuthUsername property to decide whether or not to add a Basic Auth header, it seems reasonable as long as we only offer one authentication method. In the future, we might need another property to specify authentication type, at which point the logic might need more nuance. It seems to me this could be added in a back-compatible way if needed, though.

Also note that #166 borrows its basic auth functionality from here, so if things have changed (or are going to change), we should double check that everything remains consistent there.

tomcbe commented 2 years ago

@tomcbe, regarding whether it's enough to use AuthUsername property to decide whether or not to add a Basic Auth header, it seems reasonable as long as we only offer one authentication method. In the future, we might need another property to specify authentication type, at which point the logic might need more nuance. It seems to me this could be added in a back-compatible way if needed, though.

Also note that #166 borrows its basic auth functionality from here, so if things have changed (or are going to change), we should double check that everything remains consistent there.

Thanks for the hint @demiankatz. I propose to keep the way authentication is handled like this for now. We can still decide to move the code to generate the Basic Auth header into a helper class. But that will be easier, once this PR and #166 have been merged.

tomcbe commented 2 years ago

@dbernstein You marked this PR with "Changes requested". I think all the remaining issues have been resolved by @andyundso yesterday, could you check again?

andyundso commented 2 years ago

@dbernstein PR is ready to be merged.

andyundso commented 2 years ago

weird that the pipeline fails, I ran the same code on my fork's main branch and that worked: https://github.com/andyundso/fcrepo-camel-toolbox/runs/3856286632