add realm option for e.g. Password encryption

[preussal]

add realm option for e.g. Password encryption

PR progress checklist (to be filled in by reviewers)

• Changes to documentation are appropriate (or tick if not required)
• Changes to tests are appropriate (or tick if not required)
• Reviews completed

---

What type of PR is this?

Primary type

• [build] Changes related to the build system
• [chore] Changes to the build process or auxiliary tools and libraries such as documentation generation
• [ci] Changes to the continuous integration configuration
• [feat] A new feature
• [fix] A bug fix
• [perf] A code change that improves performance
• [refactor] A code change that neither fixes a bug nor adds a feature
• [revert] A change used to revert a previous commit
• [style] Changes that do not affect the meaning of the code (white-space, formatting, missing semi-colons, etc.)

Secondary type

• [docs] Documentation changes
• [test] Adding missing or correcting existing tests

Does this PR introduce a BREAKING CHANGE?

No.

Related issues and/or pull requests

Describe the changes you're proposing

Pillar / config required to test the proposed changes

Debug log showing how the proposed changes work

Documentation checklist

• Updated the README (e.g. Available states).
• Updated pillar.example.

Testing checklist

• Included in Kitchen (i.e. under state_top).
• Covered by new/existing tests (e.g. InSpec, Serverspec, etc.).
• Updated the relevant test pillar.

Additional context

[pull-assistant]

Best reviewed: commit by commit

---

Optimal code review plan

      add realm option for e.g. Password encryption

Powered by Pull Assistant. Last update bdb7754 ... bdb7754. Read the comment docs.

[myii]

@preussal Apologies for the delay, we don't have many reviewers at the current time. My work on this formula has been primarily structural and tests, so let's try to get someone who actually uses this formula. Otherwise, please ping me again if you don't get any response and I'll try to find time to review this.

---

@danny-smit, you contributed to this formula not so long ago. Would you be able to look over this PR and give your feedback?

[danny-smit]

I'm not very familiar with the realm settings of tomcat myself, so I'll have to assume that it works as advertised.

However looking over it, i noticed the following:

1. pillar.example: there is some xml embedded in the yaml. I don't know if there are any objections against doing that? However, there already was some xml in the pillars before this change.
2. pillar.example: the 'realm' keywords in pillar.example seem to be indented too much (by 2 spaces). As a result, when using the pillar.example as a starting point, the 'realm' dictionary is interpreted as a connector and the xml is generated incorrectly.
3. pillar.example: additionally, pillar.example contains the dictionary key 'realm' twice at the same level. That doesn't work out-of-the-box and the tests executed by travis seem to fail on this (as it makes use of the pillar.example). It looks like the duplicate keys are meant to show alternatives? Perhaps commenting one out and adding some comments explaining its purpose could fix the problem and while also helping users to use the formula more easily.
4. commitlint: the linter is failing on both the commit message, it probably needs to be rewritten to comply with the symantic-release requirements.
5. yamllint: the yamllinter executed by travis doesn't like the lines longer than 88 characters in pillar.example. The checks seem to fail on that as well.

[myii]

@danny-smit Appreciate the detailed review, nice work!

[preussal]

Yes it looks like XML but is the config for authentication
Pillar

    realm:
      "org.apache.catalina.realm.UserDatabaseRealm":
        name: "org.apache.catalina.realm.UserDatabaseRealm"
        realm_parameters:
          resourceName: "UserDatabase"
        value: '<CredentialHandler className="org.apache.catalina.realm.MessageDigestCredentialHandler" algorithm="md5"/>'

config in nice without unnecessary line breaks

    <Realm className="org.apache.catalina.realm.UserDatabaseRealm" resourceName="UserDatabase">
        <CredentialHandler className="org.apache.catalina.realm.MessageDigestCredentialHandler" algorithm="md5"/>
    </Realm>

Pillar

    realm:
      "org.apache.catalina.realm.LockOutRealm":
        name: "org.apache.catalina.realm.LockOutRealm"
        realm:
          name: "org.apache.catalina.realm.UserDatabaseRealm"
          realm_parameters:
            resourceName: "UserDatabase"
          value: '<CredentialHandler className="org.apache.catalina.realm.MessageDigestCredentialHandler" algorithm="MD5" />'

config in nice without unnecessary line breaks

    <Realm className="org.apache.catalina.realm.LockOutRealm">
         <Realm className="org.apache.catalina.realm.UserDatabaseRealm" resourceName="UserDatabase">
                <CredentialHandler className="org.apache.catalina.realm.MessageDigestCredentialHandler" algorithm="MD5" />
         </Realm>
     </Realm>

the double realm will that is that is that that is no alternative is a sub-realm.
see here https://tomcat.apache.org/tomcat-9.0-doc/realm-howto.html#CombinedRealm

<Realm className="org.apache.catalina.realm.CombinedRealm" >
   <Realm className="org.apache.catalina.realm.UserDatabaseRealm" resourceName="UserDatabase"/>
   <Realm className="org.apache.catalina.realm.DataSourceRealm" dataSourceName="jdbc/authority" userTable="users" userNameCol="user_name" userCredCol="user_pass" userRoleTable="user_roles" roleNameCol="role_name"/>
</Realm>

if I look at it that way, a for loop must even be built in for the sub-realms.
You can currently only use one sub-realm.

[danny-smit]

That is fine, I can live with that.

Are you using the same pillar data as the pillar.example in this pull request? Or did you make changes already?

Currently pillar.example holds:

tomcat:
  connectors:
    realm:
      ...

However tomcat/config.sls is using:

    - text: {{ tomcat.realm }}

That doesn't match and causes the tests to fail. Un-indenting the top-level 'realm' in pillar.example with two spaces should fix that.

I meant to point out that there currently are two top-level realm keys in pillar.example. The sub-realms seemed to be fine so far:

    realm:  # <-- first realm key
      "org.apache.catalina.realm.LockOutRealm":
        name: "org.apache.catalina.realm.LockOutRealm"
        realm:  
          ...

    realm:  # <-- duplicate realm key, resulting in failures rendering the sls
      ...

That's basically a yaml syntax error, which results in failures of rendering the sls.

I'm not sure if I understand you remark about another for-loop for sub-realms, because there seems to be a for-loop handling that already.