Harmeet,
> > ii) The GROUP command wiped the previous selected group, even if the
> > group name passed in didn't correspond to a valid newsgroup.
>
> Could you give a telnet comparison, or the real bug you found.
>
> wildmat was not 100% but 99% based on what is used by nntp clients and
> supported by regex package.
Consider a news server that hosts a single group, alt.james
Examples of bugs found:
Client has an active news session. Types GROUP alt.james . This sets
the internal group pointer to alt.james . Now type GROUP moose.squirrel
. This group doesn't exist, so the group pointer should remain at
alt.james . It doesn't - it gets set to null.
Client has an active news session. Types LIST NEWSGROUPS. This gets
parsed by the handler as LIST, because when going through the "if else"
chain the second token ("NEWSGROUPS") gets consumed during the check as
to whether it matches LIST EXTENSIONS. The key problem with the LIST
commands can be found here:
else if ( command.equals("LIST") && tokens.hasMoreTokens() &&
tokens.nextToken().toUpperCase(Locale.US).equals("EXTENSIONS") )
doLISTEXTENSIONS();
else if ( command.equals("LIST") && tokens.hasMoreTokens() &&
tokens.nextToken().toUpperCase(Locale.US).equals("OVERVIEW.FMT") )
doLISTOVERVIEWFMT();
...
else if (command.equals("LIST"))
doLIST(tokens);
Note that each call to tokens.nextToken() consumes a token, advancing
the StringTokenizer.
It doesn't matter how good or correct the wildmat implementation is.
The wildmat string isn't being seen by LIST NEWSGROUPS, because of the
problem above. Honestly, I didn't even look at the wildmat
implementation - I was focused on fixing problems in NNTPHandler.
> > iii) The auth implementation was completely wrong. This fix needs
> > further refactoring, but the whole AuthService architecture was
badly
> > designed. It does not allow per-connection authentication, which
makes
> > it useless for our purposes. These changes leave the AuthService
class
> > in place, but move the authRequired configuration to the NNTP server
> > handler configuration. The AuthService is unused, and should be
removed
> > completely. If flexible, pluggable authentication services are
desired
> > in the future, a new interface and implementation should be used.
>
> AuthService alllowed
> - validation on actual commands, via <isAuthorized>
> - User Pasword state machine encapsulation.
> - plugin authentication mechanism.
> Shouldn't the bug that you found be fixed in AuthService ?
>
> Regarding perconnection check in authservice - AuthService is an
> interface,
> how can it prevent it ?
> Another implemenation can be plugged in if need be used differently in
the
> handler. How does AuthService prevent you from doing what you need ?
You're missing the point. AuthService doesn't allow any of these
things. The bug can't be fixed in AuthService because AuthService is
conceptually incorrect.
Consider the following situation. Bob and Simon both connect to the
NNTP server. Bob is a legitimate user, Simon is not. Bob's news client
authenticates him and he gets access to services. That's all fine and
dandy. Simon connects and attempts to access services for which he's
not authorized. And he gets access to them. Why? Because Bob has
already authenticated and Simon's NNTPHandler grabs the same AuthService
that Bob grabbed. In fact, in the current code, once one person
authenticates everyone will be authenticated. Disastrous.
AuthService should never have been a Phoenix block. An
AuthServiceFactory would be a potentially correct alternative, but
considering the other problems with AuthService (non-standard code, not
unified with other authentication services in James, has an obvious
java.* replacement in JAAS) and our imminent deadlines, I chose to move
the code into the NNTP Handler and make it correct.
As far as the rest,
i) validation on actual commands - that's a trivial feature as
implemented. All the current code does is "return isAuthenticated() ||
command.equals(A) || command.equals(B) || command.equals(C)". This is
supported in my changes. A properly broken out
authentication/authorization mechanism would also support this.
ii) User/Password state machine encapsulation - For the moment, I
sacrificed this. For all the reasons described above, AuthService is
not fixable. Given time constraints and the time necessary for a proper
debate as to how to architect the mechanism, I felt it was more
important to get it working in a simple fashion.
iii) plugin authentication mechanism - with all due respect, this
doesn't exist. A plugin authentication mechanism would need to allow a
variety of authentication credentials. As such it would need to have
some control over the client/server dialogue. AuthService is lock
stepped to user/password and has no control over the client/server
dialogue. Thus it is not a plugin authentication mechanism.
> >
> > iv) Added a number of comments.
>
> Found some of the comments distracting. What is the point in cutting
and
> pasting comments that are in base class in the derived class as well ?
> Wouldn't javadoc take care of this ?
> It would be better to have protocol conformance or implementation
comments
> instead.
For the most part the comments are specific to the particular class.
The few cases where they aren't specific, they are there largely for
completeness. It has also been my experience that developers are more
likely to alter pre-existing Javadoc than add new Javadoc. Finally, the
Jakarta code standards state that all methods should have Javadoc. As
we are a Jakarta project, our code base to be in line with Jakarta
standards. We could certainly use @see tags for some of these inherited
methods, as is done in some of the FetchPOP code, but it happens not to
be my default style.
> Is there a way to avoid malformed commands in any protocol. What can
be
> done
> ? One could throw an exception and stop the connection. Isn't that
what
> was/is happenning. An example would be better.
That's not what's supposed to happen as per the RFC. NNTP, like SMTP,
has full support for bad syntax notification. So the server should be
notifying the client that the syntax of the command was erroneous.
Instead, exceptions are thrown and caught at a higher level, where the
doQUIT method is invoked. Not a well behaved server. See the SMTP
server (which does syntax checking on all incoming commands) for an
example of correct behavior.
--Peter
--
To unsubscribe, e-mail: <mailto:[EMAIL PROTECTED]>
For additional commands, e-mail: <mailto:[EMAIL PROTECTED]>
