stanfordnmbl / opencap-api

Apache License 2.0
6 stars 6 forks source link

IsOwner permissions - 2FA #183

Closed antoinefalisse closed 2 months ago

antoinefalisse commented 2 months ago

@olehkorkh-planeks what do you think?

We're having an issue where if we patch new session metadata through scripting, we get a permission error if otp_verified = false. otp_verified can be false if someone has never logged in (which isn't a problem) or if they logged in but didn't complete 2fa.

The permission error is is because of IsOwner permissions looks at otp_verified, not just whether the request user == the session object owner. Thus, we get unpredictable scripting behavior for session object permissions based on whether they have successfully done 2fa last time or not. Can you see a reason why the session permissions need to look at otp_verified, vs just looking at if the object owner is the same as the request? I'm hesitant to change this since it is such a general permission.

@suhlrich @AlbertoCasasOrtiz

olehkorkh-planeks commented 2 months ago

Hi @antoinefalisse, I see that this check is a part of the initial commit. I think there is a reason for that for preventing working with API without OTP validation. It could be a real case because the frontend receives the token before the OTP validation (the validation endpoint requires it). Maybe it is not an ideal solution to check the OTP verification in the IsOwner, but it seems workable. I think that we can do the next:

What do you think?

antoinefalisse commented 2 months ago

@suhlrich thoughts?

suhlrich commented 2 months ago

I think I'd rather have an otp verification step in utilsAuth in opencap-core and opencap-processing. If otp_verified is False, we can trigger the code to be sent just like we do for the front end, and have the user enter it into the terminal, and move on.

antoinefalisse commented 2 months ago

I opened issues on core and processing