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

PERSON: Benutzer-Zugang entfernen #2069

Closed
37 of 40 tasks
Michael-Schaer opened this issue Apr 4, 2023 · 10 comments · Fixed by #2291
Closed
37 of 40 tasks

PERSON: Benutzer-Zugang entfernen #2069

Michael-Schaer opened this issue Apr 4, 2023 · 10 comments · Fixed by #2291

Comments

@Michael-Schaer
Copy link
Contributor

Michael-Schaer commented Apr 4, 2023

Bisher ist der Zugriff auf ein Hitobito-System zeitlich unbeschränkt. Das ist aus Security- und Datenschutz-Sicht nicht optimal und sollte angepasst werden.

Daraus ergibt sich folgende Überlegung:

Automatismus

  • Nach 18 Monaten ohne Login wird eine Warnung an die Haupt-E-Mail geschickt, dass der Zugang bald entfernt wird
  • Falls nicht wieder eingeloggt, wird das login einen Monat später gesperrt (ohne die Haupt-E-Mail anzupassen)
    • Dabei wird auch der betroffene Benutzer per Mail informiert
  • Die Fristen (18 + 1 Monat) sind pro Instanz anpassbar
  • Für andere Wagons wird der Job standardmässig deaktiviert
  • Neue OAuth-Zugriffe sollen bei gesperrten Accounts nicht mehr möglich sein, bestehende Sessions müssen aber nicht unbedingt überprüft werden

Funktion "Login schicken"

  • Der Button "Login schicken" entsperrt neu auch den Account.
  • Button-Text anpassen "Entsperren und Login schicken", falls der Account gesperrt ist
  • Personen mit Schreibrechten können diesen Button verwenden
  • Funktion "Passwort vergessen" schlägt neu fehl, wenn der Account gesperrt ist. ⚠️

Funktion "Login sperren"

  • Hiermit wird das Login manuell gesperrt.
  • Erscheint im Drop-Down unter "Login schicken"
  • Im Security-Tab werden die zwei Abschnitte angepasst und ein Button zur Sperrung eingebaut:

Vermutest du, dass jemand die Kontrolle über die E-Mail-Adresse von [Person] unerlaubt übernommen hat?

Möchtest du [Person] ganz aus Hitobito ausschliessen?

  • Da diese Option in der Diskussion umstritten war, sollte sie pro Wagon deaktivierbar sein.

Login-Icons

Sperrung soll im Icon miteinbezogen werden:

return :login if email? && password?

Login UI

Wenn sich eine gesperrte Person einloggt, erhält sie ein E-Mail (konfigurierbar in den Texten):

Dein Account wurde automatisch aufgrund von Inaktivität gesperrt, du kannst dich bei deinen Gruppenleiterinnen oder Adressverwalterinnen melden, um den Zugang wieder zu erhalten.

Im Moment gibt es die Möglichkeit, herauszufinden, ob es eine E-Mail-Adresse in der MiData gibt oder nicht, wenn die Passwort-Reset-Funktion verwendet wird. Dieser Umstand sollte wenn möglich auch geändert werden.


Siehe auch die ursprüngliche Idee, die kontrovers aufgenommen wurde: #1297

Tech-Spec

  • Die Sperre ist, in Absprache mit @Michael-Schaer, nicht als Login-Sperre sondern als hitobito-Benutzungs-Sperre umzusetzen. Der Login inkl. 2FA darf weiterhin funktionieren, aber nach dem Login soll der User blockiert sein und nur einen Logout-Link und eine simple Nachricht über den Zustand angezeigt bekommen. OAuth soll nicht mehr funktionieren, aber Passwortwechsel schon (falls einfach umsetzbar)
  • Umsetzung im Core, aber die Frist für die Automatisierung soll statt 18+1 Monate eine extrem lange Zeit sein (oder komplett deaktiviert)
  • Wenn die Fristen in einem Wagon geändert werden, muss man daran denken, was mit den Personen passiert die durch die Friständerung jetzt vielleicht neu von diesem Feature betroffen oder nicht mehr betroffen sind

ToDo

  • Neue Datenbankspalten inactivity_block_warning_sent_at und blocked_at auf Person, beide datetime, default nil
  • Neuer Background-Job InactivityBlockWarningJob, der täglich läuft
    • Sucht in Batches alle Personen, deren last_sign_in_at älter als X Monate ist
      • X Monate ist in den settings.yml konfigurierbar, standardmässig im Core eine extrem lange Zeit oder z.B. nil und in diesem Fall keine Personen suchen
      • Im PBS Wagon auf 18 Monate setzen
    • Setzt die Spalte inactivity_block_warning_sent_at auf DateTime.now
    • Sendet ein neues CustomContent Mail das die Person über die bevorstehende Löschung informiert. Solche Mails können mit der Methode custom_content_mail in einem Mailer versendet werden (dafür auch einen eigenen Mailer anlegen).
  • Neuer Background-Job InactivityBlockJob, der täglich läuft
    • Sucht in Batches alle Personen, deren inactivity_block_warning_sent_at älter als Y Monate ist UND deren blocked_at leer ist
      • Y Monate ist in den settings.yml konfigurierbar, standardmässig im Core eine extrem lange Zeit oder z.B. nil und in diesem Fall keine Personen suchen
      • Im PBS Wagon auf 1 Monat setzen
    • Setzt die Spalte blocked_at auf DateTime.now
  • Auf dem Sicherheits-Tab einer Person in den beiden Fällen email_compromised_situation und suspend_person_situation je einen Button "Zugang blockieren" einbauen
    • Button taucht nur auf, wenn die last_sign_in_at nicht leer ist
    • Button ruft eine neue POST-Action block im SecurityToolsController auf
      • Action verweigert, dass man sich selber oder sich als Imitator blockiert (@user darf weder current_person noch origin_user sein)
      • Action setzt blocked_at der Person auf DateTime.now und zeigt Flash-Nachricht
    • Falls die Person bereits blockiert ist (blocked_at ist nicht nil):
      • Statt den Buttons eine andere, passende Nachricht anzeigen, z.B. "Diese Person ist blockiert, und hat daher keinen Zugriff mehr."
      • Weitere Section anzeigen mit Erklär-Text dass die Person blockiert ist, und was das für Auswirkungen hat, und falls X und Y nicht nil sind auch erklären dass das nach X + Y Monaten ohne Login automatisch passiert, und mit Button um die Person zu entsperren.
      • Button ruft eine neue POST-Action unblock im SecurityToolsController auf
        • Action setzt blocked_at und inactivity_block_warning_sent_at auf nil und zeigt Flash-Nachricht
  • Im Login-Dropdown eine neue Option zum Entsperren einbauen
    • Option taucht nur auf, wenn template.can?(:update, @user)
    • Option taucht nur auf, wenn @user.blocked_at.present?
    • Option ruft POST-Action unblock im SecurityToolsController auf
  • In der Berechnung des Login-Status Icons (kann auf Personen-Detail oder in Personenliste als zusätzliche Spalte angeschaut werden) einen neuen Status "blockiert" einbauen
  • Für die eigentliche Sperrung eine neue before_action im ApplicationController anlegen
    • In diesem Hook überprüfen, ob current_person.blocked_at.present?, und wenn ja, eine spezielle minimalistische View rendern (ein render in diesem Hook müsste den Rest der Action abbrechen, falls nicht müsste es mit einem redirect gehen)
    • Ausnahmen sind alle Actions die für Login, 2FA und Passwort-Reset nötig sind. Die OAuth Actions (Doorkeeper::Authorizations#new etc.) sollen aber explizit blockiert / abgebrochen werden
    • Für Formate abgesehen von HTML kann auch einfach ein HTTP 403 ausgegeben werden wenn das einfacher ist
    • Zu klären mit @Michael-Schaer wenn das implementiert ist: Gibt es noch mehr Ausnahmen?
    • Falls es aus irgendeinem Grund nicht möglich ist, das im before_action Hook des ApplicationControllers zu realisieren, könnte man stattdessen eine eigene Rack Middleware schreiben. Aber dann müsste man sich möglicherweise ActiveRecord und Devise selber einrichten oder zumindest requiren...
  • In separater Datenbankmigration alle Personen suchen, deren last_sign_in_at bereits jetzt älter als X + Y Monate ist, und bei diesen Personen block_warning_sent_at auf heute vor Y Monaten setzen (an diese Personen wollen wir keine Warnung mehr schicken)
  • Specs schreiben
  • Kunde wegen Übersetzungen informieren
  • Mit angemessener Rolle "durchklicken"
  • CHANGELOG-Eintrag unter "unreleased" unten hinzufügen
@nchiapol
Copy link
Contributor

nchiapol commented Apr 4, 2023

@Michael-Schaer Vielen Dank für deine Beschreibung. Dieser Prozess kann aus meiner Sicht im Core umgesetzt werden. Da wir das ganz deaktivieren oder auch lange Fristen wählen können, sollte das kein Problem sein. Grundsätzlich scheint mir das ein Mehrwert.

Weitere Gedanken:

  • Ich fände ein selbstständiges Entsperren via separatem Kanal/2FA immer noch einen Mehrwert (der kürzere Fristen erlauben würde). Das ist aber ein Feature, das gut später ergänzt werden kann.
  • Bisher kann man sich das Login auch selbst via Passwort vergessen schicken. Das müsstet ihr wohl ebenfalls Anpassungen vornehmen. (Das könnte aber wieder negative Auswirkungen auf uns haben.)
  • Die Meldung beim Login-Versuch eines gesperrten Accounts müsste via Texte konfigurierbar sein.

Und noch als Hinweis: Im Zusammenhang mit der ursprünglichen Diskussion scheint auch die neue Permission dank des SWW interessant: #2065

@Michael-Schaer
Copy link
Contributor Author

Merci für die prompte Rückmeldung!

Ich fände ein selbstständiges Entsperren via separatem Kanal/2FA immer noch einen Mehrwert (der kürzere Fristen erlauben würde). Das ist aber ein Feature, das gut später ergänzt werden kann.

Ja, das müsste dann halt Wagon-Spezifisch gelöst werden.

Bisher kann man sich das Login auch selbst via Passwort vergessen schicken. Das müsstet ihr wohl ebenfalls Anpassungen vornehmen. (Das könnte aber wieder negative Auswirkungen auf uns haben.)

Ja, wir müssen wohl die Passwort-Vergessen-Logik noch anpassen. Sicher würde ich aber die Funktion offen lassen, solange der Zugriff nicht gesperrt wurde. Merci für den Hinweis!

Die Meldung beim Login-Versuch eines gesperrten Accounts müsste via Texte konfigurierbar sein.

Merci für den Hinweis! Nehme ich auf.

@richardjubla
Copy link
Contributor

@Michael-Schaer

Finde die Überlegung sinnvoll und nehme das Thema gerne bei uns in der Fachgruppe auf. Inaktive User zu "schützen" halte ich für sinnvoll.

Die Meldung beim Login-Versuch eines gesperrten Accounts müsste via Texte konfigurierbar sein.

Normalerweise gibt es keinen Hinweis zu einem Login-Versuch. Auch die Passwort-Vergessen-Logik gibt nicht preis, ob es den Account auch wirklich gibt. So wird verhindert, dass jemand alle möglichen E-Mail-Adressen durchprobiert.

@Michael-Schaer
Copy link
Contributor Author

Guter Input! Ich habe die Anforderung angepasst, so dass ein Mail verschickt wird, anstelle einer Meldung im UI.

@Michael-Schaer
Copy link
Contributor Author

Ich habe eine Anforderung gelöscht, die gar nicht nötig ist. Siehe #2148

@richardjubla
Copy link
Contributor

@Michael-Schaer

Wie sieht es mit Accounts ohne aktives Login aus?
Deine Intention ist es ja, dass ein Account nach X Monaten nicht übernommen wird. Für einen Account ohne Login kann ja aber jederzeit mit der Funktion Passwort-Vergessen dieser in einen aktiven Account umgewandelt werden.
Ein Profil ohne Login müsste demnach auch nach X Monaten oder sofort gesperrt sein. (Wirkung: Passwort vergessen geht nicht und jemand mit Schreibrechten muss Profil für ein aktives Login "aktivieren")

@Michael-Schaer
Copy link
Contributor Author

Ich verstehe deinen Punkt!

Wenn nur eine Haupt-E-Mail eingetragen ist, aber das Login gar nie verschickt wurde, ist die Angriffsfläche vielleicht etwas kleiner: Es wurde kein Einladungs-Mail verschickt und es gibt keine Login-Aktivität, die man abgreifen könnte. Daher fände ich es als ersten Schritt auch in Ordnung, diesen Fall zu ignorieren. Vielleicht hat aber Puzzle hier noch einen Tipp?

@carlobeltrame
Copy link
Member

Dinge die mir beim Durchlesen fürs Refinement aufgefallen sind:

  • In Kombination mit PERSON: Automatisierte Datenlöschung #2106 wird dem User so die einzige self-service-Möglichkeit genommen, die Datenlöschung seines Accounts zu verhindern.
  • Die Sperrungs-Meldung nach dem Login im UI anzuzeigen wäre kein Daten-Leak, weil man ja trotzdem das korrekte Passwort eingeben muss.
  • Zum Punkt mit den Personen die gar kein Login haben: Mir fällt auch keine Wunderlösung ein... Wir könnten höchstens einen Feature Toggle einrichten, mit dem jeder Wagon selber entscheiden kann, ob Personen ohne Login die Passwort-Reset-Funktion verwenden dürfen (bessere User Experience) oder nicht (mehr Security). Wir könnten das theoretisch auch Abhängig von den Rollen der Person machen, aber auch das müsste Wagon-spezifisch laufen. Eine weitere mögliche Unterscheidung: Es gibt Personen die bereits einmal ein Login-Mail oder Passwort-Reset-Mail bekommen haben, aber es nie benutzt haben. Wir könnten diese Personen noch speziell behandeln. Aber das wäre wohl auch eher verwirrend für die User.

@Michael-Schaer
Copy link
Contributor Author

In Kombination mit #2106 wird dem User so die einzige self-service-Möglichkeit genommen, die Datenlöschung seines Accounts zu verhindern.

Das würde zum nächsten Punkt passen: Das Login wird nicht verhindert, danach aber nichts mehr angezeigt. So könnte man das lösen.

Die Sperrungs-Meldung nach dem Login im UI anzuzeigen wäre kein Daten-Leak, weil man ja trotzdem das korrekte Passwort eingeben muss.

Stimmt. Wenn das einfacher ist als das E-Mail, ist das für mich auch eine Möglichkeit.

Wir könnten höchstens einen Feature Toggle einrichten, mit dem jeder Wagon selber entscheiden kann, ob Personen ohne Login die Passwort-Reset-Funktion verwenden dürfen (bessere User Experience) oder nicht (mehr Security)

Ich würde vor dem Versenden des Login-Mails keine automatische Sperrung einrichten. Einfach der Kompatibilität mit dem Core und der Einfachheit zu Liebe.

Es gibt Personen die bereits einmal ein Login-Mail oder Passwort-Reset-Mail bekommen haben, aber es nie benutzt haben.

Diese würde ich gleich behandeln, wie diejenigen, die noch kein Login-Mail erhalten haben.

@daniel-illi
Copy link
Contributor

daniel-illi commented Dec 22, 2023

Offene Punkte:

  • Beim Einloggen muss das inactivity_block_warning_sent_at zurückgesetzt werden
  • Abmelden beim Impersonifizieren eines blockierten Users funktioniert nicht
  • Text auf blocked page sollte einen Hinweis enthalten, wie der Anwender den Account entsperren lassen kann
  • Falls ein Benutzer welcher nicht blockiert ist die blocked page aufruft, ev. umleiten z.B. auf home path?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants