Skip to content

Draft: Fix omniauth tos confirmation (Closes #429070, #345524)

boontifex requested to merge boontifex/gitlab:fix-omniauth-tos-confirmation into master

What does this MR do and why?

  • new users have to confirm the Terms of Service on first login using omnitauth (Closes #429070)
  • all users have to confirm the updated Terms of Service if they are changed (Closes #345524 (closed))

Old behavior

  1. user logged in first time
  2. on some point method sign_in_user_flow is called
  3. if the user was new, the former method persist_accepted_terms_if_required was used
  4. persist_accepted_terms_if_required did accept the latest ToS automatically L319, L84 if:
    1. the the user wasn't wrote to disk L316, L81 and
    2. the agreement to ToS is mandatory L317, L82, this logical hard to understand
      • if enforce_terms? was set to false the method was left without accepting the ToS
      • if enforce_terms? was set to true the ToS were accepted automatically
  5. the ToS Page wasn't displayed

Description of how and why

  • the method User.terms_accepted? did only check if some ToS was accepted by the user not which version (Fixes #345524 (closed))
  • the former method persist_accepted_terms_if_required
    • was implemented twice in two classes, doing exactly the same so i moved it to model User
    • was not named appropriate to that what it does, it accepts the ToS automatically, so i renamed it to autoaccept_terms_if_not_enforced
    • had a logical fail in return unless Gitlab::CurrentSettings.current_application_settings.enforce_terms?
      • the method should be left if its mandatory that the latest version of the ToS is accepted by the user, this leads in displaying the ToS page later (Fixes #429070)
      • otherwise the ToS are accepted automatically (imo this is not the best solution (not now, not in the former implementation) but in doesn't break the former logic

New behavior

  1. user log in first time
  2. on some point method sign_in_user_flow is called
  3. if the user is new, the new instance method @user.autoaccept_terms_if_not_enforced is used
  4. @user.autoaccept_terms_if_not_enforced accept the latest ToS automaticaly if:
    1. the the user wasn't wrote to disk L2069 and
    2. the agreement to ToS is not mandatory L2070
  5. the ToS Page is displayed

The ToS page is also displayed to the users (including admins, except *_bots) immediately if the admin updated the ToS text.

MR acceptance checklist

This checklist encourages us to confirm any changes have been analyzed to reduce risks in quality, performance, reliability, security, and maintainability.

Edited by boontifex

Merge request reports