Restrict character sets for attribute values#397
Restrict character sets for attribute values#397Magnus-Kuhn wants to merge 56 commits intorelease/v7from
Conversation
Magnus-Kuhn
added
breaking-change
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Milena-Czierlinski
left a comment
Milena-Czierlinski
left a comment
There was a problem hiding this comment.
Requires additional approval of @stnmtz once we want to merge this PR.
jkoenig134
dismissed
Milena-Czierlinski’s stale review
The base branch was changed.
jkoenig134
removed
the
wip
@Magnus-Kuhn you can add docs now yourself against: nmshd/documentation#307 |
jkoenig134
left a comment
jkoenig134
left a comment
There was a problem hiding this comment.
Seeing what you did with #437 I think we should do all this in the controller and not on serval level. Additionally it wouldn't be a breaking change then.
|
|
||
| @serialize() | ||
| @validate({ max: 100 }) | ||
| @validate({ max: 100, regExp: characterSets.din91379DatatypeC }) |
There was a problem hiding this comment.
this key is only machine read, I don't see a reason why we should limit the charset here.
There was a problem hiding this comment.
My reason for validating everything was that integrators can safely copy attributes into their systems, even if they can't handle all characters
There was a problem hiding this comment.
I don't see someone not being able to "handle" this.
There was a problem hiding this comment.
The reason for creating the norm and making it mandatory was that there were non-conformant IT systems (some that weren't even using UTF8) - see https://www.it-planungsrat.de/fileadmin/beschluesse/2022/Beschluss2022-51_Umsetzung.pdf. I would not be as confident with this.
There was a problem hiding this comment.
Ehm, nope?! The main reason is that we don't want to have ✌🏼😜😜😜🤣😆🥹😅 as a display name or surname.
There was a problem hiding this comment.
Okay, then there has been some misunderstanding
| export class RelationshipAttributeCreationHints extends Serializable implements IRelationshipAttributeCreationHints { | ||
| @serialize() | ||
| @validate({ max: PROPRIETARY_ATTRIBUTE_MAX_TITLE_LENGTH }) | ||
| @validate({ max: PROPRIETARY_ATTRIBUTE_MAX_TITLE_LENGTH, regExp: characterSets.din91379DatatypeC }) |
There was a problem hiding this comment.
why are title and description validated?
There was a problem hiding this comment.
If a query has forbidden characters, then there would be no attribute that fits to the query
There was a problem hiding this comment.
ehm, what? this would require that title and description are validated in the attribute itself..
There was a problem hiding this comment.
They are currently validated in the Proprietary... types
There was a problem hiding this comment.
True, but also there I'd heavily argue that this is necessary.
Doing it in serval validates requests and notifications as well |
But it requires you to duplicate the logic everywhere ... I don't see a reason for doing that. |
Additionally the error message for regex sucks. When doing that in controllers we would have the possibility to return a WAY BETTER error message than |
|
Also generally the regex checks for e.g. Email occur in content - which also has a big error message. Do you suggest moving them all (and maybe the string length checks as well) into the controller? |
I don't know why this has to be a general thing. I see a thing that gets introduced in almost every content class, which is not ok and we should find a solution for that - and I proposed a solution. Projecting this to "but then we should remove all regex validations" is very misplaced here. |
|
nmshd/documentation#309 does documentation, #448 is an alternative approach |
|
Closed in favour of #448 |
4 participants
Readiness checklist