james-server-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Harmeet Bedi" <harm...@kodemuse.com>
Subject Re: [PATCH] NNTP Server fixes
Date Wed, 02 Oct 2002 02:46:18 GMT
bugs:
Good stuff about the bugs. Group command was a miss and LIST seems to have
been a really bad one. It was half correct half wrong, but the net effect
was all wrong. The strange thing is that I have tested with Outlook,
Netsacpe and KClient and still it did not show up. Others tested with news
group clients and it did not show either.

authservice:
You are right about the AuthService. Should never have been a service,
however I think it is a useful abstraction. Would it be possible to have an
AuthServiceFactory as a service instead of AuthService. UsersStore etc. are
ideally not a concern of the Handler, but a concern of AuthService. It
should be easy fix AuthService this way and still have the advantages of the
abstraction.

comments:
prefer the older style more. We can define our own coding standard if you
prefer that. Copying a Jakarta coding style if that involves cut paste
comments from base classes seems less than good. @see is the lesser of two
evils if there is no agreement. Let us reduce comments, unless they are
really helpful. A comment like '// see section <...>' is far more helpful to
me than say javadoc style comments on a private variable. Too much chaff.
I prefer 'C: ' and 'S: ' to indicate the client and server side, it seems
more standard and is less verbose.
My preference is that protocol level debugging should not be controlled by
logger debug level. I prefer a system variable for this level of debugging
but no big thing.

malformed messages:
right error messages are being returned in most handlers. If they are not
then that should be fixed. Your patch does not seem to have changed
malformed headers significantly. Did you have another patch that address
this.
Regd: throwing exception the idea was that server need not go crazy checking
all possible bad arguments. It should at some point throw an exception and
terminate connection from an obviously bad client (or test program).


Excellent patches and bad bugs killed.

cheers,
Harmeet

----- Original Message -----
From: "Peter M. Goldstein" <peter_m_goldstein@yahoo.com>
To: "'James Developers List'" <james-dev@jakarta.apache.org>
Sent: Tuesday, October 01, 2002 11:04 AM
Subject: RE: [PATCH] NNTP Server fixes


>
> 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:james-dev-unsubscribe@jakarta.apache.org>
> For additional commands, e-mail:
<mailto:james-dev-help@jakarta.apache.org>


--
To unsubscribe, e-mail:   <mailto:james-dev-unsubscribe@jakarta.apache.org>
For additional commands, e-mail: <mailto:james-dev-help@jakarta.apache.org>


Mime
View raw message