[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:
Relates
relates to MGNLUI-3671 Add the userName to the command-param... Closed
causality
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:

  1. Log in as eric
  2. Publish a single page
  3. Observe the version history and see eric's name
  4. Now publish a page w/ subnodes
  5. Observe the version history and see superuser


 Comments   
Comment by Oanh Thai Hoang [ 26/Oct/15 ]

Hi mgeljic, had

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
There are related modules need to be altered: scheduler, magnolia-core

1. There is my understanding:
This issue happened when user does async action:

  • AbstractCommandAction will be called
  • DefaultAsyncActionExecutor will be called and started the job by scheduler module
  • CommandJob will be called to execute corresponding commands
  • SimpleContext (real instance is SystemContext) is set to corresponding command -> because using SystemContext so superuser is the creator of version in VersionCommand

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:
The root cause is CommandJob create a SimpleContext (real instance is SystemContext) so I want to fix deeply in this case so it should not be problem for doing async later

  • First: Alter SimpleContext class

+ 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

  • Second: Alter CommandJob class

+ 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));
  • There is one case we need to consider like start Command in ObservationUtil module also. But I think it will be another ticket

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:

  • what if user doesn't exist anymore (we retrieve user by name)
  • what if command gets reused ... context and other things might not be valid anymore if/when injected. Are those objects proxied? Are we sure they are always "clean"? Specially system context. That's thread dependent. (should be the case, but i'm not 100% sure)
  • if command gets reused, and on different thread, that would definitively be invalid.
  • would feel much better if there was integration test as well.

for changes in context itself:

  • It should be fine, personally I would maybe invert the condition and call super only as fallback in case user is not set, but that's just me so feel free to ignore this remark

the test:

  • as mentioned above I'd love to see integration test for this issue as well
  • junit test the "when" is not really a "when" but rather "then" since the job result is captured only after execution of the commands and the execution of it is the real "when" ... as in WHEN i execute the command job THEN I capture the result and fail if result is not what I expect (true).
  • I would like to see a test case where we also try to modify from the command job the node to which user who is requestor have no write access. IIRC, the ctx.getUser() is called from AccessManagerImpl to test the permissions so I would suspect that in such setting test that would have been previously passing will fail. If this is indeed the case, we need to evaluate that this is correct behavior (I think it is). Again this might be easier implemented as integration test rather than unit test. Unless anything changed since 4.5, RepositoryTestCase is still skipping the security checks so it can't be used for this kind of 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 ]

Hi mgeljic, had

While working with integration test for previous solution . I would like provide another simple solution . Of course it will fix this issue (the issue happens with publish async action) as well

The problem part is here:
This part work correctly if we don't use the async action

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 ]

Hi mgeljic, had

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:

  • Log in as superuser
  • Choose 1 page has child
  • Click to Test Publish action (This is custom action to modify the requester name)
  • Observe the version history

Expectation:

  • See oriole's name (oriole is one test user)

Result:

  • See oriole's name

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:

  • Prepare action to modify context:
    public class TestActivationAction extends ActivationAction {
        @Override
        protected Map<String, Object> buildParams(final Item jcrItem) {
            Map<String, Object> params = super.buildParams(jcrItem);
            // Change user has no write access in this instance
            params.put(Context.ATTRIBUTE_REQUESTOR, "oriole");
            return params;
        }
    }
    
  • Prepare test case to ensure oriole will be observed instead of superuser in version history:
            // CHECK VERSIONS
            getActionBarItem(SHOW_VERSIONS_ACTION).click();
    
            // CHECK THE TAB HEADER
            delay("Waiting before check");
            // Click on version drop-down to show versions
            getSelectTabElement("Version").click();
    
            // Check the first element in dropdown and make sure it contains eric
            WebElement table = getSelectedTableElement();
            assertTrue(table.findElements(By.xpath("tbody/tr/td")).get(0).getText().contains("(oriole)"));
    
            // CHECK THE TAB HEADER
            delay("Waiting before check");
            // Click on version drop-down to show versions
            getSelectTabElement("Version").click();
    
            // Check the first element in dropdown and make sure it contains eric
            WebElement table = getSelectedTableElement();
            assertTrue(table.findElements(By.xpath("tbody/tr/td")).get(0).getText().contains("(oriole)"));
    

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.
B. Fix only the versioning use case, by using requestor explicitly.

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:

  • has indeed more potential impact
  • more consistent with synchronous execution
  • prevents operations where requestor doesn't have permission

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):

  • The main purpose of the scheduler module is to trigger Cron jobs, some of them being backed by commands.
  • The UI just "hijacked" this functionality for async actions.
    • Async actions will eventually be rewritten at some point.

For the code discussion and review, please emit a PR (I won't comment here nor on the gists).
For testing, I think writing an integration/UI test might be overly complex. The permission test should be doable as a unit test imo.

Thanks for your patience!

Comment by Espen Jervidalo [ 24/Nov/15 ]

For the records:
I agree on all points there. Concerning the leaking "requestor": I don't like the naming, and I don't like that it was moved from WorkflowConstants to Context.
On the other hand, CommandJob already reads:

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.
Ideally we would not use SystemContext in that case, but set-up a proper user-context with the correct permissions.

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.

Generated at Mon Feb 12 04:14:20 CET 2024 using Jira 9.4.2#940002-sha1:46d1a51de284217efdcb32434eab47a99af2938b.