Ask for a profile's permissions instead of letting the database refuse them - #736
Merged
Merged
Conversation
…e them The permissions are the profile — the column holding them is NOT NULL — but the API read them as optional. A caller who left them out got the database's integrity error back, a 500 saying nothing about which parameter was missing, where every other missing parameter on this endpoint answers with a 400 naming the problem. Required on create and on edit both. Dropping them from an edit would have blanked the permissions of a profile that people are already assigned to, which is worse than the create case: it takes access away from everybody holding it. The test that recorded the old integrity error now asserts the refusal, and the edit case gains one of its own.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The bug
A profile is its permissions, and the column holding them is
NOT NULL— but the API read them as optional (getParamString('profile'), no required flag).So a caller who left them out got the database's integrity error back: a 500 saying nothing about which parameter was missing, on an endpoint where every other missing parameter answers with a 400 that names the problem.
The fix
Required on create and edit both.
The edit case is the worse of the two: dropping the permissions from an edit would have blanked them on a profile that people are already assigned to — taking access away from everybody holding it, rather than merely failing to create something.
Testing
The test that recorded the old integrity error as the behaviour now asserts the refusal, and the edit case gains one of its own.
Integration: 12 green in the class. Unit: 6 green on the profile controllers. PHPStan clean.
This was one of the two findings I had left alone as "not defects" — on a second look, an endpoint answering 500-with-a-database-message where it has a 400 available is a defect, not a design choice. The other one (the user endpoints having no help class, so a missing parameter says "Wrong parameters" with an empty detail) is next.