New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

OF-1333: Add OpenFire plugin to authenticate users against a token generated by TikiWiki CMS #774

Merged
merged 1 commit into from May 18, 2017

Conversation

Projects
None yet
5 participants
@fabiomontefuscolo
Copy link
Contributor

fabiomontefuscolo commented Apr 15, 2017

Hi,

We integrated ConverseJS XMPP client to TikiWiki CMS and our preferred XMPP server is OpenFire. So, with help of Guus, we made a plugin to transparently authenticate users in OpenFire using a token generated by a Tiki instance. It would be nice to have our plugin bundled with OpenFire.

Thanks in advance!

<description>Allows users to authenticate with a Tiki token.</description>
<author>Tiki Wiki CMS GroupWare</author>
<version>0.2</version>
<date>21/03/2017</date>

This comment has been minimized.

@akrherz

akrherz Apr 16, 2017

Member

Sadly, we use US date style, please adjust to mm/dd/YYYY , sorry

@guusdk

This comment has been minimized.

Copy link
Member

guusdk commented Apr 19, 2017

I have worked with Fabio to construct this plugin. It adds a proprietary SASL mechanism that authenticates a user, using a bearer token, against a Tiki-provided web API.

@fabiomontefuscolo apart from the minor change that @akrherz mentioned, you might want to add some Javadoc comments and copyright notices?

@Alameyo

This comment has been minimized.

Copy link
Member

Alameyo commented May 8, 2017

I would suggest to add Javadoc for methods that doesn't have it yet and I don't see other issues.

@GregDThomas

This comment has been minimized.

Copy link
Contributor

GregDThomas commented May 9, 2017

It looks to me that TikiTokenSaslServerFactory is based on SaslServerFactoryImpl - which is fine, except that I think it introduces additional unnecessary complications, as the latter supports multiple SaslServer's, the former only one. Could/should createSaslServer() and getMechanismNames() be simplified to the following, which IMHO simpler to work out what's happening ...

    public SaslServer createSaslServer(final String mechanism, final String protocol, final String serverName, final Map<String, ?> props, final CallbackHandler cbh) throws SaslException {
        final boolean plainTextDisallowed = props.containsKey( Sasl.POLICY_NOPLAINTEXT ) && Boolean.parseBoolean( (String) props.get( Sasl.POLICY_NOPLAINTEXT ) );
        if( !plainTextDisallowed && TikiTokenSaslServer.MECHANISM_NAME.equals(mechanism) ) {
            return new TikiTokenSaslServer();
        } else {
            Log.info("Unable to instantiate {} Sasl Server; plainTextDisallowed={}", mechanism, plainTextDisallowed);
            return null;
        }
    }

    public String[] getMechanismNames(final Map<String, ?> props) {
        return new String[] { TikiTokenSaslServer.MECHANISM_NAME };
    }
@guusdk

This comment has been minimized.

Copy link
Member

guusdk commented May 15, 2017

Although I'm obviously biased (as I wrote most of it), I feel that the current code layout has some benefits. Granted, none of these are major benefits, and there's a good deal of style preference involved:

  • It will allow for additional implementations to be added.
  • It separates concerns.
  • It's in-line with the other implementations, making it somewhat more straight-forward to maintain.

A refactoring will make the code different, but I'm not sure if it'll be better (maintainable, performant, etc). For my taste, the downside of being somewhat convoluted does not warrant a refactoring, with the added nuisance of retesting the integration et al - but that's me :)

@guusdk guusdk changed the title Add OpenFire plugin to authenticate users against a token generated by TikiWiki CMS OF-1333: Add OpenFire plugin to authenticate users against a token generated by TikiWiki CMS May 18, 2017

@guusdk guusdk merged commit bbd0566 into igniterealtime:master May 18, 2017

3 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
lgtm analysis: Java No alert changes
Details
lgtm analysis: JavaScript No alert changes
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment