canonical / identity-platform-login-ui

Login UI for the Canonical identity broker and identity provider solution
Apache License 2.0
9 stars 6 forks source link

Support multi factor authentication #249

Open natalian98 opened 1 week ago

natalian98 commented 1 week ago

Testing

make npm-build build

export KRATOS_PUBLIC_URL="http://localhost:4433"
export HYDRA_ADMIN_URL="http://localhost:4445"
export BASE_URL="http://localhost:4455"
export PORT="4455"
export TRACING_ENABLED="false"
export LOG_LEVEL="debug"
export AUTHORIZATION_ENABLED="false"

go run . serve

docker-compose -f docker-compose.dev.yml up --build --force-recreate --remove-orphans

Settings

Go to localhost:4455/ui/reset_email and go through the recovery flow (test@example.com). You will first be redirected to change the password: image Then to set up MFA: image After linking the totp app: image

When MFA is set up:

Login

When MFA is configured, you will be required to provide it when signing in: image

When logged in, you can access the settings at localhost:4455/ui/reset_password or localhost:4455/ui/setup_secure.

nsklikas commented 6 days ago

After I input the new password, I get redirected to http://localhost:4455/setup_secure image

Does this only happen to me? I use docker to run external components and run the login-ui app using go run.

natalian98 commented 6 days ago

After I input the new password, I get redirected to http://localhost:4455/setup_secure image

Does this only happen to me? I use docker to run external components and run the login-ui app using go run.

You are right, this change broke the redirect. Adding /ui to the path: ./ui/setup_secure?flow=${data.id}${pwParam} fixes it, @edlerd could you advise why relative paths don't work here?

edlerd commented 5 days ago

You are right, this change broke the redirect. Adding /ui to the path: ./ui/setup_secure?flow=${data.id}${pwParam} fixes it, @edlerd could you advise why relative paths don't work here?

I changed all redirects to not rely on the Next.js router. The next router ignores the current path, hence switching to the vanilla js solution. When I tested it, all redirects work as expected.

nsklikas commented 5 days ago

Had a look at this https://github.com/canonical/identity-platform-login-ui/pull/249#pullrequestreview-2153619712, looks like there is something wrong with the state handling in the flow object (https://github.com/canonical/identity-platform-login-ui/blob/main/ui/components/Flow.tsx#L111). This is used to populate the method field in the request. For some reason some times this is password and some times it is totp (it should be totp). I am not sure how this works, but maybe we should explicitly specify the flow method in the ui when making the call (https://github.com/canonical/identity-platform-login-ui/blob/main/ui/pages/login.tsx#L77)

For some reason natalia was not able to reproduce this (maybe different node versions?), but it happens consistently to me.

What do you think @edlerd @natalian98? We can have a chat tomorrow as I think it will be easier to show you, rather than describe it via text.

edlerd commented 4 days ago

Had a look at this #249 (review), looks like there is something wrong with the state handling in the flow object (https://github.com/canonical/identity-platform-login-ui/blob/main/ui/components/Flow.tsx#L111). This is used to populate the method field in the request. For some reason some times this is password and some times it is totp (it should be totp). I am not sure how this works, but maybe we should explicitly specify the flow method in the ui when making the call (https://github.com/canonical/identity-platform-login-ui/blob/main/ui/pages/login.tsx#L77)

For some reason natalia was not able to reproduce this (maybe different node versions?), but it happens consistently to me.

What do you think @edlerd @natalian98? We can have a chat tomorrow as I think it will be easier to show you, rather than describe it via text.

Pushing a fix for fixating the method based on the fields that are displayed here. This should fix the error you are seeing @nsklikas

nsklikas commented 3 days ago

Pushing a fix for fixating the method based on the fields that are displayed here. This should fix the error you are seeing @nsklikas

@edlerd looks good, can't reproduce it now