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

Make user identities available as URIs in kernel API. #1242

Merged
merged 2 commits into from Sep 21, 2017

Conversation

lsitu
Copy link
Contributor

@lsitu lsitu commented Sep 20, 2017

Fixes #https://jira.duraspace.org/browse/FCREPO-2608

Make user identities available as URIs in kernel API through method getUserAgent().

Copy link
Contributor

@birkland birkland left a comment

Choose a reason for hiding this comment

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

Very good work. I would only ask that you consider adding a default base URI to the one-click jar, and adding a logging statement whenever string userid is converted to a URI (since that could be dangerous, or create unintended consequences, in some circumstances).

final ServletCredentials credentials = new ServletCredentials(request);
final FedoraSession session = repo.login(credentials);

// should thrown for unsupported digest algorithm sha256
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this the correct comment? I don't see how sha256 is involved

// Construct the default URI for the user ID that is not a URI.
final String userAgentBaseUri = System.getProperty(USER_AGENT_BASE_URI_PROPERTY);
if (isNullOrEmpty(userAgentBaseUri)) {
throw new RepositoryConfigurationException("User agent base URI prioperty is missing."
Copy link
Contributor

@birkland birkland Sep 20, 2017

Choose a reason for hiding this comment

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

This will make it required to provide a system property in order to support string user names. I believe this is a good thing. That being said, I think we should:

  • mark this as a breaking change (since somebody who deploys Fedora may have to define this new property)
  • provide a default base URI for the one-click jar in fcrepo-webapp, since it is intended for demonstration purposes, and should be easy to use. (something like info:fedora/local-user#)
  • log (either DEBUG or INFO) that a URI was created from a string.

Would you be able to add the last two bullets to this PR, if you agree it is appropriate?

Copy link
Contributor Author

@lsitu lsitu Sep 20, 2017

Choose a reason for hiding this comment

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

@birkland Thanks. I've added the logging for the default user URI created. I tried to add the default URI to jetty-console-maven-plugin but I am not able to get it works. Do you have any working examples that can point me to?

Here is I was trying:
<configuration>
...
<systemProperties>
<systemProperty>
<name>fcrepo.auth.webac.userAgent.baseUri
<value>http://fedora.info/definitions/v4/person#
</systemProperty>
</systemProperties>
</configuration>

Copy link
Contributor

@birkland birkland Sep 21, 2017

Choose a reason for hiding this comment

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

@lsitu In the profile for one-click, there is a <properties> section that currently defines fcrepo.modeshape.configuration. Somehow, it influences the resulting jar in such a manner that results in that property being set when it is run. Glancing at the code, I have no idea how or why that works, but it does. Somehow, you'll need to do the same thing with fcrepo.auth.webac.userAgent.baseUri as happens with fcrepo.modeshape.configuration.

The first step would be to examine fcrepo-webapp/pom.xml and put fcrepo.auth.webac.userAgent.baseUri everywhere fcrepo.modeshape.configuration occurs.

Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if any @fcrepo4/committers have any recollection on how the one-click jar gets the fcrepo.modeshape.configuration property, based on setting it in the pom.

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

@birkland Oh, I think that's being set by the oneclick profile in pom.xml: https://github.com/fcrepo4/fcrepo4/blob/master/fcrepo-webapp/pom.xml#L494

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@birkland I've updated it to use the default user URI as we discussed above instead of throwing the exception. See commit 422a95f.

Copy link
Contributor

Choose a reason for hiding this comment

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

@escowles Right, it's set in the pom; but what is the mechanism by which that value affects the generated artifact in a manner such that Spring will know about its value? In other words, the value clearly gets recorded somewhere in the jar file. I'm wondering where, how, and by what mechanism does Spring discover it? (this won't block the PR, but I'm really curious)

Copy link
Contributor

Choose a reason for hiding this comment

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

@lsitu Thanks, looks good

Copy link
Contributor

Choose a reason for hiding this comment

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

@birkland Sorry to be so obtuse about this: I think that the Maven resources filtering does that part: https://github.com/fcrepo4/fcrepo4/blob/master/fcrepo-webapp/pom.xml#L499-L505

@birkland birkland merged commit 55acd4b into fcrepo:master Sep 21, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants