XWIKI-23915: Introduce a rest endpoint for user information#5199
XWIKI-23915: Introduce a rest endpoint for user information#5199pjeanjean wants to merge 2 commits intoxwiki:masterfrom
Conversation
* Introduce /user, /users and /users/id REST endpoints * Introduce a UserReferenceModelSerializer component * Implement UserReferenceModelSerializer for the document user store
| * @version $Id$ | ||
| */ | ||
| @Path("/wikis/{wikiName}/user") | ||
| public interface CurrentUserResource |
There was a problem hiding this comment.
I guess it should be unstable and some javadoc would be nice, I'm surprised by the way that checkstyle is not complaining about missing javadoc on the method?
There was a problem hiding this comment.
Checkstyle rules seem to be different in xwiki-platform-rest.
It definitely complained once I moved everything, now there is some Javadoc.
|
|
||
| DocumentReference userDocumentReference = this.xcontextProvider.get().getUserReference(); | ||
| if (userDocumentReference == null) { | ||
| throw new WebApplicationException(Response.Status.UNAUTHORIZED); |
There was a problem hiding this comment.
Hmmm if guest is trying to access that endpoint and is authorized to access the wiki, we could still display info saying it's guest, no?
There was a problem hiding this comment.
Knowing also that we have a GuestUserReference for it
There was a problem hiding this comment.
Yes, there is no reason forbid guest to access this resource.
surli
left a comment
There was a problem hiding this comment.
I'm surprised this API doesn't go to a new module xwiki-platform-user-rest, it's needed because we absolutely want to reuse this in xwiki-platform-rest-model, couldn't we have had a dependency to it?
|
|
||
| DocumentReference userDocumentReference = this.xcontextProvider.get().getUserReference(); | ||
| if (userDocumentReference == null) { | ||
| throw new WebApplicationException(Response.Status.UNAUTHORIZED); |
There was a problem hiding this comment.
Knowing also that we have a GuestUserReference for it
| } | ||
|
|
||
| try { | ||
| UserReference userReference = this.userReferenceResolver.resolve(userId, |
There was a problem hiding this comment.
On the other hand here you'd need to check that the user has proper credentials no?
There was a problem hiding this comment.
This check is actually done in the DocumentUserReferenceModelSerializer, because I assume that it would not be implemented in the same way for other user stores. But this should probably be documented explicitly in the Javadoc for UserReferenceModelSerializer.
There was a problem hiding this comment.
...Or it could also be done through a hasAccess() method in UserReferenceModelSerializer, probably cleaner this way.
xwiki-platform-core/pom.xml
Outdated
| <justification> | ||
| Change in generated class of the REST model to add models for user endpoints. | ||
| </justification> | ||
| <criticality>allowed</criticality> |
There was a problem hiding this comment.
I assume it was, it only changed a "@Seealso" in the generated Javadoc. But it's no longer necessary after the refactoring.
I also agree that we need to have each business domain provide its own REST endpoint. |
|
Just to be sure (because I can't see if the comment is on something specific), when you suggest moving the API to |
the endpoints and the serializers, the whole REST API for user basically |
* Move new components and resources to xwiki-platform-user * Add missing documentation
|
I finished moving the APIs and components to |
| @@ -177,6 +177,11 @@ public final class Relations | |||
| */ | |||
| public static final String CLIENT = "http://www.xwiki.org/rel/client"; | |||
|
|
|||
| /** | |||
| * Relation for links pointing to the user. | |||
| */ | |||
There was a problem hiding this comment.
Missing @since (and @Unstable I guess).
| * @version $Id$ | ||
| */ | ||
| @Role | ||
| public interface UserReferenceModelSerializer |
| <parent> | ||
| <groupId>org.xwiki.platform</groupId> | ||
| <artifactId>xwiki-platform-user-rest</artifactId> | ||
| <version>18.1.0-SNAPSHOT</version> |
There was a problem hiding this comment.
Master moved to 18.2.0-SNAPSHOT.
| private UserReferenceSerializer<String> stringUserReferenceSerializer; | ||
|
|
||
| @Inject | ||
| private Provider<UserReferenceModelSerializer> userReferenceModelSerializerProvider; |
| Collection<String> wikiMembers = this.wikiUserManager.getMembers(wikiName); | ||
|
|
||
| // We use streams to handle pagination. | ||
| wikiMembers.stream().map(userId -> { |
There was a problem hiding this comment.
You don't seem to make sure to return only users the current user is allowed to see.
| */ | ||
| @Component | ||
| @Singleton | ||
| public class UserReferenceModelSerializerProvider implements Provider<UserReferenceModelSerializer> |
There was a problem hiding this comment.
OK, I understand now why you request a Provider in the resources. The way we handle this use case generally is more by making the default UserReferenceModelSerializer component, redirect each call to the configured one. Makes it a bit easier to use.
| <parent> | ||
| <groupId>org.xwiki.platform</groupId> | ||
| <artifactId>xwiki-platform-user-rest</artifactId> | ||
| <version>18.1.0-SNAPSHOT</version> |
| <parent> | ||
| <groupId>org.xwiki.platform</groupId> | ||
| <artifactId>xwiki-platform-user</artifactId> | ||
| <version>18.1.0-SNAPSHOT</version> |
| @@ -39,6 +39,9 @@ | |||
| <module>xwiki-platform-user-profile</module> | |||
| <module>xwiki-platform-user-script</module> | |||
| <module>xwiki-platform-user-resource</module> | |||
| <module>xwiki-platform-user-rest</module> | |||
| <module>xwiki-platform-user-rest/xwiki-platform-user-rest-api</module> | |||
There was a problem hiding this comment.
That's not how Maven modules are declared usually. You are supposed to list xwiki-platform-user-rest-api and xwiki-platform-user-rest-default in xwiki-platform-user-rest's pom only. I'm surprised Maven allows this actually.
There was a problem hiding this comment.
Ouch, that's some IDE shenanigans I missed while reviewing my commit, sorry about that...
| public interface CurrentUserResource | ||
| { | ||
| /** | ||
| * Return all the available information on the user making the request. |
There was a problem hiding this comment.
I would provide info in the doc about the behaviour when the user is guest.
| public interface UserResource | ||
| { | ||
| /** | ||
| * Return all the available information on the user {userId}. |
There was a problem hiding this comment.
I would provide info about the required credentials to access those info.
| @@ -0,0 +1 @@ | |||
| PUBLIC "http://www.xwiki.org" "maven:org.xwiki.platform:xwiki-platform-rest-model:jar::!/xwiki.rest.model.xsd" | |||
There was a problem hiding this comment.
Hmm, no, it should be the right one. The catalog serves to import LinkCollection from the main REST model.
| /** | ||
| * Relation for links pointing to the user. | ||
| */ | ||
| public static final String USER = "http://www.xwiki.org/rel/user"; |
There was a problem hiding this comment.
Do you actually use it? Btw I'm surprised: there should be a dependency to xwiki-platform-user-rest somewhere if you want the model to reuse the UserSummary in some existing REST resources no?
There was a problem hiding this comment.
I use it only in the users list endpoint, to point to the full user profile endpoint.
It could also be used in the existing REST endpoints, but that's a larger refactoring that I don't think I would like to include in this PR.
| xcontext)); | ||
| user.setCompany(Objects.toString(userProperties.getProperty("company"), "")); | ||
| user.setAbout(Objects.toString(userProperties.getProperty("comment"), "")); | ||
| user.setEmail(Objects.toString(userProperties.getProperty("email"), "")); |
There was a problem hiding this comment.
Hmmm the email should probably be obfuscated depending on the admin settings, I don't remember if UserProperties check that I imagine it doesn't but might be wrong here
There was a problem hiding this comment.
Ah that's a good point.
|
|
||
| String underlineProperty = "underline"; | ||
| userPreferences.setUnderlineLinks(Objects.toString(userProperties.getProperty(underlineProperty), | ||
| xcontext.getWiki().getXWikiPreference(underlineProperty, xcontext))); |
There was a problem hiding this comment.
I have a doubt: here your context wiki might not be the wiki of the user no for the default preference?
Jira URL
https://jira.xwiki.org/browse/XWIKI-23915
Changes
Description
This PR adds 3 new REST endpoints:
/user/users/users/{userId}Considering that user references are generic, but the necessary data can only be fetched from the actual storage, this PR also relies on a new component:
UserReferenceModelSerializer.An implementation for the document user store was included, as well as a provider.
Clarifications
Proposal: https://forum.xwiki.org/t/add-a-users-rest-api-endpoint/17982
Design page: https://design.xwiki.org/xwiki/bin/view/Proposal/RESTAPIendpointsforusers
With this PR, only the document user store is supported. If an instance uses a different user store, the API endpoints will return a
501 Not ImplementedHTTP error with an appropriate error message.The current implemention of the
/userendpoint is not ideal, however, because it relies on thegetUserReference()of the context that returns aDocumentReferencedirectly and not aUserReference. As such, I currently don't know how to make it fully generic.Screenshots & Video
N/A
Executed Tests
The PR has been built and tested locally.
Expected merging strategy