search

canonical / identity-platform-admin-ui

Admin UI for the Canonical identity broker and identity provider solution
Other
6 stars 4 forks source link

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

Closed BarcoMasile closed 2 months ago

BarcoMasile commented 2 months ago

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.

BarcoMasile commented 2 months ago

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 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 commented 2 months ago

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

nsklikas commented 2 months ago

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 commented 2 months ago

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.

BarcoMasile commented 2 months ago

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 commented 2 months ago

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

cli command is probably better as login UI has no authorization

BarcoMasile commented 2 months ago

@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 commented 2 months ago

@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

BarcoMasile commented 2 months ago

@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 commented 2 months ago

@BarcoMasile create a ticket for skaffold fixes

good to merge

BarcoMasile commented 2 months ago