Added commands for security subsystem

[olukas]

No description provided.

[Ladicek]

Not assigning a milestone just yet, this might take long and I don't want to block any particular release with this.

[Ladicek]

On the first sight, there's quite a lot of standalone builder classes. Typically, builders are embedded in the class of the command they are building. Any reason why not to do that?

Also, see the Logging class, which is a bit more friendly entrypoint for the commands in the logging subsystem -- could be useful here as well.

EDIT: I see that the tests are structured in a slightly more coarse grained way -- one test class (or two, actually) for given command. This is how the commands/builders could be structured as well, as I said above. Could help with review, too.

[olukas]

These standalone builders only extends common builder for adding login module. They serve as facade for AddLoginModule.Builder. Only reason for them is simplifying usage of AddLoginModule command for some commonly used login modules. They can be wrapped to own command classes, but since they serve as facade then these commands will not added any new functionality or something. They will become just an empty wrappers. I am not sure if it will be better to wrap them to that command classes. WDYT?

I have also taken a look on Logging class. It is nice entry point to subsystem. I can add similar entry point to Security subsystem.

[Ladicek]

If the standalone builders just simplify usage of the "generic" builder and don't add any functionality, maybe these builders could just be folded to the Security entrypoint class?

[olukas]

They can be folded to Security entrypoint class. However some inconveniences occur:

1. Security class will include a lot of lines which are not directly related to "orientation" in security subsystem (which is IMHO one of main reason for that class) - but OK, it is probably only aesthetic detail.
2. Adding a new builder will result to modifying Security class instead of simply creating of the new builder class.

If you think these inconveniences are insignificant I will move those builders to Security class.

[Ladicek]

final?

[Ladicek]

Comments are minor, this is a really good work. Does it work with EAP 6 as well?

[Ladicek]

OK, I really like this in the current form. I found two small issues:

The class AddLoginModule should mention the builders in the class javadoc.

The Remove*OnlineTest tests have ugly copy&paste errors. For example, from RemoveLoginModuleOnlineTest:

    @Test
    public void removeOneOfMoreLoginModules() throws Exception {
        client.apply(ADD_LOGIN_MODULE_1);
        assertTrue("The login module should be created", ops.exists(TEST_LOGIN_MODULE1_ADDRESS));
        client.apply(ADD_LOGIN_MODULE_2);
        assertTrue("The login module should be created", ops.exists(TEST_LOGIN_MODULE2_ADDRESS));

        client.apply(new RemoveLoginModule(TEST_SECURITY_DOMAIN_NAME, LOGIN_MODULE1_CODE));
        assertFalse("The security domain should be created", ops.exists(TEST_LOGIN_MODULE1_ADDRESS));
    }

The assert message on the last line should reflect that this is about login modules, not security domains, and about removing, not creating. So: "The login module should be removed".

And things like this are all over the Remove*OnlineTest tests.

But these are pretty small issues and I believe we don't have to have an extra round of review on that. So when you fix it, please also rebase and squash. I'll happily merge.

[Ladicek]

One other thing: the AddLoginModule builders use generics to emulate self types. With the datasources builders, I added a comment The {@code THIS} type parameter is only meant to be used by subclasses. If you're not inheriting from this class, don't use it. -- could you add it to the AddLoginModule.Builder javadoc too?