Skip to content
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

ETQ Instructeur, je dois utiliser le 2fa pour le fournisseur d'identité Agent Connect / Mon Compte Pro #10776

Merged
merged 8 commits into from
Sep 18, 2024

Conversation

LeSim
Copy link
Member

@LeSim LeSim commented Sep 11, 2024

Démo:

On a une ptite 404 en plein milieu car moncomptepro ne permet pas pour l'instant d'avoir un callback pour un dev local

Screencast.from.2024-09-16.10-52-33.webm

RAF:

  • problème de redirection infini lorsque vers la page de configuration du 2fa
  • vérifier qu'AC fournit bien les valeurs amr mm pour si la session agent connect a été initié pour un autre site
  • mettre la variable d'env MON_COMPTE_PRO_2FA_NOT_CONFIGURED_URL sur la prod

close #10550

Copy link

codecov bot commented Sep 11, 2024

Codecov Report

Attention: Patch coverage is 92.85714% with 2 lines in your changes missing coverage. Please review.

Please upload report for BASE (main@2d37cfb). Learn more about missing BASE report.
Report is 31 commits behind head on main.

Files with missing lines Patch % Lines
app/services/agent_connect_service.rb 66.66% 2 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main   #10776   +/-   ##
=======================================
  Coverage        ?   84.65%           
=======================================
  Files           ?     1131           
  Lines           ?    25091           
  Branches        ?     4690           
=======================================
  Hits            ?    21240           
  Misses          ?     3851           
  Partials        ?        0           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@LeSim LeSim force-pushed the ac_2fa branch 3 times, most recently from daa9582 to 1a29c06 Compare September 16, 2024 13:34
@LeSim LeSim marked this pull request as ready for review September 16, 2024 13:34
@LeSim LeSim force-pushed the ac_2fa branch 3 times, most recently from c77e8c2 to f1a8956 Compare September 16, 2024 15:04
Copy link
Member

@colinux colinux left a comment

Choose a reason for hiding this comment

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

très clair, quelques remarques
On déploiera progressivement ? peur que ce soit la panique au support et qu'il faille roder les premières réponses à donner auprès des bizdev

cookies.delete NONCE_COOKIE_NAME

if user_info['idp_id'] == MON_COMPTE_PRO_IDP_ID
# MON COMPTE PRO !
if user_info['idp_id'] == MON_COMPTE_PRO_IDP_ID && !amr.include?('mfa')
Copy link
Member

Choose a reason for hiding this comment

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

Vu que ENV['MON_COMPTE_PRO_2FA_NOT_CONFIGURED_URL'] est optionnel, est-ce qu'on devrait pas aussi conditionner le if à son existence, pour ne pas faire un redirect_to nil sur la (jolie) page 500 ?
(SI je comprends bien c'est la présen ec de cette var qui conditionne l'enforcement du MFA)

EDIT: j'ai rédigé ça au fur et à mesure des commits, mais ça me semble tjs valable même avec la page intermédiaire qui a ce bouton vers la page référencée par cette variable

Copy link
Member Author

Choose a reason for hiding this comment

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

La variable 'MON_COMPTE_PRO_2FA_NOT_CONFIGURED_URL' pointe vers la page de configuration du 2ieme facteur dans mon compte pro.

  • comme elle est différente par env, je l'ai mis en variable d'env
  • comme certaines instances n'utilisent pas AgentConnect, je l'ai mis en optionnel.

Du coup, je serai assez pour que ça plante si l'instance décide d'utiliser AgentConnect/MonComptePro mais n'a pas bien configuré l'url.

Copy link
Member

@colinux colinux Sep 18, 2024

Choose a reason for hiding this comment

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

OK, mais un truc m'échappe encore : si on utilise MCP, on est obligé d'avoir du 2FA ? (ça me gène pas en soi d'ailleurs). Auquel cas il faut donc bien faire le lien entre l'activation AgentConnect et cette variable (et ptet préciser que cette variable devient obligatoire)

@LeSim
Copy link
Member Author

LeSim commented Sep 18, 2024

On déploiera progressivement ? peur que ce soit la panique au support et qu'il faille roder les premières réponses à donner auprès des bizdev

Ouais, je suis aussi sur que ça va être le mode panique. D'après les stats, on aurait 400 utilisateurs par semaine concernées.
Il faudra clairement prévenir les biz dev voir assurer directement le support nous même.

Est ce que tu penses qu'il faudrait essayer d'ouvrir plus doucement ?

@colinux
Copy link
Member

colinux commented Sep 18, 2024

Je dirais qu'on peut commencer avec ~40-100 la première semaine, quitte à ouvrir à fond si c'est OK ensuite

@LeSim LeSim added this pull request to the merge queue Sep 18, 2024
Merged via the queue into main with commit 6b322d6 Sep 18, 2024
19 checks passed
@LeSim LeSim deleted the ac_2fa branch September 18, 2024 12:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ETQ admin, je me connecte en 2FA en utilisant AgentConnect
2 participants