Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[FEATURE] Support multiple RCPT TO addresses on the same session #120

Closed
4 tasks done
dandare100 opened this issue Sep 23, 2022 · 13 comments · Fixed by #126
Closed
4 tasks done

[FEATURE] Support multiple RCPT TO addresses on the same session #120

dandare100 opened this issue Sep 23, 2022 · 13 comments · Fixed by #126
Assignees
Labels
enhancement New feature or request

Comments

@dandare100
Copy link
Contributor

Please could we add the ability to specify multiple recipients by being able to send the RCPT TO: multiple times with different addresses on the same session.

At the moment the framework simply replaces the last rcpt address with the newly requested on

It should be ok because this is how the rfc defines it : https://datatracker.ietf.org/doc/html/rfc2821#section-4.1.1.3

Perhaps we can change the rcpttoRequest type in the Message struct to a slice and append the to address of every RCPT TO:

I am happy to have a go, I am just checking in with the idea.

@dandare100 dandare100 added the enhancement New feature or request label Sep 23, 2022
@bestwebua
Copy link
Member

@dandare100 Hahaha, I knew this question would be asked someday) Agree! Let's back for this point at the beginning of October 🍻

@bestwebua bestwebua changed the title [FEATURE] Support multiple RCPTTO addresses on the same session [FEATURE] Support multiple RCPT TO addresses on the same session Sep 23, 2022
@dandare100
Copy link
Contributor Author

@dandare100 Hahaha, I knew this question would be asked someday) Agree! Let's back for this point at the beginning of October 🍻

Cool, cheers

@benjamin-rood
Copy link
Collaborator

benjamin-rood commented Nov 10, 2022

👍
This is essential for testing emailer implementations. It should indeed be a string slice in sequential order of RCPT commands.
e.g. if the emailer implementation is sending the email to two recipents "a" and "b" on the "example" domain, then the RcpttoRequest() method on a Message should return:

[]string{"RCPT TO:<a@example.org>", "RCPT TO:<b@example.org>"}

@benjamin-rood
Copy link
Collaborator

@bestwebua I'm happy to give this a go, if you can point me to the bits of the code that would need changing. The current implementation seems to do some "writing" that involves some significant indirection that I would rather not spend the time trying to unravel if you could just explain it to me?

@bestwebua
Copy link
Member

@benjamin-rood Sounds good. Okay, but let's complete race condition fix firsly ;)

@bestwebua
Copy link
Member

bestwebua commented Nov 12, 2022

Mates, @dandare100, @benjamin-rood! Could somebody provide an example of multiple RCPT TO command example? I expect that it should be somesing like:

RCPT TO:<user1@example.com,user@example.com>
RCPT TO:user1@example.com,user@example.com

And it will produce 2 different messages with different RCPT TO contexts.

@dandare100
Copy link
Contributor Author

dandare100 commented Nov 12, 2022

It seems like the client sends the first one and then each subsequent one after receiving a 250 Received from the server.

image

For my quick use on my project I just did this

// Writes handled RCPTTO result to session, message. Always returns true
func (handler *handlerRcptto) writeResult(isSuccessful bool, request, response string) bool {
	session, message := handler.session, handler.message
	if !isSuccessful {
		session.addError(errors.New(response))
	}
	message.rcpttoRequests = append(message.rcpttoRequests, request)
	message.rcpttoResponse, message.rcptto = response, isSuccessful
	session.writeResponse(response, handler.configuration.responseDelayRcptto)
	return true
}

and

// Structure for storing the result of SMTP client-server interaction. Context-included
// commands should be represented as request/response structure fields
type Message struct {
	heloRequest, heloResponse                         string
	mailfromRequest, mailfromResponse                 string
	rcpttoResponse                                    string
	rcpttoRequests                                    []string
	dataRequest, dataResponse                         string
	msgRequest, msgResponse                           string
	rsetRequest, rsetResponse                         string
	helo, mailfrom, rcptto, data, msg, rset, quitSent bool
}

I had no need for separate messages in my case. I understand your requirement to have multiple messages :-)

When adding the rcpt's, something to be careful of is the clearMessage call below in handler_rcptto.go.

I commented it out because my needs were still met.

I suspect that clear to have something to do with the "-multipleMessageReceiving" option but I didn't look into it further.

// Main RCPTTO handler runner
func (handler *handlerRcptto) run(request string) {
	handler.clearError()
	//I scheme it's because of the mutlimessage disting in the config
	//handler.clearMessage()

	if handler.isInvalidRequest(request) {
		return
	}

	handler.writeResult(true, request, handler.configuration.msgRcpttoReceived)
}

@bestwebua
Copy link
Member

bestwebua commented Nov 12, 2022

@dandare100 Thanks for your knowledge sharing, Mark! I'll play with some stdlib SMTP clients from ruby, python and go to make sure that it is exactly expected multiple rcpt to behaviour. Have you screened logs from mikrotik built-in smtp client, am I right?

multipleMessageReceiving it's about ability to send multiple messages during one session. And entry point for this case is multipleMessageReceiving enabled flag and RSET command, please look on this test example:

go-smtp-mock/server_test.go

Lines 189 to 267 in 03e6fbe

t.Run("when complex successful session, multiple message receiving scenario enabled", func(t *testing.T) {
session, configuration := &sessionMock{}, createConfiguration()
configuration.multipleMessageReceiving = true
server := newServer(configuration)
session.On("writeResponse", configuration.msgGreeting, defaultSessionResponseDelay).Once().Return(nil)
session.On("setTimeout", defaultSessionTimeout).Once().Return(nil)
session.On("readRequest").Once().Return("helo example.com", nil)
session.On("clearError").Once().Return(nil)
session.On("writeResponse", configuration.msgHeloReceived, configuration.responseDelayHelo).Once().Return(nil)
session.On("isErrorFound").Once().Return(false)
session.On("setTimeout", defaultSessionTimeout).Once().Return(nil)
session.On("readRequest").Once().Return("ehlo example.com", nil)
session.On("clearError").Once().Return(nil)
session.On("writeResponse", configuration.msgHeloReceived, configuration.responseDelayHelo).Once().Return(nil)
session.On("isErrorFound").Once().Return(false)
session.On("setTimeout", defaultSessionTimeout).Once().Return(nil)
session.On("readRequest").Once().Return("mail from: receiver@example.com", nil)
session.On("clearError").Once().Return(nil)
session.On("writeResponse", configuration.msgMailfromReceived, configuration.responseDelayMailfrom).Once().Return(nil)
session.On("isErrorFound").Once().Return(false)
session.On("setTimeout", defaultSessionTimeout).Once().Return(nil)
session.On("readRequest").Once().Return("rcpt to: sender1@example.com", nil)
session.On("clearError").Once().Return(nil)
session.On("writeResponse", configuration.msgRcpttoReceived, configuration.responseDelayRcptto).Once().Return(nil)
session.On("isErrorFound").Once().Return(false)
session.On("setTimeout", defaultSessionTimeout).Once().Return(nil)
session.On("readRequest").Once().Return("data", nil)
session.On("clearError").Once().Return(nil)
session.On("writeResponse", configuration.msgDataReceived, configuration.responseDelayData).Once().Return(nil)
session.On("isErrorFound").Once().Return(false)
session.On("readBytes").Once().Return([]uint8(".some message"), nil)
session.On("readBytes").Once().Return([]uint8(".\r\n"), nil)
session.On("writeResponse", configuration.msgMsgReceived, configuration.responseDelayMessage).Once().Return(nil)
session.On("setTimeout", defaultSessionTimeout).Once().Return(nil)
session.On("readRequest").Once().Return("rset", nil)
session.On("clearError").Once().Return(nil)
session.On("writeResponse", configuration.msgRsetReceived, configuration.responseDelayRset).Once().Return(nil)
session.On("isErrorFound").Once().Return(false)
session.On("setTimeout", defaultSessionTimeout).Once().Return(nil)
session.On("readRequest").Once().Return("mail from: receiver@example.com", nil)
session.On("clearError").Once().Return(nil)
session.On("writeResponse", configuration.msgMailfromReceived, configuration.responseDelayMailfrom).Once().Return(nil)
session.On("isErrorFound").Once().Return(false)
session.On("setTimeout", defaultSessionTimeout).Once().Return(nil)
session.On("readRequest").Once().Return("rcpt to: sender1@example.com", nil)
session.On("clearError").Once().Return(nil)
session.On("writeResponse", configuration.msgRcpttoReceived, configuration.responseDelayRcptto).Once().Return(nil)
session.On("isErrorFound").Once().Return(false)
session.On("setTimeout", defaultSessionTimeout).Once().Return(nil)
session.On("readRequest").Once().Return("data", nil)
session.On("clearError").Once().Return(nil)
session.On("writeResponse", configuration.msgDataReceived, configuration.responseDelayData).Once().Return(nil)
session.On("isErrorFound").Once().Return(false)
session.On("readBytes").Once().Return([]uint8(".some message"), nil)
session.On("readBytes").Once().Return([]uint8(".\r\n"), nil)
session.On("writeResponse", configuration.msgMsgReceived, configuration.responseDelayMessage).Once().Return(nil)
session.On("setTimeout", defaultSessionTimeout).Once().Return(nil)
session.On("readRequest").Once().Return("quit", nil)
session.On("writeResponse", configuration.msgQuitCmd, configuration.responseDelayQuit).Once().Return(nil)
session.On("isErrorFound").Once().Return(false)
session.On("finish").Once().Return(nil)
server.handleSession(session)
assert.Equal(t, 2, len(server.Messages()))
})

session.On("readRequest").Once().Return("rset", nil)

What you think about next steps of implementation?:

  1. Add flag multipleRcptto
  2. Change Message structure to be able to save slice of strings:
type Message struct {
	heloRequest, heloResponse                   			string
	mailfromRequest, mailfromResponse           			string
	rcpttoRequestResponse                       			[][]string // new fileld with [["request", "response"]]
	dataRequest, dataResponse                   			string
	msgRequest, msgResponse                     			string
	rsetRequest, rsetResponse                         		string
	helo, mailfrom, rcptto, data, msg, rset, quitSent		bool
}
  1. Leave current behaviour for case when multipleRcptto is false
  2. When multipleRcptto is true we need to record all RCPT TO request/response pairs until receive succesful response and next smtp command has been passed. Also message clearing flow should be used for case when RCPT TO will passed after DATA and upper or when has been used RSET

P.S.: View for successful send messages we can add later. I believe it should be a slice of "successful mesages" where all multiple messages will be represented as independent messages. For instance, we have 1 single message and 1 multiple message with 2 receivers. Total count of this successful messages should be 3.

@dandare100
Copy link
Contributor Author

dandare100 commented Nov 13, 2022

lol, yes you are correct re Mikrotik.

The below is for a Java client.

image

I like your approach, my only question would be why would you add a switch (multipleRcptto )?
Why not just let it function in multiple mode as a normal/standard way ?

The client can send 1 message when the server is in multiple mode : no problem

@bestwebua
Copy link
Member

@dandare100 Thanks for java client examples ❤️ I think it can be configured behaviour against hardcoded. It will bring more flexibility and more context even from configuration, for example:

smtpmock.New(smtpmock.ConfigurationAttr{
  MultipleRcptto: true,
  MultipleMessageReceiving: true,
  NotRegisteredEmails: []string{"nobody@example.com"},
})

Also we should folowing current 'configuring flow' to save our main feature - 'Configurable multithreaded RFC compatible SMTP server'. Agree? 😃

If it okay I'll prepare to 2.x.x migration and will start developing this feature 👨‍💻

@dandare100
Copy link
Contributor Author

Good points, I agree

@bestwebua bestwebua linked a pull request Nov 13, 2022 that will close this issue
9 tasks
@benjamin-rood
Copy link
Collaborator

image

@bestwebua
Copy link
Member

@dandare100, @benjamin-rood Already in latest (2.0.0) release 🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Development

Successfully merging a pull request may close this issue.

3 participants