Closed jacdavi closed 1 year ago
Hey @jacdavi thanks for working on this! I haven't had a chance to review yet (and figured it's not ready yet since it's still draft), but I did want to mention that I've started work on a branch that parses role definitions client-side (like is done server-side) so we can get rid of the hard-coded roles client-side. It's been a hot minute, but I can dust off that branch and see if I can get it finished out and tested.
Ooh that would be awesome, even just as a reference point. I got all of this done before realizing the UI was going to take significant changes.
@jacdavi sorry I wasn't able to get you what I had started on before you started working on your latest commits (client-side role checking). I'll take a look at your code ASAP and see if it makes sense to still work the stuff I had done into what you've done.
Do you have a deadline for this? If so, I can try to prioritize, so let me know!
Thanks again!
No problem! The closest thing I found was this: https://github.com/eric-c-wood/phenix/tree/rbac-enhancements. I briefly looked over that, but haven't followed it too closely.
No hard deadline, but ideally done by mid-July. So no need to rush. I just posted it as a draft for awareness.
@jacdavi this branch is the one I was talking about. I only had it locally until now.
@jacdavi this is looking awesome!
Thanks! Should be getting close. I mostly need to test the frontend at this point to make sure all the new role checks work as expected (which might take a while).
@activeshadow I still have a bit more testing to do, but I think it's ready for a 2nd pair of eyes. Let me know if you have any questions or changes.
One potential issue area: I'm not sure how this interacts with the proxy authentication, I've only tried the default authentication
@jacdavi I've done my best to keep authentication and authorization completely separate, so I'm pretty sure this should work with proxy authentication as well. It's easy enough to test.
Thanks again for all your work on this! It will be a great improvement. I've been reviewing as you push new commits but I'll try to do a thorough review and some testing ASAP.
@jacdavi I tested and fixed an issue with proxy auth. I also did a quick code perusal and didn't see anything glaring. I did not do any actual testing of different roles, as I assume you've tested that pretty extensively. If there's anything specific you want me to test I can do that. Otherwise, let me know when you think it's ready and I can squash and merge (unless you want to do the squashing).
As an aside, I really need to make a little time to get rid of the protobuf crap... originally I had planned on using gRPC between the UI client and the server, but ended up bailing on that, so now the protobuf stuff just gets in the way... ugh.
@jacdavi based on the consistent failures in your GitHub Actions for this branch, it looks like we need to test upgrading the version of Node being used. Just FYI.
@jacdavi based on the consistent failures in your GitHub Actions for this branch, it looks like we need to test upgrading the version of Node being used. Just FYI.
I'll look into that. I see it's from adding babel-jest
for the unit tests.
I also want to do a little more testing with weird roles. My main concern is that I messed up one of the roleAllowed
calls and something will be incorrectly hidden from the UI.
Or is it just a matter of upgrading the node version in the dockerfile. Looks like we're using 12.18.3
there but the readme says 14.2
. It seems like those should be aligned?
Or is it just a matter of upgrading the node version in the dockerfile. Looks like we're using
12.18.3
there but the readme says14.2
. It seems like those should be aligned?
Which README says 14.2
? Regardless, yes I think we simply try upgrading the version of node in the Dockerfile to one that babel-jest
is okay with and see if the UI still builds correctly.
I also want to do a little more testing with weird roles. My main concern is that I messed up one of the
roleAllowed
calls and something will be incorrectly hidden from the UI.
Makes sense, but definitely time consuming. Might be better to let this be "crowd tested" naturally and wait for folks to raise an issue... :joy:
Ok, upgrading the node version in docker worked fine. For the readme, I just meant the main project one.
I'll wrap up testing tomorrow and let you know. Thanks!
@activeshadow ok I'm ready for the merge! Feel free to rebase/squash/etc. as needed. Thanks again for your help!
@activeshadow ok I'm ready for the merge! Feel free to rebase/squash/etc. as needed. Thanks again for your help!
@jacdavi can do! Can it wait until next Wednesday? If not I'll try to get it done tonight.
@activeshadow ok I'm ready for the merge! Feel free to rebase/squash/etc. as needed. Thanks again for your help!
@jacdavi can do! Can it wait until next Wednesday? If not I'll try to get it done tonight.
Yep. No rush!
This PR should enable the use of custom roles when creating new users. We have a need to load a role in that admins can then utilize to create new users (in our case we want a "vm admin" type of role). However, I noticed that in many places the pre-loaded roles are hard-coded as the only options.
CHANGES:
role.go#Allowed
<exp>_<vm>
resource names with<exp>/<vm>
in calls toRole.Allowed
TODO:
adminUser
orexperimentUser
to determine what to show. These are being replaced with calls toroleAllowed()
with the exact values needed