You are viewing a plain text version of this content. The canonical link for it is here.
Posted to server-dev@james.apache.org by "Noel J. Bergman" <no...@devtech.com> on 2002/10/26 18:25:18 UTC
RE: latest checkin..
> Authentication is done against repository for
> Each NNTP Comamnd in a User Session.
> It should only be done once per user session.
Are you saying that the isAuthenticated() method should short-circuit if it
has already authenticated? Something like:
private boolean isAuthenticated() {
if (alreadyAuthenticated) {
return true;
} else {
...
}
--- Noel
--
To unsubscribe, e-mail: <ma...@jakarta.apache.org>
For additional commands, e-mail: <ma...@jakarta.apache.org>
Re: latest checkin..
Posted by Harmeet Bedi <ha...@kodemuse.com>.
----- Original Message -----
From: "Noel J. Bergman" <no...@devtech.com>
> Harmeet,
>
> I have reviewed your proposal. Here is my quick take on it, as a proposed
> code change:
>
> - AuthState +1
>
> This just takes existing data from the handler, and encapsulates it as a
> Java Bean.
>
> - AuthService +1
>
> This simply takes two methods from the handler, and separates them as an
> interface.
>
> - AuthServiceImpl conditional
>
> You have eliminated the stateful problems, but it remains to be seen what
> you do with this class. As I understand it, an instance of AuthService
> would be passed to the NNTPHandler through its service specific
> configuration object.
>
> Apparently, Peter objects to exposing AuthService to other James
components,
> so if this is a Block in james-assembly.xml, he has stated that he intends
> to veto it.
> I'm curious to see what other folks have to say. However, if
> you hide the AuthService inside NNTPServer, e.g.,
>
> NNTPServer() { ... authService = new AuthServiceImpl(); ... }
> compose(...) { ... authService.compose(...); ... }
> configure(...) { ... authService.configure(...); ... }
> enableLogging(...) { super(logger); setupLogger(authService); ... }
>
> or equivalent means, then that might address the objections that I have
read
> so far. All of this presumes, of course, that the code works, otherwise
it
> would need to be fixed.
>
I was thinking along similar lines after the last offline conversion,
however wasn't sure if I should do it right away or defer when the email
flow started.
I was also thinking that configuration can be centralized, but the actual
code
can exist as a service. This would give ease of configuration and
pluggability.
> However, before you invest much more into this effort, consider the
> following.
>
>
> As I understand it the primary purpose for the AuthService is that you
want
> to decouple handlers from repositories: (a) basically don't trust the
> repository interface, and (b) you want to allow an LDAP or Radius
> authentication server to be used. If someone deploying James has that
need,
> it can be addressed. In the meantime, you can easily isolate the
> AuthService inside the NNTPServer, allowing NNTPHandler to remain entirely
> unchanged, which is your primary goal.
>
> One reason for limiting the investment of time and energy into AuthService
> is that if JAAS ends up being used in v3, then it is likely that
AuthService
> would disappear again forever, since JAAS has its own means by which
service
> providers are installed.
>
JAAS is possible and a good idea. But JAAS may not be overkill for some.
A simple authentication service that allows folks to plugin authentication
without understanding Avalon User Respositories may be valuable. Implement 2
methods to plugin authentication seems simple to explain to anyone who may
need to do this.
AuthService is also minor change on the current code. It has been there but
stateful and broken so in essence it is a fix more than anything else.
Regd. effort - has already been done. I was trying to write automated
regression tests
to verify things better for now and future.
I prefer that we defer this change and any scheduler changes
till 2.1 is complete. I would like to minimize chances of the kind of
interaction that has gone on recently.
> Your thoughts, Kind Sir?
Hopefully someday I will be as kind and graceful as you. :-)
Harmeet
--
To unsubscribe, e-mail: <ma...@jakarta.apache.org>
For additional commands, e-mail: <ma...@jakarta.apache.org>
RE: latest checkin..
Posted by "Noel J. Bergman" <no...@devtech.com>.
Harmeet,
I have reviewed your proposal. Here is my quick take on it, as a proposed
code change:
- AuthState +1
This just takes existing data from the handler, and encapsulates it as a
Java Bean.
- AuthService +1
This simply takes two methods from the handler, and separates them as an
interface.
- AuthServiceImpl conditional
You have eliminated the stateful problems, but it remains to be seen what
you do with this class. As I understand it, an instance of AuthService
would be passed to the NNTPHandler through its service specific
configuration object.
Apparently, Peter objects to exposing AuthService to other James components,
so if this is a Block in james-assembly.xml, he has stated that he intends
to veto it. I'm curious to see what other folks have to say. However, if
you hide the AuthService inside NNTPServer, e.g.,
NNTPServer() { ... authService = new AuthServiceImpl(); ... }
compose(...) { ... authService.compose(...); ... }
configure(...) { ... authService.configure(...); ... }
enableLogging(...) { super(logger); setupLogger(authService); ... }
or equivalent means, then that might address the objections that I have read
so far. All of this presumes, of course, that the code works, otherwise it
would need to be fixed.
However, before you invest much more into this effort, consider the
following.
As I understand it the primary purpose for the AuthService is that you want
to decouple handlers from repositories: (a) basically don't trust the
repository interface, and (b) you want to allow an LDAP or Radius
authentication server to be used. If someone deploying James has that need,
it can be addressed. In the meantime, you can easily isolate the
AuthService inside the NNTPServer, allowing NNTPHandler to remain entirely
unchanged, which is your primary goal.
One reason for limiting the investment of time and energy into AuthService
is that if JAAS ends up being used in v3, then it is likely that AuthService
would disappear again forever, since JAAS has its own means by which service
providers are installed.
Your thoughts, Kind Sir?
--- Noel
--
To unsubscribe, e-mail: <ma...@jakarta.apache.org>
For additional commands, e-mail: <ma...@jakarta.apache.org>
Re: latest checkin..
Posted by Harmeet Bedi <ha...@kodemuse.com>.
----- Original Message -----
From: "Noel J. Bergman" <no...@devtech.com>
> > > > Sceduler implementation has already been verified. You made a number
> > > > of fixes too in watchdog.
> > > Noel seems to disagree with this statement. He states that there is
at
> > > least one open bug in your scheduler implementation.
> > The only thing I know of is optimization issues not correctness ones.
>
> Harmeet, I have mentioned at least twice this week that there is one
> outstanding issue that prevents your Scheduler from being a drop-in for
the
> Avalon Scheduler. I'll remind you.
>
> The problem is/was that it doesn't handle inserting a time earlier than
the
> head of the list. And since there are other components that might use a
> Scheduler AS a scheduler, e.g., FetchPOP, that bug needs to be fixed.
>
Sure, will send it around tonight or tomorrow.
Got lost in the vast flood of email, code and life.
thanks for the reminder.
> Between Peter and myself, we have already made the changes to permit a
> Scheduler to be used for Watchdog implementation, but you need to finish
> your side of the equation.
cool,
Harmeet
--
To unsubscribe, e-mail: <ma...@jakarta.apache.org>
For additional commands, e-mail: <ma...@jakarta.apache.org>
RE: latest checkin..
Posted by "Noel J. Bergman" <no...@devtech.com>.
> > > Sceduler implementation has already been verified. You made a number
> > > of fixes too in watchdog.
> > Noel seems to disagree with this statement. He states that there is at
> > least one open bug in your scheduler implementation.
> The only thing I know of is optimization issues not correctness ones.
Harmeet, I have mentioned at least twice this week that there is one
outstanding issue that prevents your Scheduler from being a drop-in for the
Avalon Scheduler. I'll remind you.
The problem is/was that it doesn't handle inserting a time earlier than the
head of the list. And since there are other components that might use a
Scheduler AS a scheduler, e.g., FetchPOP, that bug needs to be fixed.
Between Peter and myself, we have already made the changes to permit a
Scheduler to be used for Watchdog implementation, but you need to finish
your side of the equation.
--- Noel
--
To unsubscribe, e-mail: <ma...@jakarta.apache.org>
For additional commands, e-mail: <ma...@jakarta.apache.org>
Re: latest checkin..
Posted by Harmeet Bedi <ha...@kodemuse.com>.
----- Original Message -----
From: "Peter M. Goldstein" <pe...@yahoo.com>
>
> Harmeet,
>
> > > It was resolved, as it always is, in a matter of hours
> > > of being reported. Not days. Or months.
> >
> > Agreed, but you broke it, I reported it and you fixed it. I could have
>
> > fixed it too.
>
> Just like you could've fixed the broken AuthService anytime in the last
> nine months. But you didn't. And thus it doesn't count. You see,
> saying you're going to do something doesn't cut any mustard when you've
> got no follow through.
>
> This is in fact just like AuthService. You claimed to be able to write
> a service that would replace it. You never submitted it for
> consideration. It doesn't exist. You never bothered to make it exist.
> It's much easier to not do the work and play the victim.
Alright Peter here is the attached service. I basically decided to let you
do your thing and that was not due to laziness, but just did not want the
conflict.
>
> Test code for test code's sake is useless. It's only when it's part of
> a comprehensive notion of testing - and when those test cases are
> clearly labeled - that the testing is at all worthwhile. See any of
> Noel's emails regarding test code.
It would be good to have some contributions from you in this area since you
have such strong views on this.
As far as I know the testing ideas are in line what has been talked off and
I have been commuicating with everyone interested in this topic to make it
better.
>
>
> > > ii) A superfluous class (CRLFWriter.java) that didn't even address a
>
> > > documented issue in the NNTP code. This issue means that the
> > > CRLFWriter code was wrong.
> >
> > This is no longer there. As soon as I saw something eqivalent I
> > removed it. This was yesterday or a couple of days back.
>
> Exactly. So it wasn't a useful contribution. Of course you didn't
> bother to actually read the code base to investigate this...
No it wasn't. I had read the code before but did not remember. What is your
point ?. I added a class, realized it was redundant and then removed it. So
is that an offence in Apache ?
>
>
> > >
> > > iii) An incorrect scheduler that exhibited almost exactly the same
> > > behavior as the current Scheduler
> > >
> > > iv) Another Scheduler implementation that no one but you seems to
> > > think we need, and that you haven't put the time in to build and
> > > test with Noel.
> >
> > Sceduler implementation has already been verified. You made a number
> > of fixes too in watchdog.
>
> Noel seems to disagree with this statement. He states that there is at
> least one open bug in your scheduler implementation.
The only thing I know of is optimization issues not correctness ones. Did
not want to optimize as I was not sure it would be production code.
> Moreover, as far
> as I can tell from the posts, you've never sent him a .sar for testing.
I usu. zip the the James dist and put it up. Sent Noel and cc'd list a few
times.
> So we have no idea, save for your word (which you also gave for the
> Timer implementation of the scheduler, as I recall), that the thing
> works properly under load. It may. It may not.
Noel's test confirmed that it stays up under load.
> > > A regression indeed. And fixed. In an hour. Not nine months.
> >
> > After I posted the exact snippet that exhibitted the problem. Would
> > have been nicer if you would have once tested it yourself or
> > discovered the snippet rather than adding code and breaking the
> > server.
>
>
> And it would've been nice if you hadn't implemented an architecturally
> unsound AuthService. We all make mistakes.
true.
> The difference is that I
> fix my own. You do not.
You are fixing things that I wrote 9 months back and after I have sent the
exact problem and place where it is broken.
>
> So to sum up, in the entirety of 2002 you haven't contributed a single
> useful line of code or documentation. Yet you wish to block those
> who've been working diligently on the project for months. It all comes
> back to ethics...
>
> >
> > As the NNTP code stood today morning at least, it was much worse than
> > anytime in last 9 months. Auth was broken for 9 months but in the last
>
> > few days everything was broken. I am trying to test and fix.
>
> In a word, this is a lie.
>
> There are zero (that's none) outstanding bugs against the current code
> base. All the issues you reported have been addressed - quickly and
> efficiently. What's your issue?
>
> Name your issue and I'll look at it. But the server works - and was
> tested a great deal more than it ever was before.
It sometimes takes a lot longer to find the issues and narrowed to 5 lines
of code than fix the issue.
>
> Just the sheer unmitigated gall of this response is amazing. We have
> here a committer who abandoned the product for nine months, with a slew
> of open critical bugs against his nominally owned component.
This happens. I did make significant contributions then got busy in other
things. You cannot have a person contribute in open source and fault him for
not contributing at the same pace or being around.
Rest of the email doesn't merit a response, but I do wish you could cool
down.
Harmeet
RE: latest checkin..
Posted by "Peter M. Goldstein" <pe...@yahoo.com>.
Harmeet,
> > It was resolved, as it always is, in a matter of hours
> > of being reported. Not days. Or months.
>
> Agreed, but you broke it, I reported it and you fixed it. I could have
> fixed it too.
Just like you could've fixed the broken AuthService anytime in the last
nine months. But you didn't. And thus it doesn't count. You see,
saying you're going to do something doesn't cut any mustard when you've
got no follow through.
This is in fact just like AuthService. You claimed to be able to write
a service that would replace it. You never submitted it for
consideration. It doesn't exist. You never bothered to make it exist.
It's much easier to not do the work and play the victim.
You simply ignored all the posed technical points and rather than trying
to address them, you went off and sulked. And then you took out your
passive-aggressive tendencies on Noel's committer vote. Lovely.
>
> > Perhaps this is the core of the issue. You seem to think that
> > you're work contributed prior to this release allows you dictatorial
> > control over the code.
>
> I have reported the issue and exact code where things were broken. If
> I felt what you are saying, I woundn't have been sending code
> problematic snippets
> to the list.
>
>
> > i) Test code with an unmarked protocol script in violation of the
> > RFC
>
> Some test code is better than none.
> If there is a test script that voilates RFC why don't you suggest a
> patch or add tests.
Test code for test code's sake is useless. It's only when it's part of
a comprehensive notion of testing - and when those test cases are
clearly labeled - that the testing is at all worthwhile. See any of
Noel's emails regarding test code.
> > ii) A superfluous class (CRLFWriter.java) that didn't even address a
> > documented issue in the NNTP code. This issue means that the
> > CRLFWriter code was wrong.
>
> This is no longer there. As soon as I saw something eqivalent I
> removed it. This was yesterday or a couple of days back.
Exactly. So it wasn't a useful contribution. Of course you didn't
bother to actually read the code base to investigate this...
> >
> > iii) An incorrect scheduler that exhibited almost exactly the same
> > behavior as the current Scheduler
> >
> > iv) Another Scheduler implementation that no one but you seems to
> > think we need, and that you haven't put the time in to build and
> > test with Noel.
>
> Sceduler implementation has already been verified. You made a number
> of fixes too in watchdog.
Noel seems to disagree with this statement. He states that there is at
least one open bug in your scheduler implementation. Moreover, as far
as I can tell from the posts, you've never sent him a .sar for testing.
So we have no idea, save for your word (which you also gave for the
Timer implementation of the scheduler, as I recall), that the thing
works properly under load. It may. It may not. But you haven't put in
the effort to test and possibly fix. As far as the Watchdog goes, I
have. It works and has been demonstrated to work by other people.
> > A regression indeed. And fixed. In an hour. Not nine months.
>
> After I posted the exact snippet that exhibitted the problem. Would
> have been nicer if you would have once tested it yourself or
> discovered the snippet rather than adding code and breaking the
> server.
And it would've been nice if you hadn't implemented an architecturally
unsound AuthService. We all make mistakes. The difference is that I
fix my own. You do not.
So to sum up, in the entirety of 2002 you haven't contributed a single
useful line of code or documentation. Yet you wish to block those
who've been working diligently on the project for months. It all comes
back to ethics...
>
> As the NNTP code stood today morning at least, it was much worse than
> anytime in last 9 months. Auth was broken for 9 months but in the last
> few days everything was broken. I am trying to test and fix.
In a word, this is a lie.
There are zero (that's none) outstanding bugs against the current code
base. All the issues you reported have been addressed - quickly and
efficiently. What's your issue?
Name your issue and I'll look at it. But the server works - and was
tested a great deal more than it ever was before.
Just the sheer unmitigated gall of this response is amazing. We have
here a committer who abandoned the product for nine months, with a slew
of open critical bugs against his nominally owned component.
For god's sake, one of them was a typo in a string. It took a grand
total of 20 seconds for me to find it.
And yet it's like every nugget of code he wrote came down from the
mountain with Moses. The attitude just leaves me speechless.
And we once again come back to the same point - if you don't like the
changes in James Harmeet, use an older version. You haven't contributed
one whit to this or to the last version (2.0a3), so I don't see what
you're complaining about.
--Peter
--
To unsubscribe, e-mail: <ma...@jakarta.apache.org>
For additional commands, e-mail: <ma...@jakarta.apache.org>
Re: latest checkin..
Posted by Harmeet Bedi <ha...@kodemuse.com>.
----- Original Message -----
From: "Peter M. Goldstein" <pe...@yahoo.com>
> A NPE that only occurred in a fresh server, not in a test server.
> Largely because the previous NNTP code mixed up the directory lookup and
> validation code.
NPE and server not starting was not a problem in earlier code.
> It was resolved, as it always is, in a matter of hours
> of being reported. Not days. Or months.
Agreed, but you broke it, I reported it and you fixed it. I could have fixed
it too.
> Perhaps this is the core of the issue. You seem to think that you're
> work contributed prior to this release allows you dictatorial control
> over the code.
I have reported the issue and exact code where things were broken. If I felt
what you are saying, I woundn't have been sending code problematic snippets
to the list.
> i) Test code with an unmarked protocol script in violation of the RFC
Some test code is better than none.
If there is a test script that voilates RFC why don't you suggest a patch or
add tests.
>
> ii) A superfluous class (CRLFWriter.java) that didn't even address a
> documented issue in the NNTP code. This issue means that the CRLFWriter
> code was wrong.
This is no longer there. As soon as I saw something eqivalent I removed it.
This was yesterday or a couple of days back.
>
> iii) An incorrect scheduler that exhibited almost exactly the same
> behavior as the current Scheduler
>
> iv) Another Scheduler implementation that no one but you seems to think
> we need, and that you haven't put the time in to build and test with
> Noel.
Sceduler implementation has already been verified. You made a number of
fixes too in watchdog.
> A regression indeed. And fixed. In an hour. Not nine months.
After I posted the exact snippet that exhibitted the problem. Would have
been nicer if you would have once tested it yourself or discovered the
snippet rather than adding code and breaking the server.
As the NNTP code stood today morning at least, it was much worse than
anytime in last 9 months. Auth was broken for 9 months but in the last few
days everything was broken. I am trying to test and fix.
Harmeet
--
To unsubscribe, e-mail: <ma...@jakarta.apache.org>
For additional commands, e-mail: <ma...@jakarta.apache.org>
RE: latest checkin..
Posted by "Peter M. Goldstein" <pe...@yahoo.com>.
Harmeet,
> > It was run a great deal more than once. Of course in my config, the
var
> > is deliberately kept at root. So I wouldn't see this issue.
>
> The last refactoring had caused a NullPointerException and Sever not
> starting.
A NPE that only occurred in a fresh server, not in a test server.
Largely because the previous NNTP code mixed up the directory lookup and
validation code. It was resolved, as it always is, in a matter of hours
of being reported. Not days. Or months.
> MODE READER is the first command sent by an NNTP Client.
> As you can probabaly see it is broken...
>
> Your intent is good and changes knowledgeable, but neither justifies
> removing work done by others(effective veto over other folks work) nor
> does
> it justify breaking things. At least have a test or two. I could have
> checked in the auth fix a few hours back but I am holding off till I
have
> a
> regression test...
What work?
If you want to keep your current NNTP service, you're welcome to use an
older version James. I contributed absolutely nothing prior to James
2.0a3, so you're welcome to use that. It's wrong, but you haven't
contributed any changes to fix it, so I don't see how you have any
problem with it.
Perhaps this is the core of the issue. You seem to think that you're
work contributed prior to this release allows you dictatorial control
over the code.
These are the hard facts. James has almost no one using NNTP because it
simply didn't work. Check the traffic on james-user. The only mention
of NNTP in the last three months was a user rejecting the implementation
as unsuitable for production. No one was maintaining it. There was a
typo bug in the SSL connection that had gone unattended for nine months.
There was the AuthService bug, an architectural problem, that lay
dormant for the same nine months. Your return coincided almost exactly
with actual repairs to the NNTP server to bring it up to a production
quality server (or close to it).
Since your return you have contributed:
i) Test code with an unmarked protocol script in violation of the RFC
ii) A superfluous class (CRLFWriter.java) that didn't even address a
documented issue in the NNTP code. This issue means that the CRLFWriter
code was wrong.
iii) An incorrect scheduler that exhibited almost exactly the same
behavior as the current Scheduler
iv) Another Scheduler implementation that no one but you seems to think
we need, and that you haven't put the time in to build and test with
Noel.
So whose work am I wasting? Certainly not yours.
> Here is the MODE READER implementation. Argument as sent by outlook
> express
> is null.
> yielding
> S: 501 Syntax error - unexpected parameter
> S: 200 Posting Permitted
>
> private void doMODEREADER(String argument) {
> // 7.2
> if ( argument != null ) {
> writer.println("501 Syntax error - unexpected parameter");
> }
> writer.println(theConfigData.getNNTPRepository().isReadOnly()
> ? "201 Posting Not Permitted" : "200 Posting
> Permitted");
> }
A regression indeed. And fixed. In an hour. Not nine months.
--Peter
--
To unsubscribe, e-mail: <ma...@jakarta.apache.org>
For additional commands, e-mail: <ma...@jakarta.apache.org>
Re: latest checkin..
Posted by Harmeet Bedi <ha...@kodemuse.com>.
----- Original Message -----
From: "Peter M. Goldstein" <pe...@yahoo.com>
> It was run a great deal more than once. Of course in my config, the var
> is deliberately kept at root. So I wouldn't see this issue.
The last refactoring had caused a NullPointerException and Sever not
starting.
MODE READER is the first command sent by an NNTP Client.
As you can probabaly see it is broken...
Your intent is good and changes knowledgeable, but neither justifies
removing work done by others(effective veto over other folks work) nor does
it justify breaking things. At least have a test or two. I could have
checked in the auth fix a few hours back but I am holding off till I have a
regression test...
Here is the MODE READER implementation. Argument as sent by outlook express
is null.
yielding
S: 501 Syntax error - unexpected parameter
S: 200 Posting Permitted
private void doMODEREADER(String argument) {
// 7.2
if ( argument != null ) {
writer.println("501 Syntax error - unexpected parameter");
}
writer.println(theConfigData.getNNTPRepository().isReadOnly()
? "201 Posting Not Permitted" : "200 Posting
Permitted");
}
Harmeet
--
To unsubscribe, e-mail: <ma...@jakarta.apache.org>
For additional commands, e-mail: <ma...@jakarta.apache.org>
RE: latest checkin..
Posted by "Peter M. Goldstein" <pe...@yahoo.com>.
Harmeet,
Auth issue isn't a regression, as discussed in my previous email.
> - NNTP Stores should be under apps, now they are created under root
> directory.
This is correct, but the identification of cause is off.
The previous code was broken - it didn't handle absolute URLs correctly.
So it places file:/// URLs in the apps directory, contrary to every
other URL in the system.
Correcting the default configuration so that the URLs are relative by
default.
> Possible cleanup regressions... Nothing to do with scheduler or
service
> patch.
> Fixing second issue, first one is a bit more problematic and would
have
> been
> caught if the server was started once post cleanup/redesign. Will look
> into
> it and fix soon.
Second issue is fixed, thank you.
It was run a great deal more than once. Of course in my config, the var
is deliberately kept at root. So I wouldn't see this issue.
--Peter
--
To unsubscribe, e-mail: <ma...@jakarta.apache.org>
For additional commands, e-mail: <ma...@jakarta.apache.org>
Re: latest checkin..
Posted by Harmeet Bedi <ha...@kodemuse.com>.
2 more problems.
- NNTP Stores should be under apps, now they are created under root
directory.
- The hook to see input output commands is gone. There does not seem to be
any way to dump output from server to log file.
Possible cleanup regressions... Nothing to do with scheduler or service
patch.
Fixing second issue, first one is a bit more problematic and would have been
caught if the server was started once post cleanup/redesign. Will look into
it and fix soon.
Harmeet
--
To unsubscribe, e-mail: <ma...@jakarta.apache.org>
For additional commands, e-mail: <ma...@jakarta.apache.org>
Re: latest checkin..
Posted by Harmeet Bedi <ha...@kodemuse.com>.
Instead of code like this
theWatchdogFactory = new ThreadPerWatchdogFactory(threadPool,
timeout);
Would it be better to have 'WatchdogFactory' as a Avalon Block ?
To replace the impl would require code changes in the *Server files.
Having a pluggable block would be better and more in line with the component
oriented philosophy in the rest of James code.
Harmeet
--
To unsubscribe, e-mail: <ma...@jakarta.apache.org>
For additional commands, e-mail: <ma...@jakarta.apache.org>
RE: latest checkin..
Posted by "Noel J. Bergman" <no...@devtech.com>.
> It's a basically three line change to change the current code to allow
> re-authentication (rather than returning a 482, the user, pass, and
> isAlreadyAuthenticated values get wiped). I don't really care - so long
> as the other behavior (handling double USER submission, out of order,
> double PASSWORD) is there.
>From the look of Harmeet's initial posting, it is a single object that would
be associated with the handler, so per session. I'm not sure that we really
need another object, but what's one more pair of strings?
--- Noel
--
To unsubscribe, e-mail: <ma...@jakarta.apache.org>
For additional commands, e-mail: <ma...@jakarta.apache.org>
RE: latest checkin..
Posted by "Peter M. Goldstein" <pe...@yahoo.com>.
Noel,
> Is there any additional language that refers to a prefered solution,
or to
> any additional semantics for re-authentication? Without the RFC
> specifying
> a preference, or common practice if the RFC is neutral, I don't
> particularly
> care which optional behavior is manifested in the code, so long as it
> works.
> Do anyone else? Harmeet seems to have a preference.
Nothing documented in the RFC. There might be something somewhere else.
It's a basically three line change to change the current code to allow
re-authentication (rather than returning a 482, the user, pass, and
isAlreadyAuthenticated values get wiped). I don't really care - so long
as the other behavior (handling double USER submission, out of order,
double PASSWORD) is there.
> > This is a MAY, not a MUST, so it is indeed optional.
>
> I generally find that MAY is used to specify minimally required
behavior,
> when alternatives are more complex and not universally necessary.
Yep.
--Peter
--
To unsubscribe, e-mail: <ma...@jakarta.apache.org>
For additional commands, e-mail: <ma...@jakarta.apache.org>
RE: latest checkin..
Posted by "Noel J. Bergman" <no...@devtech.com>.
> The only statement on reauthentication in the RFC is the following:
So the RFC language permits either an a priori 482 return or a
re-authentication.
> I chose to implement the specified behavior,
> because that's what is discussed in the RFC.
Is there any additional language that refers to a prefered solution, or to
any additional semantics for re-authentication? Without the RFC specifying
a preference, or common practice if the RFC is neutral, I don't particularly
care which optional behavior is manifested in the code, so long as it works.
Do anyone else? Harmeet seems to have a preference.
> This is a MAY, not a MUST, so it is indeed optional.
I generally find that MAY is used to specify minimally required behavior,
when alternatives are more complex and not universally necessary.
> Note that this is not a regression.
We know.
--- Noel
--
To unsubscribe, e-mail: <ma...@jakarta.apache.org>
For additional commands, e-mail: <ma...@jakarta.apache.org>
RE: latest checkin..
Posted by "Peter M. Goldstein" <pe...@yahoo.com>.
Noel et al,
> As far as I'm concerned, no one --- not me, not you, not Peter, no one
---
> should be submitting changes to the wire-level-protocol syntax and
> semantics
> without documenting the relevant RFC issues. If there is disagreement
on
> how to interpret the RFC, we can check with other sources. But this
is
> not
> a subjective matter. There may be options that render multiple
choices
> correct, but that also needs to be demonstrated from the RFC.
The only statement on reauthentication in the RFC is the following:
"If a client attempts to reauthenticate, the server may return 482
response
indicating that the new authentication data is rejected by the server."
This is a MAY, not a MUST, so it is indeed optional. It would certainly
be possible to make this configurable. I chose to implement the
specified behavior, because that's what is discussed in the RFC.
Note that this is not a regression. Since the AuthService never worked
- try having two different authenticated users on James 2.0a3 and see
what happens when you do a re-authentication. Bad, bad things happen.
--Peter
--
To unsubscribe, e-mail: <ma...@jakarta.apache.org>
For additional commands, e-mail: <ma...@jakarta.apache.org>
RE: latest checkin..
Posted by "Noel J. Bergman" <no...@devtech.com>.
> Would prefer to have deferred this discussion...
> Here it is:
You were the one who raise the discussion. I didn't ask to see code
(finished or unfinished). I asked to see a state description of the
behavior. I can read the previous code to see what IT manifests the
behavior to be, I can read your new code to see what IT manifests the
behavior to be, but NEITHER is an RFC driven description of authentication
states and transitions.
This isn't a style issue, a refactoring issue, or some other subjective.
There is an RFC that dictates the correct behavior. You said that the
current code does not model the correct behavior. I'm not debating the
point with you. I'm asking you for an RFC driven description of the proper
behavior. If necessary, I'll read the RFC, myself.
As far as I'm concerned, no one --- not me, not you, not Peter, no one ---
should be submitting changes to the wire-level-protocol syntax and semantics
without documenting the relevant RFC issues. If there is disagreement on
how to interpret the RFC, we can check with other sources. But this is not
a subjective matter. There may be options that render multiple choices
correct, but that also needs to be demonstrated from the RFC.
--- Noel
--
To unsubscribe, e-mail: <ma...@jakarta.apache.org>
For additional commands, e-mail: <ma...@jakarta.apache.org>
Re: latest checkin..
Posted by Harmeet Bedi <ha...@kodemuse.com>.
----- Original Message -----
From: "Noel J. Bergman" <no...@devtech.com>
regd. AuthState.
> AuthI'm looking at it now ... I don't see where it does what you say it
does. I
> see two setters and the isAuthenticated() method:
Noel I have not been ready with this yet, but if are keen I'll post. Would
prefer to have deferred this discussion...
Here it is:
/**
* Provides Authentication Credentials.
*
* Helps manage the Protocol Authentication state, resets
* authenticated flag as need be.
*
* Helps avoid unnecessary re authentication.
*
*/
public class AuthState
{
private String userID;
private String password;
private boolean authenticated;
// ---- methods invoked by authentication service -----
/**
* @return value of authenticated flag.
*/
public boolean isAuthenticated() {
return authenticated;
}
/**
* Get the value of userID.
* @return value of userID.
*/
public String getUserID() {
return userID;
}
/**
* Get the value of password.
* @return value of password.
*/
public String getPassword() {
return password;
}
/**
* @param v Value to assign to indicate authentication done or not.
*/
void setAuthenticated(boolean v) {
this.authenticated = v;
}
/**
* Set the value of userID. reset authentication state
* @param v Value to assign to userID.
*/
void setUserID(String v) {
reset();
this.userID = v;
}
/**
* Set the value of password. reset authentication flag
* @param v Value to assign to password.
*/
void setPassword(String v) {
this.authenticated = false;
this.password = v;
}
/** reset to initial state */
void reset() {
userID = password = null;
authenticated = false;
}
}
--
To unsubscribe, e-mail: <ma...@jakarta.apache.org>
For additional commands, e-mail: <ma...@jakarta.apache.org>
RE: latest checkin..
Posted by "Noel J. Bergman" <no...@devtech.com>.
> Had the AuthState object
I'm looking at it now ... I don't see where it does what you say it does. I
see two setters and the isAuthenticated() method:
public boolean isAuthenticated() {
if ( requiredAuth ) {
if (userSet && passwordSet ) {
return repo.test(user,password);
} else {
return false;
}
} else {
return true;
}
}
Once the userSet or passwordSet are set it always checks. They are never
cleared. Therefore if requiredAuth is ever set, and once the user and
password are set, it always checks the repository. Which is precisely the
complaint you initially raised this morning.
It appears that there is some question about WHAT the behavior should be,
not how to do it. So, PLEASE submit the state machine you believe describes
the behavior. We ought to resolve this issue at that level first, then make
any necessary code changes.
--- Noel
--
To unsubscribe, e-mail: <ma...@jakarta.apache.org>
For additional commands, e-mail: <ma...@jakarta.apache.org>
Re: latest checkin..
Posted by Harmeet Bedi <ha...@kodemuse.com>.
----- Original Message -----
From: "Noel J. Bergman" <no...@devtech.com>
> Why don't you start by posting what you think the authentication state
> machine looks like?
I am almost there, need to do a bit more testing...
Had the AuthState object before AuthService, and then I started refactoring
James to have multiple service(from one single James specific service) and
probably got too excited and introduced AuthService bug.
http://cvs.apache.org/viewcvs.cgi/jakarta-james/src/java/org/apache/james/nn
tpserver/Attic/AuthState.java
I won't be exactly like this, but something similar. Should be in better
position to send out later today.
Harmeet
--
To unsubscribe, e-mail: <ma...@jakarta.apache.org>
For additional commands, e-mail: <ma...@jakarta.apache.org>
RE: latest checkin..
Posted by "Noel J. Bergman" <no...@devtech.com>.
> > > Authentication is done against repository for
> > > Each NNTP Comamnd in a User Session.
> > > It should only be done once per user session.
> > Are you saying that the isAuthenticated() method
> > should short-circuit if it has already authenticated?
> if the authentication credentials are passed in again
> the alreadyAuthenticated state needs to be cleared.
Why don't you start by posting what you think the authentication state
machine looks like?
--- Noel
--
To unsubscribe, e-mail: <ma...@jakarta.apache.org>
For additional commands, e-mail: <ma...@jakarta.apache.org>
RE: latest checkin..
Posted by "Peter M. Goldstein" <pe...@yahoo.com>.
Harmeet,
> One flaw is that if the authentication credentials are passed in again
the
> alreadyAuthenticated state needs to be cleared.
>
>
Actually this description of the problem is not necessarily correct.
>From section 3.1.1 of RFC 2980, which discusses the AUTHINFO command,
specifies a set of as of yet unimplemented behavior (rejecting out of
order authentication state, possibly rejecting reauthentication). I'm
putting in the requisite return codes. Missed them because I was
focusing on the original NNTP RFC.
--Peter
--
To unsubscribe, e-mail: <ma...@jakarta.apache.org>
For additional commands, e-mail: <ma...@jakarta.apache.org>
Re: latest checkin..
Posted by Harmeet Bedi <ha...@kodemuse.com>.
----- Original Message -----
From: "Noel J. Bergman" <no...@devtech.com>
> > Authentication is done against repository for
> > Each NNTP Comamnd in a User Session.
> > It should only be done once per user session.
>
> Are you saying that the isAuthenticated() method should short-circuit if
it
> has already authenticated? Something like:
>
> private boolean isAuthenticated() {
> if (alreadyAuthenticated) {
> return true;
> } else {
> ...
> }
>
One flaw is that if the authentication credentials are passed in again the
alreadyAuthenticated state needs to be cleared.
I would prefer to separate protocol authentication state management from the
handler. Likely to fix this sometime today.
Harmeet
--
To unsubscribe, e-mail: <ma...@jakarta.apache.org>
For additional commands, e-mail: <ma...@jakarta.apache.org>