-
Notifications
You must be signed in to change notification settings - Fork 32
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix #3654: jupyterhub moderation requests #4211
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First set of comments. I would like to discuss parse_post.require_sim_type.
b65eed7
to
84a51a4
Compare
sirepo/package_data/auth_role_moderation/moderation_email.jinja
Outdated
Show resolved
Hide resolved
Reminding myself - we need to write a migration for existing users |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
More comments. I didn't finish. LMK when you are ready for another review.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might be good to walk through these changes together. I have particular opinions here that may need more explanations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I backtracked on some decisions and added more feedback. I'm being pedantic, because there are a lot of interesting coding and testing cases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I should help with this. Set up a meeting when you want to work on this.
@robnagler where should we check for guest users to keep them from accessing the moderation apis? |
06a9302
to
3e86c22
Compare
@robnagler ready for another review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is good to go. Is there a migration for users? Do you want to push to alpha?
Yes a migration needs to be written, thanks for the reminder. I'll add that then we can merge. |
Thinking about this more, no migration is needed. User's who are in the approved community pool already have the sim_type_jupyterhublogin role. So, things will continue to work like normal for them. User's who are in the denied list (not actually used anywhere currently but we kept it around) don't have any records in the db so we have no way of finding a uid for them and setting them as denied in user_role_invite_t. So, I think we're good to go. Please confirm and I'll merge. |
b0a1f12
to
2bb8368
Compare
Did you run a check for the denied list? I think we should run this check: validate that only users in valid pools (uspas, etc.) have the role. Any users not in these pools should get role removed. We need to migrate from the community pool to the everybody pool once moderation is released to production. We still need pools for uspas, etc. |
e785b29
to
19e2293
Compare
@robnagler I've written a script (hand/check-jupyter-users/check-jupyter-users.sh) to do the validation. You can run it as is (only does reads currently). I left a TODO with a question at the top. If you could answer as well as do a full review of the script (I'd really like to improve my bash) I'd appreciate it. |
ad20d6b
to
2b6c814
Compare
2b6c814
to
ed15491
Compare
No description provided.