uhd-urz / elAPI

An extensible API client for eLabFTW
GNU Affero General Public License v3.0
5 stars 0 forks source link

nice-to-have warn/notice/fail on wrong permission #24

Closed alexander-haller closed 2 months ago

alexander-haller commented 5 months ago

Purely as a future bonus:

I think we already discussed this at some point.

We could check if any action has the required api-key rights to do this. (Mainly sysadmin/admin and r/w comes to mind)

Bill team does not validate if it has sysadmin key and does nothing if not (without error)

alexander-haller commented 5 months ago

In GitLab by @mhxion on Mar 27, 2024, 17:27

I am glad you brought this up, because our PermissionValidator seems to be more buggy than I thought, and it totally slipped my mind! We initially discussed this validation in our old repository.

bill-teams should already be using PermissionValidator to validate the API key. I just reproduced your case, and this is another bug I didn't notice before! PermissionValidator isn't sending the error to STDERR and only to the log file. You should be able to see error CRITICAL:permission.py: Requesting user doesn't have elabftw 'sysadmin' permission to be able to access the resource. on the log file.

I will look into why it's not sending to STDERR. Great find!

alexander-haller commented 5 months ago

In GitLab by @mhxion on Mar 27, 2024, 17:31

I wanted to show PermissionValidator in our workshop presentation. But the just the day before, I found out that PermissionValidator was invalidating an API token even when the API token had the right access which is the other bug. I forgot to open an issue later!

I never did enough testing with PermissionValidator, of course it'd come back to bite us :/

alexander-haller commented 5 months ago

In GitLab by @mhxion on Mar 27, 2024, 17:44

I will look into why it's not sending to STDERR. Great find!

Turns out the context manager where you see a semi-fancy Validating... loader in the beginning after you run bill-teams is only allowing printing STDOUT, and not STDERR. The fix should be really easy in this case :)

alexander-haller commented 5 months ago

In GitLab by @mhxion on Mar 28, 2024, 24:37

Fixed in !37. Though there's nothing much to it to discuss, we can still discuss tomorrow.