[SANTUARIO-555] Order of output processors in a chain is not deterministic

• Type: Bug
• Status: Closed
• Priority: Minor
• Resolution: Fixed
• Affects Version/s: Java 2.2.0
• Fix Version/s: Java 2.3.0
• Component/s: Java
• Assignee: Colm O hEigeartaigh
• Reporter: Peter De Maeyer
• Created: 12/Sep/20 19:54
• Updated: 12/Nov/21 09:34
• Resolved: 11/Dec/20 07:10

Description

Output processors and their chains are a powerful feature once you understand how they work.
The partial order of output processors is defined by:

1. Their phase.
2. Their "before" and "after" output processors - before and after which other output processors they are ordered based on classes.

However, the mechanism has some subtle bugs:

1. The partial order may be violated if the output processors are not added in the right (list) order. The root cause is a bug in the implementation of OutputProcessorChainImpl.addProcessor(OutputProcessor), which only considers the "before" and "after" of the new output processor to add, whilst ignoring the "before" and "after" of the output processors which were already added. That makes the outcome very hard to predict. The implementation of that method is generally unintuitive and hard to read - it can be simplified a lot whilst making it more readable.
2. When independent "groups" of output processors are added, e.g. a group of encryption output processors and a group of signature output processors, they may get intermingled in the wrong way. In the current implementation it works "by accident" because of the above bug.

Acceptance criterion

Given

• InitialOutputProcessor N and FinalOutputProcessor F provide the basics of an output processor chain. N knows that it comes before F.
• MyCustomOutputProcessor C is an optional processor that adds a feature - N and F don't need to know about it, but C knows that, if it's added, it comes in between N and F.

When
N, F and C are added in any random order

Then
The resulting order of output processors in the chain is always N < C < F.

Test case to illustrate this

    @Test
    public void testOrderOfProcessorsIsIndependentOfWhenTheyAreAddedToTheChain() {
        AbstractOutputProcessor outputProcessor1 = new AbstractOutputProcessor() {
        };
        AbstractOutputProcessor outputProcessor2 = new AbstractOutputProcessor() {
        };
        AbstractOutputProcessor outputProcessor3 = new AbstractOutputProcessor() {
        };
        outputProcessor1.addBeforeProcessor(outputProcessor3.getClass());
        outputProcessor2.addBeforeProcessor(outputProcessor3.getClass());
        outputProcessor2.addAfterProcessor(outputProcessor1.getClass());
        outputProcessor3.addAfterProcessor(outputProcessor1.getClass());

        OutputProcessorChain outputProcessorChain1 = new OutputProcessorChainImpl(new OutboundSecurityContextImpl());
        outputProcessorChain1.addProcessor(outputProcessor1);
        outputProcessorChain1.addProcessor(outputProcessor2);
        outputProcessorChain1.addProcessor(outputProcessor3);

        List<OutputProcessor> processors1 = outputProcessorChain1.getProcessors();
        assertEquals(outputProcessor1, processors1.get(0));
        assertEquals(outputProcessor2, processors1.get(1));
        assertEquals(outputProcessor3, processors1.get(2));

        OutputProcessorChain outputProcessorChain2 = new OutputProcessorChainImpl(new OutboundSecurityContextImpl());
        outputProcessorChain2.addProcessor(outputProcessor1);
        outputProcessorChain2.addProcessor(outputProcessor3);
        outputProcessorChain2.addProcessor(outputProcessor2);

        List<OutputProcessor> processors2 = outputProcessorChain1.getProcessors();
        assertEquals(outputProcessor1, processors2.get(0));
        assertEquals(outputProcessor2, processors2.get(1));
        assertEquals(outputProcessor3, processors2.get(2));
    }

When fixing this, SignatureEncryptionTest started failing.
The fix uncovers that the previous implementation "implicitly worked by accident", and we need to find an additional solution.
The problem is now that when multiple output processors corresponding to multiple unrelated actions SIGNATURE and ENCRYPTION are combined in a single chain, the result is not ordered as expected.
We don't want to hardcode that signature output processors come before encryption output processors, because both the use cases SIGNATURE followed by ENCRYPTION and ENCRYPTION followed by SIGNATURE are valid.
In fact, we want "groups" of output processors per action to know about others in the same "group", i.e. encryption output processors know their relation towards other encryption output processors, but do not need to know about the existence of signature output processors.
The solution here is to group based on runtime action - the order in which the action is defined in the list of actions.

Test case to illustrate this

    @Test
    public void testActionOrderOfProcessorsGroupsThemTogether() {
        AbstractOutputProcessor finalOutputProcessor = new AbstractOutputProcessor() {
        };
        finalOutputProcessor.setAction(null, -1);

        AbstractOutputProcessor initialEncryptionOutputProcessor = new AbstractOutputProcessor() {
        };
        initialEncryptionOutputProcessor.setAction(XMLSecurityConstants.ENCRYPTION, 0);
        initialEncryptionOutputProcessor.addBeforeProcessor(finalOutputProcessor.getClass());

        AbstractOutputProcessor myEncryptionOutputProcessor = new AbstractOutputProcessor() {
        };
        myEncryptionOutputProcessor.setAction(XMLSecurityConstants.ENCRYPTION, 0);
        myEncryptionOutputProcessor.addBeforeProcessor(finalOutputProcessor.getClass());
        myEncryptionOutputProcessor.addAfterProcessor(initialEncryptionOutputProcessor.getClass());

        AbstractOutputProcessor initialSignatureOutputProcessor = new AbstractOutputProcessor() {
        };
        initialSignatureOutputProcessor.setAction(XMLSecurityConstants.SIGNATURE, 1);
        initialSignatureOutputProcessor.addBeforeProcessor(finalOutputProcessor.getClass());

        AbstractOutputProcessor mySignatureOutputProcessor = new AbstractOutputProcessor() {
        };
        mySignatureOutputProcessor.setAction(XMLSecurityConstants.SIGNATURE, 1);
        mySignatureOutputProcessor.addBeforeProcessor(finalOutputProcessor.getClass());
        mySignatureOutputProcessor.addAfterProcessor(initialSignatureOutputProcessor.getClass());

        OutputProcessorChain outputProcessorChain = new OutputProcessorChainImpl(new OutboundSecurityContextImpl());
        outputProcessorChain.addProcessor(finalOutputProcessor);
        outputProcessorChain.addProcessor(initialSignatureOutputProcessor);
        outputProcessorChain.addProcessor(mySignatureOutputProcessor);
        outputProcessorChain.addProcessor(myEncryptionOutputProcessor);
        outputProcessorChain.addProcessor(initialEncryptionOutputProcessor);

        List<OutputProcessor> outputProcessors = outputProcessorChain.getProcessors();
        assertEquals(initialEncryptionOutputProcessor, outputProcessors.get(0));
        assertEquals(myEncryptionOutputProcessor, outputProcessors.get(1));
        assertEquals(initialSignatureOutputProcessor, outputProcessors.get(2));
        assertEquals(mySignatureOutputProcessor, outputProcessors.get(3));
        assertEquals(finalOutputProcessor, outputProcessors.get(4));
    }

Issue Links

• Dependent: SANTUARIO-554 Use Class parameter instead of Object when defining OutputProcessor's "before" and "after"
• is required by: SANTUARIO-541 Indent secure XML elements the same way as the input XML
• links to: GitHub Pull Request #8
• links to: GitHub Pull Request #8
• links to: GitHub Pull Request #15