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

1790: Verein360 endpoint protection #1811

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

ztefanie
Copy link
Member

@ztefanie ztefanie commented Dec 3, 2024

Short description

Finalize the verein360 endpoint

Proposed changes

  • protect the applications endpoint correctly
  • show verein360 in digitale druckerei

Side effects

  • May break the normal application flow

Testing

Create an api token
create applications with and without the api token
create applications with and without the pre-verified flag set
create applications with one or multiple organizations

Resolved issues

Fixes: #1790 #1625

@ztefanie ztefanie force-pushed the 1790-verein360-endpoint-protection branch from 8924111 to 62e1ae5 Compare December 3, 2024 11:31
@steffenkleinle steffenkleinle changed the title 1790 verein360 endpoint protection 1790: Verein360 endpoint protection Dec 3, 2024
@seluianova
Copy link
Contributor

seluianova commented Dec 3, 2024

❓ Not directly related to the current PR but I have to ask somewhere:
The endpoint documentation contains some requirements to the acceptable values, but some of them we don't check.
For example:
"applicationType": "FIRST_APPLICATION", // Must be FIRST_APPLICATION
But I can submit RENEWAL_APPLICATION, and it will be accepted.
And for some values we have checks, but we return a 500 error with no meaningful description.
For example, if I send "givenInformationIsCorrectAndComplete": false, // Must be true
So I wonder if we actually need some user-friendly error handling here? (as a separate task maybe)

@seluianova
Copy link
Contributor

seluianova commented Dec 3, 2024

❓ and one more general question:
when verein360 application is submitted, the emails to the organisation ("Bestätigung notwendig") and to the user ("Antrag erfolgreich eingereicht") are sent, but I feel like it's not actually needed, isn't it? At least when isAlreadyVerified: true

@f1sh1918
Copy link
Contributor

f1sh1918 commented Dec 3, 2024

❓ and one more general question: when verein360 application is submitted, the emails to the organisation ("Bestätigung notwendig") and to the user ("Antrag erfolgreich eingereicht") are sent, but I feel like it's not actually needed, isn't it? At least when isAlreadyVerified: true

Yes i think we decided on not sending mails in this case. For organizations it makes no sense and for the users we would have to adjust the texts because they not really submitted the application

@ztefanie
Copy link
Member Author

ztefanie commented Dec 4, 2024

❓ and one more general question: when verein360 application is submitted, the emails to the organisation ("Bestätigung notwendig") and to the user ("Antrag erfolgreich eingereicht") are sent, but I feel like it's not actually needed, isn't it? At least when isAlreadyVerified: true

You are right, this was a bug. I fixed it.

@ztefanie
Copy link
Member Author

ztefanie commented Dec 4, 2024

❓ Not directly related to the current PR but I have to ask somewhere: The endpoint documentation contains some requirements to the acceptable values, but some of them we don't check. For example: "applicationType": "FIRST_APPLICATION", // Must be FIRST_APPLICATION But I can submit RENEWAL_APPLICATION, and it will be accepted. And for some values we have checks, but we return a 500 error with no meaningful description. For example, if I send "givenInformationIsCorrectAndComplete": false, // Must be true So I wonder if we actually need some user-friendly error handling here? (as a separate task maybe)

Ok good point. I would split this:

  1. I created a new issue for general validation for this endpoint: Input validation for application endpoint #1816
  2. Validation that is specific for verein360 pre-verified requests: Will add this to this PR.

Base automatically changed from 1791-create-application-verification-automatically-for-verein360 to main December 4, 2024 11:53
@seluianova
Copy link
Contributor

pls update the branch, I will re-test all together :)

# Conflicts:
#	backend/src/main/kotlin/app/ehrenamtskarte/backend/application/webservice/EakApplicationMutationService.kt
@ztefanie
Copy link
Member Author

ztefanie commented Dec 4, 2024

pls update the branch, I will re-test all together :)

done

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.

Verein360 application data endpoint protection Show Verein360 icon in application overview
3 participants