Provide imap auth interface for plugins

[jaudriga]

Addresses #9377 .

Main change: Allows to use the rcmail::login() function for merely logging into IMAP by introducing a new flag $just_connect. This allows applications using the Roundcube Framework to reuse IMAP authentication logic.

Background: It would be beneficial for applications like Roundcube JMAP (see audriga/roundcube-jmap#6 ) or Kolab's FreeBusy to reuse large parts of the existing IMAP Authentcation logic of Roundcube. This is my try at providing an interface for reusing that.

Minor additional change: Adds a function in rcube to connect to IMAP. I actually consider this optional as its only beneficial for FreeBusy for now. This would basically allow FreeBusy to just call rcube::imap_connect() instead of what they are doing right now (from lib/Kolab/FreeBusy/SourceIMAP.php):

// enable proxy authentication
if (!empty($config['proxy_auth'])) {
	$imap->set_options(array('auth_cid' => $config['proxy_auth'], 'auth_pw' => $config['pass']));
}

// authenticate user in IMAP
if (!$imap->connect($host, $config['user'], $config['pass'], $port, $ssl)) {
	Logger::get('imap')->addWarning("Failed to connect to IMAP server: " . $imap->get_error_code(), $config);
	return false;
}

[pabzm]

Besides the one request about the comment I find this well done and would merge it (which I don't decide on my own), thanks a lot for the thoughful and careful way to introduce a new feature!

[pabzm]

@jaudriga After reading the corresponding ticket I'm wondering you want this actually merged or if it's more of a proof of concept to illustrate your broader request?

[jaudriga]

@pabzm

It would be great to have this merged as this is an obvious and straight forward aspect of #9377 to address for me.

I realize that that might not address all aspects of #9377 , however, its an important and good step in the right direction.

Addressing even broader changes would be great, but not strictly necessary in my opinion. I would need a bit more support for those as a bunch of design decisions would need to be taken that I am not comfortable taking on my own.

I would suggest that we focus on this PR for now and potentially discuss about further improvements in #9377 .

[jaudriga]

I rebased on newest master as on of the CI jobs failed: https://github.com/roundcube/roundcubemail/actions/runs/9059543070/job/24905660338

[pabzm]

The CI will be fixed with #9449, please rebase again after that was merged.

[jaudriga]

I rebased again ;)

[pabzm]

@alecpl Any thoughts on this?

[pabzm]

@alecpl Please have a look at this and provide feedback, or let us know that you're fine with us proceeding without your involvement.

[alecpl]

I don't like it as it is.

1. It should be storage_connect not imap_connect. The existing rcmail::storage_connect() would need to be renamed (or moved to rcmail_action).
2. I think this new storage_connect() should contain more code from login(), i.e. lines from 751-809. Possibly also set_user() and set_storage_prop() calls.
3. Then that method could return rcube_user on success.

[jaudriga]

I don't like it as it is.

Thanks @alecpl for taking a look at the PR. I came around to addressing your feedback today.

Your first suggestion was relatively straight-forward to address IMHO.

I intentionally left out returning an rcube_user object in the first iteration of the PR to avoid as much refactoring as possible. However, I agree that its more well-designed to return rcube_user in the new storage_connect.

The refactoring that I now did is moving the error codes from the rcmail into the rcube parent class. I chose this refactoring, because the $user is checked for brute-force prevention directly after querying for the username. This potentially sets the error to ERROR_RATE_LIMIT. Alternative to this refactoring I had the ideas of either throwing a novel Exception within storage_connect to signal ERROR_RATE_LIMIT or duplicating the rcube_user::query() call.

Please take a look if the newest changes are fine for you.

[alecpl]

I think it would be better to not have the $imap argument. We can call get_storage() in here. Then probably $imap_options should be gone too. BTW, you miss $imap_options in the storage_connect() call.

[alecpl]

I think it would be better to never return null from this method. I.e. return error code or object, here it could be ERROR_STORAGE. That would mean we probably need other codes to cover the cases below.

I'm not sure what to do with $just_connect=true and empty $user (but successful connection).

[github-actions]

@pabzm, @alecpl
🛎️ This PR has had no activity in two weeks.

[pabzm]

@jaudriga Are you still interested in this? If so, please have a look at the comments from Alec from August. If not, please close. Thank you!