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

Merged
merged 7 commits into from
Dec 5, 2024

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.

@ztefanie ztefanie requested a review from seluianova December 4, 2024 09:21
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

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 fine! Nicely done!

One thing i think we have to handle somehow
If i send an empty string for any value like forename i get a server response of

{
  "title": "Server Error",
  "status": 500,
  "type": "https://javalin.io/documentation#internalservererrorresponse",
  "details": {}
}

And the api throws internally

Caused by: app.ehrenamtskarte.backend.exception.webservice.exceptions.InvalidJsonException: Exception for GraphQL error INVALID_JSON was thrown.

Just added some minor comments about code style in react

@ztefanie ztefanie closed this Dec 4, 2024
@ztefanie ztefanie reopened this Dec 4, 2024
@ztefanie
Copy link
Member Author

ztefanie commented Dec 4, 2024

One thing i think we have to handle somehow
If i send an empty string for any value like forename i get a server response of

This will be handled here, right? #1816
or is there anything additional you noticed that needs to be fixed? I added your example to the issue description.

@f1sh1918
Copy link
Contributor

f1sh1918 commented Dec 4, 2024

One thing i think we have to handle somehow
If i send an empty string for any value like forename i get a server response of

This will be handled here, right? #1816 or is there anything additional you noticed that needs to be fixed? I added your example to the issue description.

Sorry i just realized that there is already an issue! I think thats fine

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.

Tested on chrome. Works fine. 👍

I'm okay with merging it even i think the small proposals for the quick indicator would improve the code

@ztefanie
Copy link
Member Author

ztefanie commented Dec 4, 2024

I'm okay with merging it even i think the small proposals for the quick indicator would improve the code

Added your suggestion

Copy link
Contributor

@seluianova seluianova left a comment

Choose a reason for hiding this comment

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

lgtm! 👍

@seluianova
Copy link
Contributor

seluianova commented Dec 4, 2024

💭 maybe would be nice to have more or less similar style for the icons. I assume we will have more of them in the future. shall we create a ui/ux ticket for that?
image

@ztefanie
Copy link
Member Author

ztefanie commented Dec 5, 2024

💭 maybe would be nice to have more or less similar style for the icons. I assume we will have more of them in the future. shall we create a ui/ux ticket for that?

Good point. Yes may be an option. I will create ticket.

@ztefanie ztefanie merged commit 9b3f04b into main Dec 5, 2024
2 checks passed
@ztefanie ztefanie deleted the 1790-verein360-endpoint-protection branch December 5, 2024 07:43
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