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

Rethinking "Card Activation" #766

Closed
michael-markl opened this issue Jan 29, 2023 · 11 comments · Fixed by #804
Closed

Rethinking "Card Activation" #766

michael-markl opened this issue Jan 29, 2023 · 11 comments · Fixed by #804
Assignees
Labels
prio: high Issue must be solved within the next weeks. Task

Comments

@michael-markl
Copy link
Member

michael-markl commented Jan 29, 2023

Das Issue diskutiert einen Vorschlag wie wir den Vorgang "Karte aktivieren" ändern können, um
a) sicherzustellen, dass nur maximal ein Gerät (oder eine konfigurierbare maximale Anzahl an Geräten) über eine aktivierte Karte verfügt.
b) eine bessere Kontrolle über aktivierte Karten zu erhalten. (Wir können merken, ob eine Karte "zu" häufig aktiviert wird)
[ c) den Vorgang "Karte aktivieren" strukturell trennen vom Verifizieren ]

Status Quo

  1. Der Berechtigte bekommt per Post/Mail einen Aktivierungscode.
  2. Der Aktivierungscode enthält ein TOTP-Secret, welches auch in der DB hinterlegt ist.
  3. Der Kartenhalter scannt den Aktivierungscode mit der App ein.
  4. Die App versucht den Code zu verifizieren, indem sie mit dem TOTP-Secret einen TOTP-Code ableitet und gegen den Server verifiziert.
  5. Ist die Verifikation erfolgreich, wird der Aktivierungscode in der App gespeichert
  6. Die App erzeugt kontinuierlich TOTP-Codes anhand des TOTP-Secrets (und damit die zeitlich begrenzt gültigen Verifizierungs QR-Codes)

Vorschlag

Der Vorschlag sieht vor:

  1. Das TOTP-Secret im Aktivierungscode wird mit einem Aktivierungs-Secret ersetzt, welches in der DB hinterlegt ist.
  2. Beim Scannen des Aktivierungscodes in der App, fragt die App mit dem Aktivierungs-Secret beim Server ein TOTP-Secret an.
  3. Falls die Karte gültig ist und das Aktivierungs-Secret mit dem hinterlegten übereinstimmt, erzeugt der Server ein neues TOTP-Secret, speichert es in der DB und sendet es an App. Falls in der DB bereits ein TOTP-Secret für die Karte existierte, wird dieses überschrieben.
  4. Die App speichert das TOTP-Secret, und nutzt es um TOTP-Codes für die Verifizierung zu erzeugen (wie bisher).

Durch Schritt 3. werden bereits aktivierte Apps automatisch "deaktiviert", da ihr TOTP-Secret "nicht mehr aktuell" ist.

Wir könnten außerdem das Konzept "CardActivation" einführen: Eine erfolgreiche Aktivierung einer Karte in einer App. (Und etwa Card~CardActivation zu einer 1:n Beziehung machen).

Nachteile / Konsequenzen

  • Die Apps sollten regelmäßig "prüfen", ob die für sie aktivierte Karte inkl. TOTP-Secret noch gültig ist (das ist allerdings bereits geplant: Check the validity of a card against the backend regularly #677 )
  • Es könnte den Kartenhalter verwirren, wenn seine aktivierte App "ohne sein Zu-Tun" deaktiviert wird, weil er den Aktivierungscode auf einem anderen Gerät eingescannt hat.

Aufwandsschätzung

Könnte in einem einzigen PR umsetzbar sein.
Nötige Stellschrauben:

  • Anpassen des DynamicActivationCode (Rename totpSecret to activationSecret)
  • Neue GraphQL-Schnittstelle "activateCode"
  • Neue Spalte in der Card-Table in der DB für "activationSecret"
  • Aufrufen dieser Schnittstelle im Frontend statt der "verifyCode" Schnittstelle
  • Anpassen der Persistenz im Frontend (z.B. durch einführen eines neuen protos, welcher cardInfo, pepper und totp-secret enthält)
@michael-markl michael-markl added Task discussion-needed We need to resolve the questions in the issue. blocked Task or user story can not be continued at the moment prio: high Issue must be solved within the next weeks. labels Jan 29, 2023
@michael-markl michael-markl added this to the Nürnberg Launch milestone Jan 29, 2023
@dkehne
Copy link

dkehne commented Jan 30, 2023

Aus nicht-technischer Sicht eine sinnvolle Erweiterung.

@sarahsporck
Copy link
Contributor

Macht definitiv Sinn. Hab mich schon gefragt wie wir verhindern wollen, dass sonst eine Karte mehrmals aktiviert wird :)

@sarahsporck
Copy link
Contributor

Und vermutlich macht es Sinn Schritt 3 dann irgendwann mit so einem ping-pong zu implementieren á la
-> user scannt aktivierungscode auf neuem gerät
-> db findet einen bereits existierenden aktivierten eintrag
-> user wird gefragt (und informiert) ob er die karte auf ein neues gerät übertragen möchte
-> ja: eine "activation_override" request wird ans backend geschickt und die karte wird überschrieben
-> nein: nichts wird getan

@michael-markl michael-markl removed discussion-needed We need to resolve the questions in the issue. blocked Task or user story can not be continued at the moment labels Feb 3, 2023
@michael-markl
Copy link
Member Author

@sarahsporck Thanks for the input. That's a good extension. The override_existing can be a flag in the input of the same mutation activateCard. The return type of activateCard should probably be an enum SUCCESS | DID_NOT_OVERRIDE_EXISTING | FAILED.

I removed the blocked tag. I think we should implement this now.

@maxammann
Copy link
Member

Sounds all good!

Und vermutlich macht es Sinn Schritt 3 dann irgendwann mit so einem ping-pong zu implementieren á la

Should we create a seprate task for this though? It involves some UI which maybe delays how quickly we can implement this.

@michael-markl
Copy link
Member Author

Should we create a seprate task for this though? It involves some UI which maybe delays how quickly we can implement this.

Yes, we should. We should keep this in mind when implementing though (and probably add the override_existing flag in the graphql endpoint already, but just always set it to true in the frontend for now).

@maxammann
Copy link
Member

Yes that sounds like a plan!

@maxammann
Copy link
Member

We could name that flag maybe dry though. Or force_overwrite? Is overwrite or override correct here? Or both?

@sarahsporck sarahsporck self-assigned this Feb 6, 2023
@sarahsporck
Copy link
Contributor

Should Nuernbergs Static QR codes will still not support the activation secret. How will we implement this?

@michael-markl
Copy link
Member Author

Yes, this proposal will not change the static qr codes.

@sarahsporck
Copy link
Contributor

We could name that flag maybe dry though. Or force_overwrite? Is overwrite or override correct here? Or both?

I prefer overwrite as override is already a reserved kotlin keyword

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
prio: high Issue must be solved within the next weeks. Task
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants