search

cesium-ml / baselayer

Fully customizable (scientific, compute-intensive) web application template
http://cesium-ml.org/baselayer/
31 stars 18 forks source link

Add expiration_date to Users #233

Closed kmshin1397 closed 3 years ago

kmshin1397 commented 3 years ago

Also changed the is_active function for Users, which currently is just returning True no matter what, so that it serves as a check for expired users. We don't seem to be using this function anywhere, but doesn't hurt to have it be more of a real function in case we use it later. This will be accompanied by a PR on SkyPortal to actually use the new column for user expiration functionality (setting expired users to view only automatically). Part of addressing fritz-marshal/fritz#180

Plus black formatting.

pep8speaks commented 3 years ago

Hello @kmshin1397! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

Line 1271:80: E501 line too long (108 > 79 characters)

Comment last updated at 2021-06-30 18:34:08 UTC
acrellin commented 3 years ago

This all looks good, but one note for the SP PR: it feels more intuitive to have a user expiration date and the active/inactive flag correspond to deactivating the account altogether rather than just making them view only. But I guess that'll have to be decided by the PIs.

kmshin1397 commented 3 years ago

This all looks good, but one note for the SP PR: it feels more intuitive to have a user expiration date and the active/inactive flag correspond to deactivating the account altogether rather than just making them view only. But I guess that'll have to be decided by the PIs.

Ah yeah, that makes sense and we can discuss that further. I think the view only way was the easier path of implementation, since I imagine completely deactivating accounts will mean implementing a new "Deactivated accounts" table for the user management page, maybe blocking inactive accounts from logging in, canceling their API tokens, etc. I just saw less steps to simply setting them to view only but definitely wiling to change directions here.

kmshin1397 commented 3 years ago

Instead of adding another flag column, I just used the existing User.is_active() function mentioned above to use to guard against expired users doing anything on the API. This guard logic has been incorporated into access.py

stefanv commented 3 years ago

@kmshin1397 Sorry, I may not have been clear: Your implementation was correct, we just needed to switch the code on the old one. I.e., 401 if the token is not recognized and 403 if the token is valid (user is recognized) but access is forbidden.

kmshin1397 commented 3 years ago

Ah right okay yes I see the difference between the two now. Now it should be fixed

stefanv commented 3 years ago

Thanks, Kyung!