Mpsk devices #749
Mpsk devices #749
Conversation
added a new Subsection for MPSK Clients for the creating of them #744
added the database model for storing added the needed functions as well as the relation to the user and back Signed-off-by: Alex <[email protected]> #744
changed the name Signed-off-by: Alex <[email protected]>
now enforces name for mpsk client refactoring changed name Signed-off-by: Alex <[email protected]> #744
refactoring and renaming for better usability Signed-off-by: Alex <[email protected]> #744
added the api background for adding mpsk clients and editing them Signed-off-by: Alex <[email protected]>
Signed-off-by: Alex <[email protected]>
added the naming constraint that names can not be empty or just be constructed out of witespaces Signed-off-by: Alex <[email protected]> #744
added tests for the model of mpsk Signed-off-by: Alex <[email protected]> #744
agmes4
force-pushed
the
MPSK_devices
branch
from
5f55f51
to
6b16064
Compare
Co-authored-by: Lukas Juhrich <[email protected]>
Co-authored-by: Lukas Juhrich <[email protected]>
Co-authored-by: Lukas Juhrich <[email protected]>
there was a small error which just checked rather Signed-off-by: Alex <[email protected]> #744
Co-authored-by: Lukas Juhrich <[email protected]>
Co-authored-by: Lukas Juhrich <[email protected]>
Co-authored-by: Lukas Juhrich <[email protected]>
removed the old decorator Signed-off-by: Alex <[email protected]> #744
Signed-off-by: Alex <[email protected]> #744
added the user who deleted the mpsk client as processor Signed-off-by: Alex <[email protected]> #744
|
Please add a certain limit for the number of MPSK devices per user. (e.g. 50) |
|
I will set it to 10 dont think this should be exceeded by anyone |
|
If you choose a limit this low, there may be situations where it is not enough for a user with many devices. |
Generally in favor, but is there any technical limit we should care about? or do the controllers allow arbitrarily many devices? |
Besides the subnet size, there shouldn't be a limit AFAIK. |
added an error indecating the maximum amount of clients was exceeded added a check for the client to exceed the maximum number of mpsks devices Signed-off-by: Alex <[email protected]> #744
checking for amount exceeded error as well as naming error in MPSKS clients Signed-off-by: Alex <[email protected]> #744
added a test for too much mpsks clients added Signed-off-by: Alex <[email protected]> #744
|
but we dont preserve a IP for them so yeah when he has that many devices we have a problem but when he adds so much devices. Or am I missing something? |
|
Anyway I added now the limit of 10 devices we could set it down. |
pycroft/lib/mpsk_client.py
Outdated
| if len(owner.mpsks) >= 10: | ||
| raise AmountExceededError( | ||
| "the limit of added mpsks clients is exceeded", limit=10, actual=len(owner.mpsks) | ||
| ) |
There was a problem hiding this comment.
Contrary as suggested by me in #749 (comment), you introduced the limit in the lib.
This also prevents admins to add more devices.
There was a problem hiding this comment.
This could also be solved by adding an extra parameter to the function that disables the check if needed.
There was a problem hiding this comment.
Yeah but then I can not test that also I dont think any one would use more then 10 but I can do so
There was a problem hiding this comment.
I agree with @ibot3, it is better to have flexibility for the admins. Don't worry about testing it, adding a test for the API can be done later (I will do it).
There was a problem hiding this comment.
thats true but I dont think that these members are not using a router also maybe its a nice way to get active members :)
There was a problem hiding this comment.
But I can also set it higher what do you think would be a good number of devices? 50 Sounds a bit much for me in my opinion
There was a problem hiding this comment.
If the wifi network works as planned in the future, then we don't want that users have their own wifi router, we might even want to prohibit other wifi networks than ours.
However, this is not the case yet.
But our wifi should work in a way that the users don't have the need for another wifi-router.
I would suggest increasing the limit to 30.
There was a problem hiding this comment.
true I now set it to 30 thank
There was a problem hiding this comment.
This could also be solved by adding an extra parameter to the function that disables the check if needed.
Sorry for the confusion, but I just now saw that @ibot3 suggested adding a boolean flag. I was agreeing only with his first message.
As a rule of thumb, boolean flags are a code smell.
Think about it, this flag makes the problem worse: Instead of removing the check from the lib function, it leaves it in, but conditionally. This makes mpks_client_create more complex (e.g., it duplicates the test efforts).
This kind of rate limit is not part of pycrofts business logic, it's an implementation decision from the API. Since the API should depend on the lib, but not vice versa, just the fact that the word api is now part of a lib function is a clear sign that we're entering the depths of hell accidental complexity.
- The API endpoint should check it (when we implement it).
- I also agree with 30, but the fact that we are arguing about the value of a hard-coded number shows that it should be a configurable environment variable.
So for now I would ask you to just remove the check, and we can agree on a pre-configured limit of 30 when the API gets implemented. Edit: The API is already implemented, so just move the check there.
added key arg which is for determining rather the amount of mpsks clients should be validated for a amount Signed-off-by: Alex <[email protected]> #744
added test for clients exceeding admin fixed test for api call Signed-off-by: Alex <[email protected]> #744
changed the creat mpsks so that it now validates for the length of added clients Signed-off-by: Alex <[email protected]> #744
the api is now able to add more clients Signed-off-by: Alex <[email protected]> #744
Co-authored-by: Lukas Juhrich <[email protected]>
Signed-off-by: Alex <[email protected]> #744
|
Looks pretty good to me. Do we want to add a note in Pycroft if the user has a legacy wifi password? That could simplify debugging, but in the end it affects only very few users. |
Added
Resolves #744