[MAGNOLIA-6419] Publish including subnodes always shows superuser as version creator Created: 17/Oct/15 Updated: 30/Nov/15 Resolved: 25/Nov/15 |
|
| Status: | Closed |
| Project: | Magnolia |
| Component/s: | None |
| Affects Version/s: | 5.3.11, 5.4.2 |
| Fix Version/s: | 5.3.12, 5.4.4 |
| Type: | Bug | Priority: | Neutral |
| Reporter: | Richard Gange | Assignee: | Espen Jervidalo |
| Resolution: | Fixed | Votes: | 0 |
| Labels: | support | ||
| Remaining Estimate: | 0d | ||
| Time Spent: | 5d 1h | ||
| Original Estimate: | 4d | ||
| Issue Links: |
|
||||||||||||
| Template: |
|
||||||||||||
| Acceptance criteria: |
Empty
|
||||||||||||
| Task DoD: |
[ ]*
Doc/release notes changes? Comment present?
[ ]*
Downstream builds green?
[ ]*
Solution information and context easily available?
[ ]*
Tests
[ ]*
FixVersion filled and not yet released
[ ] 
Architecture Decision Record (ADR)
|
||||||||||||
| Bug DoR: |
[ ]*
Steps to reproduce, expected, and actual results filled
[ ]*
Affected version filled
|
||||||||||||
| Date of First Response: | |||||||||||||
| Sprint: | Basel 20 | ||||||||||||
| Story Points: | 8 | ||||||||||||
| Description |
|
To see this issue:
|
| Comments |
| Comment by Oanh Thai Hoang [ 26/Oct/15 ] |
|
This is my understanding after investigating: According to ticket, this is systematic issue and I reproduced bug on version 5.3.11 and 5.4.2 1. There is my understanding:
Here is unit test for reproducing also:
@Test
public void testUserInCommandJobIsCorrectUser() throws JobExecutionException {
// GIVEN
CommandJob cj = new CommandJob();
final Exception exception = new Exception();
JobDataMap jobDataMap = new JobDataMap();
jobDataMap.put(SchedulerConsts.CONFIG_JOB_COMMAND, "testCommand");
jobDataMap.put(SchedulerConsts.CONFIG_JOB_COMMAND_CATALOG, "testCatalog");
Map params = new HashMap<String, Object>();
params.put(SchedulerConsts.ATTRIBUTE_REQUESTOR, "eric");
jobDataMap.put(SchedulerConsts.CONFIG_JOB_PARAMS, params);
MockContext context = new MockContext();
User user = mock(User.class);
when(user.getName()).thenReturn("eric");
context.setUser(user);
MgnlContext.setInstance(context);
when(userManager.getUser("eric")).thenReturn(user);
final String username = MgnlContext.getUser().getName();
when(jobCtx.getJobDetail()).thenReturn(jobDetail);
when(jobDetail.getName()).thenReturn("Test Job");
when(jobCtx.getJobDetail().getJobDataMap()).thenReturn(jobDataMap);
when(commandsManager.getCommand("testCatalog", "testCommand")).thenReturn(new Command() {
@Override
public boolean execute(Context context) throws Exception {
if (MgnlContext.getUser().getName().equalsIgnoreCase(username)) {
return true;
}
throw exception;
}
@Override
public Command clone() throws CloneNotSupportedException {
return null;
}
});
// WHEN
final ArgumentCaptor<CommandJob.JobResult> argumentCaptor =
ArgumentCaptor.forClass(CommandJob.JobResult.class);
// THEN
try {
cj.execute(jobCtx);
} catch (JobExecutionException e) {
// THEN
fail("user context is not correct " + username);
}
}
2. There are my approach:
+ Create new SimpleContext constructor
public SimpleContext(Map<String, Object> map, User user) {
this(map);
this.user = user;
}
+ Alter getUser() function public User getUser() { if (this.user == null) { return this.ctx.getUser(); } else { return this.user; } } + From now on, SimpleContext can get correct user if we use a new constructor
+ Get username from params if (jobData.containsKey(SchedulerConsts.CONFIG_JOB_PARAMS) && jobData.get(SchedulerConsts.CONFIG_JOB_PARAMS) != null) { Map params = (Map) jobData.get(SchedulerConsts.CONFIG_JOB_PARAMS); jobClusterId = (String) params.get("clusterId"); if (params.containsKey(SchedulerConsts.ATTRIBUTE_REQUESTOR)) { String requester = (String) params.get(SchedulerConsts.ATTRIBUTE_REQUESTOR); user = securitySupport.getUserManager().getUser(requester); } } + Alter code to init context
MgnlContext.setInstance(new SimpleContext(systemContext, user));
For more details, I have a patch fix, please feel free to review https://gist.github.com/oanhthai/74ab4d72aa50a4b79120 |
| Comment by Jan Haderka [ 03/Nov/15 ] |
|
Scheduler changes:
for changes in context itself:
the test:
|
| Comment by Mikaël Geljić [ 03/Nov/15 ] |
|
Moving to 5.4.4; too short timespan to complete and evaluate side-effects if any. |
| Comment by Oanh Thai Hoang [ 09/Nov/15 ] |
|
had, Thank you so much for your advice I agree with you it maybe run incorrectly if the command gets reused and on different thread. I will do integration test for this issue to ensure that we can use it later. I'll inform you when I finish the integration test. According to the case for junit test, i didn't quite get your idea. It would be great if you could elaborate it for me. Thanks in advance |
| Comment by Oanh Thai Hoang [ 09/Nov/15 ] |
|
While working with integration test for previous solution The problem part is here: private String getSafelyUserNameFromMgnlContext() { String userName = ""; if (MgnlContext.getUser() != null) { userName = MgnlContext.getUser().getName(); } return userName; } My approach is providing a new API in BaseVersionManager class: Version addVersion(final Node node, final Rule rule, final String userName) And username will be getting from VersionCommand class: String userName = null; if (ctx.containsKey(Context.ATTRIBUTE_REQUESTOR) && ctx.get(Context.ATTRIBUTE_REQUESTOR) != null) { userName = ctx.get(Context.ATTRIBUTE_REQUESTOR).toString(); } Here is my patch: https://gist.github.com/oanhthai/613a11449f1c2cd26c69. Please feel free to review it |
| Comment by Oanh Thai Hoang [ 11/Nov/15 ] |
|
As previous comments, I will create one integration test case where we try to modify from the context the requester name to which user who have no write access. This test case just supports for my solution 1 (which will alter code in Scheduler and Simple Context). Test case:
Expectation:
Result:
The oriole name instead of superuser is observed because I do make a change to force to keep another user name whose user has read-only permission in pages app. There are some important parts to prepare test case:
For more detail about test case, please feel free to review in https://gist.github.com/oanhthai/bac0ab9df22793efcb2a. It includes the solution 1 and integration test. Magnolia version is 5.3.12-SNAPSHOT. |
| Comment by Mikaël Geljić [ 11/Nov/15 ] |
|
This was pretty long to digest, so I summarize the two approaches: A. Fix command job so that all *async* commands get executed "on behalf" of the original requestor. In any case, bear in mind that the ATTRIBUTE_REQUESTOR is a UI detail. Aside from the AbstractCommandAction, commands can be executed directly via the CommandsManager, so we have no guarantee that this attribute will always be available. Anyway, I tend to prefer option A:
I'm not that fond of the requestor notion "leaking" into the scheduler module, but I don't see an easy way around that (at least not under maintenance):
For the code discussion and review, please emit a PR (I won't comment here nor on the gists). Thanks for your patience! |
| Comment by Espen Jervidalo [ 24/Nov/15 ] |
|
For the records: Context.ATTRIBUTE_PATH Context.ATTRIBUTE_REPOSITORY Context.ATTRIBUTE_UUID Only to resolve the UUID from the path. It shouldn't. The implementation assumes we're dealing with some sort of BaseRepositoryCommand. But that's another topic.. I don't care about it reading another property from the context, but I don't like adding the user to the SimpleContext. I would read the user from the parameter map and pass it to info.magnolia.cms.core.version.BaseVersionManager#addVersion() VersionCommand#setUser and VersionCommand#getUser BaseVersionManager#addVersion(final Node node, final Rule rule, final String userName) As Mika already pointed out: by adding the user to SimpleContext, we will have inconsistency between the username and the permissions used to access JCR. |
| Comment by Philip Mundt [ 25/Nov/15 ] |
|
Issue was reopen when ejervidalo declined pull request #4 (and oldie) on Bitbucket Server – Some min. ago. Re-closing this issue. |