CAM-4261 fix

[filiphr]

Save the exception properties consistently.

I modified the AuthorizationException so there is a consistent way of accessing the different number of Permissions.

Should there be some tests for this?

[buildhive]

camunda BPM » camunda-bpm-platform #1534 SUCCESS
This pull request looks good
(what's this?)

[ThorbenLindhauer]

Hi Filip,

Thanks for the pull request. Didn't expect that after creating the ticket two days ago :)

I'll have a look at it in the next days. As for tests, it would be great if you could add some assertions to https://github.com/camunda/camunda-bpm-platform/blob/master/engine/src/test/java/org/camunda/bpm/engine/test/authorization/util/AuthorizationScenarioInstance.java#L95-L99

This is used in the authorization test infrastructure we added last week (see https://github.com/camunda/camunda-bpm-platform/blob/master/engine/src/test/java/org/camunda/bpm/engine/test/authorization/jobdefinition/SetJobDefinitionPriorityAuthorizationTest.java for an example test case that uses it).

Cheers,
Thorben

[filiphr]

Hi Thorben,

Thanks for pointing out the AuthorizationScenarioInstance. I had a look at it and I added some assertions. To be honest, I am not really fond of the changes that I added.
I was not sure about the order of the missing authorizations. That is why I did the loop with the iterator. If you think that the order of the ExceptionInfo should be the same as the missingAuthorizations then I can use that, it would look much nicer.

You had forgotten to add missingAuthorizations for one of the tests, so I added that as well.

[buildhive]

camunda BPM » camunda-bpm-platform #1540 UNSTABLE
Looks like there's a problem with this pull request
(what's this?)

[ThorbenLindhauer]

Hi Filip,

I now had an in-detail look at the code you provide.

Before merging, I'd like to discuss some things:

• Since you also extended the AuthorizationExceptionDto, could you please add a test case to https://github.com/camunda/camunda-bpm-platform/blob/master/engine-rest/src/test/java/org/camunda/bpm/engine/rest/AbstractExceptionHandlerTest.java that asserts the additional properties?
• I am not so happy with the name AuthorizationExceptionInfo. Perhaps we could use a more meaningful name like MissingAuthorization. What do you think?
• In addition to the last point, it would be great to have some Javadoc in AuthorizationException and AuthorizationExceptionInfo so that users can understand their meaning. For example, it is important to know that the list of missing authorizations is a disjunction, meaning that you need any of the authorizations for the engine to continue execution beyond the point where it failed. A note why the existing methods in AuthorizationException are deprecated now and what to use instead would be also helpful.
• What is the purpose of catching AssertionError in filiphr@4ce8948#diff-6d1c36506e9c29dbbf52a19ea933f88dR116?
• There is already a test util for authorization tests, namely https://github.com/camunda/camunda-bpm-platform/blob/master/engine/src/test/java/org/camunda/bpm/engine/test/authorization/util/AuthorizationTestUtil.java. Perhaps you can move the logic from CamundaAssert there.
• We could move the generation of the exception message from AuthorizationManager#checkAuthorization to AuthorizationException. I think its better located there.
• As for the logic in AuthorizationScenarioInstance: I think we should not make an assumption about the order, because it is not important from the user's perspective. That said, I am not aware of more elegant code, so the current solution is fine with me.

Cheers,
Thorben

[filiphr]

Thanks for the feedback Thorben,

• I will add a test case for the https://github.com/camunda/camunda-bpm-platform/blob/master/engine-rest/src/test/java/org/camunda/bpm/engine/rest/AbstractExceptionHandlerTest.java
• I completely agree with you about the name, I will rename the class to what you suggested, thanks a lot.
• Will add Javadoc and try to be as descriptive as possible.
• This point is the one I said that I don't quite like it. In my opinion we need to make sure that all the missing authorizations are consisted in the list of AuthorizationExceptionInfo(s) from the AuthorizationException. Thus for each AuthorizationExceptionInfo I check if it is equal to the expected parameters, if it is not I catch the thrown error and check the next one. If all AuthorizationExceptionInfo are passed without any hit I do the assertion filiphr@4ce8948#diff-6d1c36506e9c29dbbf52a19ea933f88dR120. If the assertion was ok, I remove that entry from the list, because I think we should have an exact match. If we do not remove it there can be a test where you have the same missing authorizations and pass the test even though it is not correct as I understand. What I could do is to create an assertion method that will do that logic without throwing an AssertionError, or a method that returns the element from the list satisfying the condition. Do you have some proposal about this?
• I will move the CamundaAssert logic to the AuthorizationTestUtil
• I'll move the generation of the message into the constructor as well. I have only one question, I saw that some other type of messages are used as well. How do you suggest to do the generation?
• This last point is related to the AssertionError as well, it is the same code.

[filiphr]

Hey Thorben,

I addressed most of your bullet points. There are multiple commits and each one is for a different bullet point. I will work on the AuthorizationScenario over the weekend.

Cheers,
Filip

[buildhive]

camunda BPM » camunda-bpm-platform #1554 SUCCESS
This pull request looks good
(what's this?)

[ThorbenLindhauer]

Hi Filip,

Thanks for your quick response and changes to the pull request.

Also thank you for the clarification on the logic in AuthorizationScenarioInstance, I get it now :)
Here's another idea how to solve it: perhaps we can employ hamcrest matchers for matching all elements of a list, something along the lines of:

List<MissingAuthorizationMatcher> authorizationMatchers = asMatchers(missingAuthorizations);
Assert.assertThat(infos, Matchers.containsInAnyOrder(authorizationMatchers));

To implement asMatchers(missingAuthorizations), you would have to write a Hamcrest matcher for MissingAuthorization that can be instantiated based on an Authorization object.
That approach would save us the nested loop and the matching logic.

Regarding your question on exception messages: What "other type of messages" do you mean exactly? If you mean the message in the constructor public AuthorizationException(String userId, AuthorizationExceptionInfo exceptionInfo), I think it can reuse the logic you added in your last commit.

Cheers,
Thorben

[buildhive]

camunda BPM » camunda-bpm-platform #1557 FAILURE
Looks like there's a problem with this pull request
(what's this?)

[filiphr]

Hey Thorben,

Thanks for the tip about Hamcrest. I did it and now it is much better.

As for the other type of messages I meant about the public AuthorizationException(String message) constructor. I extracted some of the message generation to the method.

Cheers,
Filip

[buildhive]

camunda BPM » camunda-bpm-platform #1558 FAILURE
Looks like there's a problem with this pull request
(what's this?)

[ThorbenLindhauer]

Hi Filip,

I merged your pull request with commit 08d7c73

I made some smaller cosmetic and naming changes on top of your work. Most importantly, I removed the MissingAuthorization builder. We should not have such a builder in a public API package anyway (since we have to maintain it then as part of the public API) and it appears almost overkill for a class with three properties.

Thank you very much for your contribution!

Cheers,
Thorben

[filiphr]

Hey Thorben,

Thanks for accepting it. No problem for the builder. I will keep it in mind not to use builders for small classes in the future.

Cheers,
Filip