IAM 978 User invite - send email for identity creation with recovery code and link #403

BarcoMasile · 2024-09-06T15:45:35Z

Description

This PR introduces a new dependency for the low level email operations.
With this PR when a new identity is created through the REST API, an email is sent to the identity email with a link and code to use for the first login to complete the registration resetting the password.

Right now we're using a temporary template I made, but @huwshimi will implement a new one as soon as our UX expert provides one.

Testing

To perform a manual test for this PR you can deploy the Admin UI with its dependencies (you can disable authentication and authorization to make testing smoother) and with the MAIL_HOST, MAIL_PORT, MAIL_USERNAME, MAIL_PASSWORD env variables, and just create an identity.
Suggestion: use maildev for testing, run it as a docker container with

$ docker run -p 1080:1080 -p 1026:1025 maildev/maildev

then go to http://localhost:1080 to visit maildev dashboard. You'll be able to see any email "sent" by admin ui in that dashboard.

internal/mail/service.go

pkg/identities/service.go

nsklikas

Good job overall, I have a couple of comments.

This breaks the skaffold deployment, will this be addressed in another PR?

On top of that I had trouble testing it locally. I was unable to make the maildev server to work. I started the skaffold setup, then spun up a localhost admin-ui and configured it with:

"MAIL_HOST": "0.0.0.0",
"MAIL_PORT": "1025",
"MAIL_FROM_ADDRESS": "a@b.com",

I couldn't use the API via the frontend, because the validation failed as the frontend does not allow you to set a password. I had to use curl to access the API:

curl 'http://localhost:8080/api/v0/identities' -X POST \
  -H 'Accept: application/json, text/plain, */*' \
  -H 'Content-Type: application/x-www-form-urlencoded' \
  -H 'Cookie: <cookie>' \
  -H 'Sec-Fetch-Dest: empty' -H 'Sec-Fetch-Mode: cors' -H 'Sec-Fetch-Site: same-origin' \
  --data-raw '{
  "credentials": {"password": {"config": {"password": "123"}}},
  "schema_id": "default",
  "traits": {
    "email": "a@s.com"
  }
}'

But this resulted in the following panic:

2024/09/09 15:19:39 http: panic serving 127.0.0.1:41704: runtime error: invalid memory address or nil pointer dereference
goroutine 245 [running]:
net/http.(*conn).serve.func1()
	/usr/local/go/src/net/http/server.go:1903 +0x13f
panic({0x27c6360?, 0x3ca4d80?})
	/usr/local/go/src/runtime/panic.go:770 +0x136
github.com/canonical/identity-platform-admin-ui/pkg/identities.(*API).error(0xc0007e4060, 0x0)
	/home/nikos/projects/identity-platform-admin-ui/pkg/identities/handlers.go:259 +0x43
github.com/canonical/identity-platform-admin-ui/pkg/identities.(*API).handleCreate(0xc0007e4060, {0x75a1e1f812f8, 0xc0008eb700}, 0xc00088e5a0)
	/home/nikos/projects/identity-platform-admin-ui/pkg/identities/handlers.go:155 +0x608
net/http.HandlerFunc.ServeHTTP(0xc00022da50, {0x75a1e1f812f8, 0xc0008eb700}, 0xc00088e5a0)
	/usr/local/go/src/net/http/server.go:2171 +0x33
github.com/canonical/identity-platform-admin-ui/internal/validation.(*ValidationRegistry).ValidationMiddleware.func1({0x75a1e1f812f8, 0xc0008eb700}, 0xc0007f4a20)
	/home/nikos/projects/identity-platform-admin-ui/internal/validation/registry.go:89 +0xa74
net/http.HandlerFunc.ServeHTTP(0xc00052e380, {0x75a1e1f812f8, 0xc0008eb700}, 0xc0007f4900)
	/usr/local/go/src/net/http/server.go:2171 +0x33
github.com/canonical/identity-platform-admin-ui/internal/authorization.(*Middleware).Authorize.func1.1({0x75a1e1f812f8, 0xc0008eb700}, 0xc0007f4120)
	/home/nikos/projects/identity-platform-admin-ui/internal/authorization/middlewares.go:161 +0x67b
net/http.HandlerFunc.ServeHTTP(0xc00052e3a0, {0x75a1e1f812f8, 0xc0008eb700}, 0xc0007f4120)
	/usr/local/go/src/net/http/server.go:2171 +0x33
github.com/canonical/identity-platform-admin-ui/pkg/authentication.(*Middleware).oAuth2CookieAuthentication.func1({0x75a1e1f812f8, 0xc0008eb700}, 0xc00003f7a0)
	/home/nikos/projects/identity-platform-admin-ui/pkg/authentication/middlewares.go:139 +0x9d3
net/http.HandlerFunc.ServeHTTP(0xc00052e3c0, {0x75a1e1f812f8, 0xc0008eb700}, 0xc00003f7a0)
	/usr/local/go/src/net/http/server.go:2171 +0x33
github.com/canonical/identity-platform-admin-ui/pkg/authentication.(*Middleware).oAuth2BearerAuthentication.func1({0x75a1e1f812f8, 0xc0008eb700}, 0xc00003f680)
	/home/nikos/projects/identity-platform-admin-ui/pkg/authentication/middlewares.go:86 +0x59b
net/http.HandlerFunc.ServeHTTP(0xc00052e3e0, {0x75a1e1f812f8, 0xc0008eb700}, 0xc00003f560)
	/usr/local/go/src/net/http/server.go:2171 +0x33
github.com/go-chi/chi/v5.(*ChainHandler).ServeHTTP(0xc000896600, {0x75a1e1f812f8, 0xc0008eb700}, 0xc00003f560)
	/home/nikos/projects/identity-platform-admin-ui/vendor/github.com/go-chi/chi/v5/chain.go:31 +0x34
github.com/go-chi/chi/v5.(*Mux).routeHTTP(0xc0006f9f20, {0x75a1e1f812f8, 0xc0008eb700}, 0xc00003f560)
	/home/nikos/projects/identity-platform-admin-ui/vendor/github.com/go-chi/chi/v5/mux.go:459 +0x306
net/http.HandlerFunc.ServeHTTP(0xc0008284a0, {0x75a1e1f812f8, 0xc0008eb700}, 0xc00003f560)
	/usr/local/go/src/net/http/server.go:2171 +0x33
github.com/go-chi/chi/v5/middleware.RequestLogger.func1.1({0x75a1e1f812f8, 0xc0008eb680}, 0xc00003ea20)
	/home/nikos/projects/identity-platform-admin-ui/vendor/github.com/go-chi/chi/v5/middleware/logger.go:55 +0x28f
net/http.HandlerFunc.ServeHTTP(0xc000804c30, {0x75a1e1f812f8, 0xc0008eb680}, 0xc00003ea20)
	/usr/local/go/src/net/http/server.go:2171 +0x33
github.com/go-chi/cors.(*Cors).Handler.func1({0x75a1e1f812f8, 0xc0008eb680}, 0xc00003ea20)
	/home/nikos/projects/identity-platform-admin-ui/vendor/github.com/go-chi/cors/cors.go:228 +0x2e5
net/http.HandlerFunc.ServeHTTP(0xc00067c560, {0x75a1e1f812f8, 0xc0008eb680}, 0xc00003ea20)
	/usr/local/go/src/net/http/server.go:2171 +0x33
github.com/canonical/identity-platform-admin-ui/internal/monitoring.(*Middleware).ResponseTime.func1.1({0x2e44690, 0xc0007d4c00}, 0xc00003ea20)
	/home/nikos/projects/identity-platform-admin-ui/internal/monitoring/middlewares.go:39 +0x127
net/http.HandlerFunc.ServeHTTP(0xc00067c580, {0x2e44690, 0xc0007d4c00}, 0xc00003ea20)
	/usr/local/go/src/net/http/server.go:2171 +0x33
github.com/go-chi/chi/v5/middleware.RequestID.func1({0x2e44690, 0xc0007d4c00}, 0xc00003e900)
	/home/nikos/projects/identity-platform-admin-ui/vendor/github.com/go-chi/chi/v5/middleware/request_id.go:76 +0x253
net/http.HandlerFunc.ServeHTTP(0xc0005dca08, {0x2e44690, 0xc0007d4c00}, 0xc00003e900)
	/usr/local/go/src/net/http/server.go:2171 +0x33
github.com/go-chi/chi/v5.(*Mux).ServeHTTP(0xc0006f9f20, {0x2e44690, 0xc0007d4c00}, 0xc00003e900)
	/home/nikos/projects/identity-platform-admin-ui/vendor/github.com/go-chi/chi/v5/mux.go:90 +0x29f
go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp.(*middleware).serveHTTP(0xc0008ce0b0, {0x2e44690, 0xc0007d4c00}, 0xc00003cb40, {0x2e2f480, 0xc0006f9f20})
	/home/nikos/projects/identity-platform-admin-ui/vendor/go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp/handler.go:217 +0x1468
go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp.NewMiddleware.func1.1({0x2e400b8, 0xc0008fe000}, 0xc00003cb40)
	/home/nikos/projects/identity-platform-admin-ui/vendor/go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp/handler.go:81 +0x51
net/http.HandlerFunc.ServeHTTP(0xc0008d8400, {0x2e400b8, 0xc0008fe000}, 0xc00003cb40)
	/usr/local/go/src/net/http/server.go:2171 +0x33
net/http.serverHandler.ServeHTTP({0xc0008e4000}, {0x2e400b8, 0xc0008fe000}, 0xc00003cb40)
	/usr/local/go/src/net/http/server.go:3142 +0x257
net/http.(*conn).serve(0xc0008dc360, {0x2e4db10, 0xc0008c4960})
	/usr/local/go/src/net/http/server.go:2044 +0x1ab5
created by net/http.(*Server).Serve in goroutine 242
	/usr/local/go/src/net/http/server.go:3290 +0xa9a

The reason for this is that:

ids, err := a.service.CreateIdentity(r.Context(), &identity.CreateIdentityBody)

returned:
"dial failed: dial tcp 0.0.0.0:1025: connect: connection refused"

pkg/web/router.go

internal/mail/service.go

pkg/identities/service.go

BarcoMasile · 2024-09-09T16:23:31Z

This breaks the skaffold deployment, will this be addressed in another PR?

Yes, we did it also for the auth implementation, we'll ask our trusty @shipperizer to adjust the skaffold :D

On top of that I had trouble testing it locally. I was unable to make the maildev server to work. I started the skaffold setup, then spun up a localhost admin-ui and configured it with:
"MAIL_HOST": "0.0.0.0",
"MAIL_PORT": "1025",
"MAIL_FROM_ADDRESS": "a@b.com",

For local testing, I used localhost as MAIL_HOST. Try that also.

I couldn't use the API via the frontend, because the validation failed as the frontend does not allow you to set a password. I had to use curl to access the API:

I assume this needs to be addressed by the web team to fix it on the frontend.

2024/09/09 15:19:39 http: panic serving 127.0.0.1:41704: runtime error: invalid memory address or nil pointer dereference
goroutine 245 [running]:
net/http.(*conn).serve.func1()
	/usr/local/go/src/net/http/server.go:1903 +0x13f
panic({0x27c6360?, 0x3ca4d80?})
	/usr/local/go/src/runtime/panic.go:770 +0x136
github.com/canonical/identity-platform-admin-ui/pkg/identities.(*API).error(0xc0007e4060, 0x0)
	/home/nikos/projects/identity-platform-admin-ui/pkg/identities/handlers.go:259 +0x43
github.com/canonical/identity-platform-admin-ui/pkg/identities.(*API).handleCreate(0xc0007e4060, {0x75a1e1f812f8, 0xc0008eb700}, 0xc00088e5a0)
	/home/nikos/projects/identity-platform-admin-ui/pkg/identities/handlers.go:155 +0x608

The reason for this is that:
ids, err := a.service.CreateIdentity(r.Context(), &identity.CreateIdentityBody)
returned: "dial failed: dial tcp 0.0.0.0:1025: connect: connection refused"

About this, it is actually 2 different issues:

the CreateIdentity returns an error, and that's related to the maildev container, make sure it's exposed on the right port and use localhost as MAIL_HOST.

The other issue is a null pointer exception in the error method inside the identities handlers.
It seems to lament that the *kClient.GenericError passed in the method is actually nil.
On this one I can open an issue. Thing is, I didn't change how that value gets passed so that bug must have been in there already.

wood-push-melon · 2024-09-09T21:53:19Z

Throw some dummy questions because I probably lack some context about the feature:

What SMTP service will be used when going to staging/production?
Did not find the corresponding CLI command. Is it going to be another PR?
A very minor stuff: should we add the new env vars to the README.md? (Maybe we would like to find another way to doc the env vars in the future)

internal/config/specs.go

nsklikas · 2024-09-10T04:31:16Z

I didn't notice that you were forwarding the mail server's port to 1026, that's why it didn't work before. Now it works great.

nsklikas · 2024-09-10T04:35:27Z

About this, it is actually 2 different issues:
* the CreateIdentity returns an error, and that's related to the maildev container, make sure it's exposed on the right port and use `localhost` as MAIL_HOST.
The other issue is a null pointer exception in the error method inside the identities handlers. It seems to lament that the *kClient.GenericError passed in the method is actually nil. On this one I can open an issue. Thing is, I didn't change how that value gets passed so that bug must have been in there already.

Before this change, the Service.CreateIdentity method would return an error only if an error was returned from Kratos. In that case the Error field in the Kratos response would always be populated. With this change, an error can occur from the mail server as well. This causes this panic, as the Kratos response (correctly) has no error.

shipperizer

I think we need a service/backoffice endpoint that is only exposed internally just in case stuff goes wrong and we need to resend the email

happy for an issue to be created and tackled down the line

BarcoMasile · 2024-09-10T09:11:15Z

Before this change, the Service.CreateIdentity method would return an error only if an error was returned from Kratos. In that case the Error field in the Kratos response would always be populated. With this change, an error can occur from the mail server as well. This causes this panic, as the Kratos response (correctly) has no error.

@nsklikas I fixed this by separating at the service level the creation of the identity from the sending of the email (I should have done that earlier so each service method only has a single responsibility.

I think we need a service/backoffice endpoint that is only exposed internally just in case stuff goes wrong and we need to resend the email

happy for an issue to be created and tackled down the line

@shipperizer I'll add a handler for sending the email. I'll either add the timeout or open an issue for it.

shipperizer · 2024-09-10T09:55:01Z

@shipperizer I'll add a handler for sending the email.

cli command is probably better as login UI has no authorization

nsklikas

Looks much better now, good job

BarcoMasile · 2024-09-10T13:31:57Z

@wood-push-melon

What SMTP service will be used when going to staging/production?

This is up to who deploys the platform, I personally don't have a strong preference

Did not find the corresponding CLI command. Is it going to be another PR?

After talking to @shipperizer , this should be part of IAM-979 (I will need to update that description though)

A very minor stuff: should we add the new env vars to the README.md? (Maybe we would like to find another way to doc the env vars in the future)

Yes, we should! I forgot about that, I was a little in a rush to push this PR, my bad. Thanks for reminding me!

BarcoMasile · 2024-09-10T13:33:55Z

@shipperizer

cli command is probably better as login UI has no authorization

I'll add a task for that then since it deserves it's own activity

to avoid handling low level operations with email sending

… test

BarcoMasile · 2024-09-10T14:13:47Z

@shipperizer @nsklikas @wood-push-melon
Added env vars to the Readme, added a new env var set an explicit timeout on the sending of the mail, default value 15 seconds.
And switched to the compound obeject for the email service creation.

shipperizer · 2024-09-10T14:25:45Z

@BarcoMasile create a ticket for skaffold fixes

good to merge

BarcoMasile · 2024-09-10T14:29:17Z

BarcoMasile requested a review from a team as a code owner September 6, 2024 15:45

shipperizer reviewed Sep 6, 2024

View reviewed changes

internal/mail/service.go Outdated Show resolved Hide resolved

internal/mail/service.go Outdated Show resolved Hide resolved

internal/mail/service.go Show resolved Hide resolved

pkg/identities/service.go Outdated Show resolved Hide resolved

pkg/identities/service.go Show resolved Hide resolved

BarcoMasile force-pushed the IAM-978-user-invite-initiation branch from 7655253 to 602972d Compare September 9, 2024 08:15

nsklikas reviewed Sep 9, 2024

View reviewed changes

pkg/web/router.go Outdated Show resolved Hide resolved

internal/mail/service.go Outdated Show resolved Hide resolved

internal/mail/service.go Outdated Show resolved Hide resolved

pkg/identities/service.go Show resolved Hide resolved

BarcoMasile force-pushed the IAM-978-user-invite-initiation branch from 602972d to c13f0a2 Compare September 9, 2024 15:31

wood-push-melon reviewed Sep 10, 2024

View reviewed changes

internal/config/specs.go Outdated Show resolved Hide resolved

shipperizer reviewed Sep 10, 2024

View reviewed changes

BarcoMasile force-pushed the IAM-978-user-invite-initiation branch from c13f0a2 to 5de9995 Compare September 10, 2024 09:02

nsklikas previously approved these changes Sep 10, 2024

View reviewed changes

BarcoMasile added 2 commits September 10, 2024 15:38

feat: add github.com/wneessen/go-mail v0.4.4 dependency

5182270

to avoid handling low level operations with email sending

feat: add template loading + test + TEMPORARY mail template

6c95a25

BarcoMasile dismissed nsklikas’s stale review via 4258426 September 10, 2024 13:39

BarcoMasile force-pushed the IAM-978-user-invite-initiation branch from 5de9995 to 4258426 Compare September 10, 2024 13:39

BarcoMasile added 8 commits September 10, 2024 16:05

feat: add env vars for mail client

3ab1acb

feat: add interfaces + implement emailservice

b2f0ae9

feat: add SendUserCreationEmail method

0cc1d3f

refactor: adopt new mailService

d5d7567

test: add email service tests

6c5a775

refactor: adjust existing tests

97e8182

refactor: add send-email as separate action inside creation handler +…

1a62043

… test

doc: added mailing env vars to readme

f4d0ca9

BarcoMasile force-pushed the IAM-978-user-invite-initiation branch from 4258426 to f4d0ca9 Compare September 10, 2024 14:09

BarcoMasile requested review from nsklikas and shipperizer September 10, 2024 14:13

shipperizer enabled auto-merge September 10, 2024 14:25

shipperizer approved these changes Sep 10, 2024

View reviewed changes

wood-push-melon approved these changes Sep 10, 2024

View reviewed changes

shipperizer merged commit 24391a4 into main Sep 10, 2024
10 checks passed

shipperizer deleted the IAM-978-user-invite-initiation branch September 10, 2024 19:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

IAM 978 User invite - send email for identity creation with recovery code and link #403

IAM 978 User invite - send email for identity creation with recovery code and link #403

BarcoMasile commented Sep 6, 2024 •

edited

Loading

nsklikas left a comment

BarcoMasile commented Sep 9, 2024

wood-push-melon commented Sep 9, 2024

nsklikas commented Sep 10, 2024

nsklikas commented Sep 10, 2024

shipperizer left a comment

BarcoMasile commented Sep 10, 2024

shipperizer commented Sep 10, 2024

nsklikas left a comment

BarcoMasile commented Sep 10, 2024 •

edited

Loading

BarcoMasile commented Sep 10, 2024

BarcoMasile commented Sep 10, 2024

shipperizer commented Sep 10, 2024

BarcoMasile commented Sep 10, 2024

IAM 978 User invite - send email for identity creation with recovery code and link #403

IAM 978 User invite - send email for identity creation with recovery code and link #403

Conversation

BarcoMasile commented Sep 6, 2024 • edited Loading

Description

Testing

nsklikas left a comment

Choose a reason for hiding this comment

BarcoMasile commented Sep 9, 2024

wood-push-melon commented Sep 9, 2024

nsklikas commented Sep 10, 2024

nsklikas commented Sep 10, 2024

shipperizer left a comment

Choose a reason for hiding this comment

BarcoMasile commented Sep 10, 2024

shipperizer commented Sep 10, 2024

nsklikas left a comment

Choose a reason for hiding this comment

BarcoMasile commented Sep 10, 2024 • edited Loading

BarcoMasile commented Sep 10, 2024

BarcoMasile commented Sep 10, 2024

shipperizer commented Sep 10, 2024

BarcoMasile commented Sep 10, 2024

BarcoMasile commented Sep 6, 2024 •

edited

Loading

BarcoMasile commented Sep 10, 2024 •

edited

Loading