JAMES-4205: Fix OIDC mailbox provisioning#3044
Conversation
chibenwa
left a comment
There was a problem hiding this comment.
Good with me.
A 🍏 AI run and it shall be good to go!
We are either authenticated or STARTTLS is active
It will do Rationals: we need to verify STARTTLS command is not altered midayy to batch extra commands in. This costly check is no longer needed as soon as auth or starttls is passed. CF https://nostarttls.secvuln.info/ for rationnals. |
4e6e265 to
17ee7c8
Compare
Okay, I removed all calls of Another question: After a successful authentication, the server tries to provision the mailboxes for the user but ignores potential errors. The authentication is not aborted. Is that the behavior you would like in that case? Before my changes, the server would send a |
Good question I think being lenient prevents not accessing his mail... So current behaviour is likely good.
I think answering yes anyway and doing provisionning in a best effort way is indeed better. |
17ee7c8 to
4e5aa79
Compare
Currently, mailboxes (even INBOX) are only created when logging in using PLAIN or LOGIN.
There are now two methods (authFailure and authSuccess) and all paths that result in successful authentication or an error caused by the user are now handled in those two places. There are still some errors that are caused by James itself which are not handled by those two methods.
4e5aa79 to
310073a
Compare
310073a to
f2ecd05
Compare
Benoit asked in https://issues.apache.org/jira/browse/JAMES-4205 that independent of the authentication mechanism, James should perform provisioning.
To fulfill that requirement, I introduced (or rather repurposed) the method
authSuccessto perform all operations that are necessary after a successful operation.While I was at it, I also introduced
authFailurewhich is called quite often and therefore should reduce the complexity quite a bit.This also changes the semantics a little in some places, mainly because every failure is now consistently logged with AuditTrail and effects the failure count.
I am still running tests but I would also appreciate if somebody more familiar with the code base could validate that I did not accidentally change the semantics in some place other than log messages.
Additionally, I am not sure where
session.stopDetectingCommandInjection();is supposed to be used but I could not find any logic to how it was used before and changed it so that it is consistently called after every processed request.