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

Allow support of Office 365 #584

Merged
merged 26 commits into from Nov 19, 2018

Conversation

Projects
None yet
5 participants
@oxynux
Copy link
Contributor

oxynux commented Oct 19, 2018

Issue: #532

This allow fider user's to set EMAIL_SMTP_AUTH_LOGIN=1 in .env file, for support Office 365 SMTP (smtp.office365.com) and others SMTP server who use "LOGIN Simple Authentication".

Office 365 SMTP use "LOGIN Simple Authentication" defined by this Internet Draft: draft-murchison-sasl-login-00.txt.

Add support for SMTP LOGIN auth
- This commit allow support of Office 365
- Add EMAIL_SMTP_AUTH_LOGIN option to environment configuration file
- Closes #532

EMAIL_SMTP_AUTH_LOGIN can be set to 1 or 0 (default is 0).
@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Oct 19, 2018

Codecov Report

Merging #584 into master will decrease coverage by 0.29%.
The diff coverage is 85.29%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #584      +/-   ##
==========================================
- Coverage   74.35%   74.06%   -0.29%     
==========================================
  Files         103      105       +2     
  Lines        6631     6695      +64     
==========================================
+ Hits         4930     4958      +28     
- Misses       1340     1376      +36     
  Partials      361      361
Impacted Files Coverage Δ
app/middlewares/setup.go 59.29% <ø> (ø) ⬆️
app/pkg/email/smtp/smtp.go 18.46% <100%> (ø) ⬆️
app/pkg/email/smtp/auth.go 29.82% <80.95%> (ø)
app/pkg/email/smtp/auth_login.go 91.67% <91.67%> (ø)
app/pkg/web/context.go 84.38% <0%> (+1.62%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8aa0aad...6b06685. Read the comment docs.

@goenning

This comment has been minimized.

Copy link
Member

goenning commented Oct 19, 2018

That looks great, thank you @oxynux

I have a suggestion if you don't mind. I'd like to avoid the extra environment variable and I believe it's possible.

The gotsmtp.ServerInfo has an auth property that is an array of all supported Auth mechanisms of the SMTP server. We can use that to decide whether we use PlainAuth or LoginAuth. What do you think? We might end up having a third Auth struct that decides which of those other two Auth methods to use.

By avoiding the extra parameter we make it flexible and easier for the end user :)

@oxynux

This comment has been minimized.

Copy link
Contributor

oxynux commented Oct 19, 2018

Great idea @goenning, I didn't think about using gosmtp.ServerInfo auth property to avoid an extra parameter.
I will try to do that this week-end 👍

oxynux and others added some commits Oct 24, 2018

add AgnosticAuth() for choose correct auth method
remove EMAIL_SMTP_AUTH_LOGIN option (now useless)
support SMTP LOGIN authentication (#532)
- This commit allow support of Office 365 (smtp.office365.com)
- Add func AgnosticAuth() for choose correct authentication method

@oxynux oxynux force-pushed the oxynux:smtp-auth-login branch 2 times, most recently from b6c8b39 to 79c21bd Oct 24, 2018

@oxynux

This comment has been minimized.

Copy link
Contributor

oxynux commented Oct 24, 2018

@goenning It should be ok 🙂

@goenning

This comment has been minimized.

Copy link
Member

goenning commented Oct 24, 2018

@oxynux that's great! I didn't know that Go smtp had support for CRAMMD5Auth. So this PR will fix a bug and introduce a feature, great job 👍 ! Looking forward to merge this one.

I have my last two request, hope you don't get angry at me 😃 .

We should remove that duplicate code of initialising the Auth's twice + calling Start(server) and Next(fromServer, more) twice as well.

If we extract the logic to find the correct auth into a new function, like. func (a *agnosticAuth) FindAuth(server *gosmtp.ServerInfo) gosmtp.Auth that can return either an instance of a gosmtp.Auth or nil. You can then use call this function on Start and store the result into agnosticAuth (instead of saving just a string). This instance can then be reused on Next.

We also need to cover AgnosticAuth with simple unit tests. The ones I can think about is:

  1. It should choose the correct Auth method based on the auth string array;
  2. It should return an error if we don't support any of the auth string array;
@oxynux

This comment has been minimized.

Copy link
Contributor

oxynux commented Nov 2, 2018

@goenning Sorry for the response delay.

I agree it's sounds good to remove duplicate code, however I don't really understand how to "extract the logic", maybe when I will put my mind to it I'll understand better 😅.

I'll try to finish this, this weekend.

@goenning

This comment has been minimized.

Copy link
Member

goenning commented Nov 2, 2018

That's ok, don't worry! If you need something I'll be online most of the weekend so just ping me :)

oxynux added some commits Nov 7, 2018

@oxynux

This comment has been minimized.

Copy link
Contributor

oxynux commented Nov 16, 2018

func (a *agnosticAuth) FindAuth(server *gosmtp.ServerInfo) gosmtp.Auth is now implemented 😀
@goenning I'm not familiar with fider unit test suite can you give me an exemple ?

@goenning

This comment has been minimized.

Copy link
Member

goenning commented Nov 16, 2018

Awesome! That's exactly what I was thinking about 😄

As for the test, you can create a auth_test.go file and paste this as an example:

package smtp_test

import (
	gosmtp "net/smtp"
	"testing"

	. "github.com/getfider/fider/app/pkg/assert"
	"github.com/getfider/fider/app/pkg/email/smtp"
)

func TestAgnosticAuth_Login(t *testing.T) {
	RegisterT(t)

	auth := smtp.AgnosticAuth("", "jon", "s3cr3t", "test.fider.io")
	proto, bytes, err := auth.Start(&gosmtp.ServerInfo{
		Auth: []string{"LOGIN"},
	})

	Expect(err).IsNil()
	Expect(proto).Equals("LOGIN")
	Expect(bytes).Equals([]byte("jon"))

	bytes, err = auth.Next([]byte("Username:"), true)
	Expect(err).IsNil()
	Expect(bytes).Equals([]byte("jon"))

	bytes, err = auth.Next([]byte("Password:"), true)
	Expect(err).IsNil()
	Expect(bytes).Equals([]byte("s3cr3t"))
}

func TestAgnosticAuth_NoMatchingAuth(t *testing.T) {
	RegisterT(t)

	...
}

The second test you can start like the first, but set Auth: []string{"FAKE-MD5"}. This is kind of mocking a SMTP server that only supports an auth mechanism that we don't implement. The result should be that smtp.AgnosticAuth returns an error. :)

oxynux added some commits Nov 18, 2018

@oxynux

This comment has been minimized.

Copy link
Contributor

oxynux commented Nov 18, 2018

@goenning It's finally done 🎉
Thanks for the test exemple, implementing TestAgnosticAuth_NoMatchingAuth(t *testing.T) helped me fix Start method of AgnosticAuth.

@goenning goenning merged commit 54e1c15 into getfider:master Nov 19, 2018

5 checks passed

ci/circleci: build Your tests passed on CircleCI!
Details
ci/circleci: push Your tests passed on CircleCI!
Details
ci/circleci: setup Your tests passed on CircleCI!
Details
ci/circleci: test-server Your tests passed on CircleCI!
Details
ci/circleci: test-ui Your tests passed on CircleCI!
Details
@goenning

This comment has been minimized.

Copy link
Member

goenning commented Nov 19, 2018

Thank you @oxynux! It's a great contribution 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment