Skip to content

Conversation

@std-odoo
Copy link
Contributor

@std-odoo std-odoo commented Jan 10, 2025

Purpose

When a user has TOTP enabled, on its first login, he receives 2 emails

  • "New Connection" email, sent when he enters his credentials
  • "Trusted Device Added", if he checked "Don't ask again on this device"

Because the user needs to login to add a device, and that he will receive the
"New Connection" email anyway, we don't notify when adding a new device.

We merge the information (like location, etc) in the "New Connection" email
template, and we also make all notification emails consistent (same template, same
information).

Task-4070794

@robodoo
Copy link
Contributor

robodoo commented Jan 10, 2025

Pull request status dashboard

@C3POdoo C3POdoo added the RD research & development, internal work label Jan 10, 2025
@std-odoo std-odoo force-pushed the master-auth_totp-dont-spam-on-new-device-std branch 5 times, most recently from 8c7f9f1 to 22595bf Compare January 16, 2025 09:00
@C3POdoo C3POdoo requested review from a team January 16, 2025 09:03
@std-odoo std-odoo changed the title WIP [IMP] auth_signup: do not spam the user on new connection Jan 16, 2025
@std-odoo std-odoo force-pushed the master-auth_totp-dont-spam-on-new-device-std branch 8 times, most recently from a2ce264 to add1e9e Compare January 16, 2025 11:35
Copy link
Contributor

@reth-odoo reth-odoo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks pretty clean as far as this is doable.

Would maybe add a test case around the test you updated to cover this feature if it's not too finicky to set up

Sadly I don't think we can avoid some kind of hacky global storage :/

@std-odoo std-odoo force-pushed the master-auth_totp-dont-spam-on-new-device-std branch 2 times, most recently from 9936b7b to 218c65b Compare January 23, 2025 08:01
Copy link
Contributor Author

@std-odoo std-odoo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@reth-odoo Hi :)

Thanks for the validation :) I did the changes, and answered

Cheers

Copy link
Contributor

@reth-odoo reth-odoo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed, too bad we don't have a warning regardless of TOTP to be honest
Linting comment and one for the session key again

Greening for testing already

@std-odoo std-odoo force-pushed the master-auth_totp-dont-spam-on-new-device-std branch from 218c65b to f1585e5 Compare January 23, 2025 09:58
Copy link
Contributor Author

@std-odoo std-odoo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@reth-odoo Good idea for the pop :) Done :)

@seb-odoo seb-odoo removed the request for review from a team January 23, 2025 10:00
@std-odoo std-odoo force-pushed the master-auth_totp-dont-spam-on-new-device-std branch 3 times, most recently from 97dd66d to 06748d0 Compare February 5, 2025 10:16
@C3POdoo C3POdoo requested review from a team, rco-odoo and xmo-odoo and removed request for a team February 6, 2025 09:26
@std-odoo std-odoo force-pushed the master-auth_totp-dont-spam-on-new-device-std branch 2 times, most recently from 9a25a41 to f1dceb6 Compare February 6, 2025 12:12
Copy link
Contributor

@awa-odoo awa-odoo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello @std-odoo

Left some picky remarks & questions.
Overall a very clean work!

Thanks & cheers!

@std-odoo std-odoo force-pushed the master-auth_totp-dont-spam-on-new-device-std branch 7 times, most recently from 5cf1e98 to 228f30d Compare February 7, 2025 10:50
Copy link
Contributor Author

@std-odoo std-odoo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@awa-odoo Hi :)

I did the changes :)

Cheers

Copy link
Contributor

@awa-odoo awa-odoo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, just one extra cleaning comment 🧦

@std-odoo std-odoo force-pushed the master-auth_totp-dont-spam-on-new-device-std branch from 228f30d to ee3914c Compare February 7, 2025 13:13
Copy link
Contributor Author

@std-odoo std-odoo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@awa-odoo Change done :)

@awa-odoo
Copy link
Contributor

awa-odoo commented Feb 7, 2025

@std-odoo

instead of waiting for the CROM to run

I like CROM better, we should rename all CRONs to CROMs !

@std-odoo std-odoo force-pushed the master-auth_totp-dont-spam-on-new-device-std branch from ee3914c to c1c3714 Compare February 7, 2025 13:59
@std-odoo
Copy link
Contributor Author

std-odoo commented Feb 7, 2025

@awa-odoo Good catch 😅
(language tool rale for CRON but not for CROM 😆)

Purpose
=======
When a user has TOTP enabled, on its first login, he receives 2 emails
- "New Connection" email, sent when he enters his credentials
- "Trusted Device Added", if he checked "Don't ask again on this device"

Because the user needs to login to add a device, and that he will receive the
"New Connection" email anyway, we don't notify when adding a new device.

We merge the information (like location, etc) in the "New Connection" email
template, and we also make all notification emails consistent (same template, same
information).

Make the "Ask Password" and "TOTP settings" dialog a bit smaller.

For all security notification, send right away the notification
(instead of waiting for the CRON to run).

Task-4070794
@std-odoo std-odoo force-pushed the master-auth_totp-dont-spam-on-new-device-std branch from c1c3714 to ba167b1 Compare February 10, 2025 07:41
@awa-odoo
Copy link
Contributor

@robodoo r+

robodoo pushed a commit that referenced this pull request Feb 10, 2025
Purpose
=======
When a user has TOTP enabled, on its first login, he receives 2 emails
- "New Connection" email, sent when he enters his credentials
- "Trusted Device Added", if he checked "Don't ask again on this device"

Because the user needs to login to add a device, and that he will receive the
"New Connection" email anyway, we don't notify when adding a new device.

We merge the information (like location, etc) in the "New Connection" email
template, and we also make all notification emails consistent (same template, same
information).

Make the "Ask Password" and "TOTP settings" dialog a bit smaller.

For all security notification, send right away the notification
(instead of waiting for the CRON to run).

Task-4070794

closes #193181

Related: odoo/upgrade#7077
Signed-off-by: Warnon Aurélien (awa) <awa@odoo.com>
@robodoo robodoo added the 18.2 label Feb 10, 2025
@robodoo robodoo closed this Feb 10, 2025
@tde-banana-odoo
Copy link
Contributor

conan-the-barbarian-crom

@fw-bot fw-bot deleted the master-auth_totp-dont-spam-on-new-device-std branch February 24, 2025 13:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

18.2 RD research & development, internal work

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants