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

NXP-29759: Make the task endpoint use the UserManagerResolver #4384

Conversation

charlesboidot
Copy link
Contributor

Fix for NXP-29759: Make the task endpoint use the UserManagerResolver

@charlesboidot charlesboidot requested a review from a team as a code owner October 14, 2020 16:02
@nuxeojenkins
Copy link
Collaborator

View issue in JIRA: NXP-29759: Make the task endpoint use the UserManagerResolver

@ataillefer ataillefer removed the request for review from a team October 19, 2020 07:45
kevinleturc
kevinleturc previously approved these changes Oct 20, 2020
@charlesboidot charlesboidot force-pushed the fix-NXP-29759-make-task-endpoint-use-UserManagerResolver branch 4 times, most recently from e960bd4 to 4d92a07 Compare October 21, 2020 15:30
@charlesboidot charlesboidot force-pushed the fix-NXP-29759-make-task-endpoint-use-UserManagerResolver branch from 4d92a07 to bc7dd33 Compare October 22, 2020 08:07
* complete informations on this principal (the missing attributes will be if the principal is an
* Administrator and the groups he is in).
*/
boolean isComplete();
Copy link
Member

Choose a reason for hiding this comment

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

Adding this kind of method on public and important API like that is an architecture change that should be discussed first. Please open a discussion on Slack in #team-platform about this, because I'm really not sure it's the best way to move forward.

Copy link
Member

Choose a reason for hiding this comment

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

Cc @nuxeo/platform

@efge
Copy link
Member

efge commented Oct 22, 2020

Apart from the architecture aspects I mentioned above, I don't understand from the patch who is the intended user of the new "isComplete" that is being returned in the JSON. Is there an associated ticket on the UI side that shows the goal?

@efge
Copy link
Member

efge commented Oct 22, 2020

If we want to add additional flags to the principal, then two things:

  • I'd prefer if the flag was false by default on a complete instance, so named instead "isIncomplete" or "isPartial". But the doc and maybe the naming should be clear about in what way it's incomplete, what's the difference between a complete and incomplete instance?
  • Setting the flag should be done at a lower level than in UserManagerResolver. It should probably be done in UserManagerImpl.makePrincipal when the caller (higher in the stack) has fetchReferences=false.

Copy link
Member

@troger troger left a comment

Choose a reason for hiding this comment

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

I agree with @efge, nothing should be done in UserManagerResolver. What about removing the public API but just use a simple flag in the underlying DocumentModel through #putContextData?

Could be done in UserManagerImpl#getUserModel if fetchReferences is false, and then this flag will be read by the NuxeoPrincipalWriter to set accordingly the isPartial field?

@charlesboidot
Copy link
Contributor Author

If we want to add additional flags to the principal, then two things:

* I'd prefer if the flag was false by default on a complete instance, so named instead "isIncomplete" or "isPartial". But the doc and maybe the naming should be clear about in what way it's incomplete, what's the difference between a complete and incomplete instance?

* Setting the flag should be done at a lower level than in `UserManagerResolver`. It should probably be done in `UserManagerImpl.makePrincipal` when the caller (higher in the stack) has `fetchReferences=false`.

The difference between a complete and a partial principal are that in the case where we don't want to fetch the references we can't tell the groups a principal is in and without the groups we can't determine if he is an Administrator. In this case we will have two incorrect entries for the principal: the groups will be empty and the isAdministrator value will be false, because without fetching the groups we have no way of knowing either one.

For the second issue it could be done even inside the getUserManager().getPrincipal() method if the fetchReferences boolean is false.

@charlesboidot charlesboidot deleted the fix-NXP-29759-make-task-endpoint-use-UserManagerResolver branch October 28, 2020 14:18
@charlesboidot charlesboidot restored the fix-NXP-29759-make-task-endpoint-use-UserManagerResolver branch October 28, 2020 14:18
@charlesboidot charlesboidot deleted the fix-NXP-29759-make-task-endpoint-use-UserManagerResolver branch October 28, 2020 14:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
7 participants