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

1415: Revoke existing cards via user import endpoint #1674

Merged
merged 7 commits into from
Oct 28, 2024

Conversation

seluianova
Copy link
Contributor

@seluianova seluianova commented Oct 8, 2024

Short description

Revoke cards by entitlementId, when user entitlements are revoked or the expiration date is updated to a past date

Proposed changes

  • when user entitlements are updated, check whether they have been revoked or whether the endDate has been moved back in time and revoke existing cards
  • add new tests

Side effects / open questions

  • do we want to update cards if the startDate (in the user entitlements) has been moved to the future?
  • do we want to handle the case, when regionId (in the user entitlements) is updated?

Testing

Full testing is currently blocked, until the app config is ready.
Currently the following can be checked:

  1. Open administration --> login as a koblenz project admin --> go to settings --> create token
  2. Try to import users using curl or Postman
    curl -H "Authorization: Bearer <my_token>" -F file=@import_testdata_local.csv http://0.0.0.0:8000/users/import

CSV content:

regionKey,userHash,startDate,endDate,revoked
07111,"$argon2id$v=19$m=19456,t=2,p=1$cr3lP9IMUKNz4BLfPGlAOHq1z98G5/2tTbhDIko35tY",01.01.2024,01.01.2025,false
  1. Go to http://localhost:3000/ and select Koblenz
  2. Go to http://localhost:3000/erstellen, create a card with a birthday 10.06.2003 and a reference number 123K
  3. Update the CSV file to import users, change revoked to ‘true’ or endDate to past
  4. Import users again --> check in the database that previously created cards (dynamic and static) have been revoked

Resolved issues

Fixes: #1415

@seluianova seluianova force-pushed the 1417-implement-authentication-for-user-data-endpoint branch 4 times, most recently from c6896d1 to db41432 Compare October 14, 2024 11:00
@seluianova seluianova force-pushed the 1415-revoke-existing-cards-via-http-endpoint branch from f9b6150 to 4fd7777 Compare October 14, 2024 11:36
@seluianova seluianova marked this pull request as ready for review October 14, 2024 12:17
@f1sh1918
Copy link
Contributor

Just a general question. Does it make sense to also revoke cards where the DateEnd is in the past.
I think we we agreed on that koblenz revokes cards by setting revoked: true as this is a clean process.
To give another opportunity to revoke cards by changing the EndDate makes it just more complex i think.
I can't remember that there is a reason why we need that

@seluianova
Copy link
Contributor Author

Does it make sense to also revoke cards where the DateEnd is in the past

as discussed, I will remove that

Base automatically changed from 1417-implement-authentication-for-user-data-endpoint to main October 15, 2024 16:26
@ztefanie
Copy link
Member

I updated base branch, so I can fully test it.

@seluianova seluianova linked an issue Oct 17, 2024 that may be closed by this pull request
@f1sh1918
Copy link
Contributor

I updated base branch, so I can fully test it.

I think the removal of the end date check is missing. So not sure if its ready for review

Copy link
Member

@ztefanie ztefanie left a comment

Choose a reason for hiding this comment

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

I tested this and it only partly works as expected:

Use cases:

  1. If I import non revoked user data, then create a card and activate it in the app, then upload a new csv where the card is revoked, the card is revoked in the app and it is displayed properly to the user
  2. If I import non revoked user data, then create a pdf, then upload a new csv where the card is revoked and then try to activate the card in the app, with the pdf previously created, i get the error message "Fehler beim lesen des codes", here should be a message: "Your card was revoked"
  3. it i go to self service (/erstellen) and try to create a card for a revoked user entitlement i get the error "Sie sind scheinbar nicht berechtigt einen KoblenzPass zu erstellen. Bitte prüfen Sie Ihre Eingaben" i think this is missleading, i think the error message should be "You are no longer entitlet for a koblenzpass" or something like thi.

@seluianova
Copy link
Contributor Author

seluianova commented Oct 17, 2024

  1. If I import non revoked user data, then create a pdf, then upload a new csv where the card is revoked and then try to activate the card in the app, with the pdf previously created, i get the error message "Fehler beim lesen des codes", here should be a message: "Your card was revoked"

@ztefanie it's just our current behaviour. if we revoke a card in bayern and the user tries to activate it, he gets the same message. it might be nice to add a separate message for this case, but I think that's a separate task.

Upd. created #1692

  1. it i go to self service (/erstellen) and try to create a card for a revoked user entitlement i get the error "Sie sind scheinbar nicht berechtigt einen KoblenzPass zu erstellen. Bitte prüfen Sie Ihre Eingaben" i think this is missleading, i think the error message should be "You are no longer entitlet for a koblenzpass" or something like thi.

tbh it doesn't sound like a big difference to me. if a user is no longer entitlet, it also means, he is not entitlet. So, I don't find it measleading.
however, if we decide, there is a benifit, I don't mind adding a separate message for that. @f1sh1918 what do you think?

@seluianova
Copy link
Contributor Author

Updated:

  • removed the condition for revoking cards when the user's entitlement endDate is in the past
  • refactored importData() function a bit to reduce complexity, not sure we need even more splitting

@f1sh1918
Copy link
Contributor

f1sh1918 commented Oct 21, 2024

Some thoughts to 3) @seluianova
The "Sie sind scheinbar nicht berechtigt...." was not final and the message will be adjusted to

Wir konnten Ihre Angaben nicht im System finden. Bitte überprüfen Sie Ihre Angaben und versuchen Sie es erneut.

design

Due to this upcoming adjustments it will make sense to throw a different error and show a different message to the user as steffi mentioned
@hauf-toni
So we have another state that should be handled and we need a proper error message for that.
If the user data can be found but the user is no longer entitled.
This should be reflected in the design

@seluianova
Copy link
Contributor Author

The "Sie sind scheinbar nicht berechtigt...." was not final and the message will be adjusted to

Updated the message when the user entitlement is not found. Will add an additional message once provided.
Do we also want to separate errors when the user entitlement is revoked and when it has expired?

@seluianova seluianova force-pushed the 1415-revoke-existing-cards-via-http-endpoint branch from 9425077 to 775f035 Compare October 21, 2024 10:50
@f1sh1918
Copy link
Contributor

i think the expiration info will be fetched from another endpoint when the app was started and there should already be an error message.

@seluianova
Copy link
Contributor Author

seluianova commented Oct 21, 2024

@f1sh1918 you mean we should allow card creation when the user entitlement is expired?
(Currently we return an error in this case when a user tries to create a card)

@f1sh1918
Copy link
Contributor

Ah okay i thought the issue is about what to show if an activated card is expired in the app.

I think we don't need separate errors but this depends on the error message. I think the user does not have to know if its revoked or the entitlement is expired the important thing is that the entitlement is not valid anymore

Copy link
Contributor

@f1sh1918 f1sh1918 left a comment

Choose a reason for hiding this comment

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

Works great. Tested on ios.
Please address the one issue i mentioned

@@ -145,9 +145,9 @@ class CardMutationService {
)
val userHash = Argon2IdHasher.hashKoblenzUserData(user)

val userEntitlements = transaction { UserEntitlementsRepository.findUserEntitlements(userHash.toByteArray()) }
val userEntitlements = transaction { UserEntitlementsRepository.findByUserHash(userHash.toByteArray()) }
if (userEntitlements == null || userEntitlements.revoked || userEntitlements.endDate.isBefore(LocalDate.now())) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we have to throw different exceptions here. At least we should create a TODO and ticket if it will not be done here.
We have to distinguish between the input data does not match the entitlement hash then we throw UserEntitlementNotFoundException()

If revoked or expired i would throw
UserEntitlementExpiredException or sth similar with a separate message

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will need to associate a new exception with some error message.
Do you think Sie sind nicht mehr berechtigt, einen KoblenzPass zu erstellen is fine?

Copy link
Member

@ztefanie ztefanie left a comment

Choose a reason for hiding this comment

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

not tested again

@seluianova seluianova force-pushed the 1415-revoke-existing-cards-via-http-endpoint branch from 184ee6b to a0c605a Compare October 28, 2024 09:57
@seluianova seluianova merged commit 5371268 into main Oct 28, 2024
1 check passed
@seluianova seluianova deleted the 1415-revoke-existing-cards-via-http-endpoint branch October 28, 2024 10:25
@hauf-toni
Copy link

Here is the error display for the third variant that @ztefanie mentioned.
Here is also the updated version (only the text has been changed), which includes the changes from Koblenz.

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.

Revoke existing cards via HTTP endpoint
4 participants