Skip to content

fix: Don't allow group chair to change group parent#6496

Merged
rjsparks merged 6 commits intoietf-tools:mainfrom
pselkirk:fix-6037
Nov 5, 2023
Merged

fix: Don't allow group chair to change group parent#6496
rjsparks merged 6 commits intoietf-tools:mainfrom
pselkirk:fix-6037

Conversation

@pselkirk
Copy link
Copy Markdown
Contributor

Fixes #6037

@codecov
Copy link
Copy Markdown

codecov Bot commented Oct 18, 2023

Codecov Report

Merging #6496 (0593b8f) into main (f6a2d8c) will increase coverage by 0.14%.
Report is 52 commits behind head on main.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #6496      +/-   ##
==========================================
+ Coverage   88.70%   88.85%   +0.14%     
==========================================
  Files         290      284       -6     
  Lines       40440    40262     -178     
==========================================
- Hits        35874    35774     -100     
+ Misses       4566     4488      -78     
Files Coverage Δ
ietf/doc/views_conflict_review.py 97.05% <100.00%> (-0.07%) ⬇️
ietf/group/forms.py 91.51% <100.00%> (+0.09%) ⬆️
ietf/group/views.py 90.29% <100.00%> (+0.04%) ⬆️

... and 19 files with indirect coverage changes

Comment thread ietf/group/tests_info.py Outdated
Comment on lines +959 to +962
login_testing_unauthorized(self, 'mars-chair', url)
r = self.client.get(url)
self.assertEqual(r.status_code, 302)
self.assertEqual(len(r.content), 0)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not clear what the intent of this test block is? The view that would only edit the parent field should probably just refuse entry?

What about testing the form that allows editing the whole group (when you don't pass in field to the view). - I would expect a test that shows that the parent field isn't present on that form.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the chair, /edit/parent/ behaves exactly like /edit/xyzzy/ - it renders an edit page with no editable fields, just a Submit button and a Back button, which both lead back to the about page.

However, I bolluxed the user creation, and what's actually going on there is a redirection to the login page. 🤦

But yeah, I can test the whole group form.

Copy link
Copy Markdown
Member

@rjsparks rjsparks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for adjusting the tests. This is close to ready but has one more change that should be made.

Right now, in this branch, /group/rswg/edit/parent presents the following:

image

There is risk of unexpected happening with submitting a null-form to the current code (which hunts for fields).

I suspect the current code would also let someone provide the parent field manually here (or on the full edit form) in a post and the value would be honored.

Please test those posts (attempting to change parent as a chair should result in no change).

I think the form view at /group/rswg/edit/parent should 403 unless you have the roles you've already identified for showing the field.

@pselkirk
Copy link
Copy Markdown
Contributor Author

It turns out that the form is accepted (form.is_valid==True), but form.cleaned_data only contains the fields that are shown to the user - in the case of editing just the parent field, it's an empty dict.

@rjsparks
Copy link
Copy Markdown
Member

I still think it's the right time to change the view behavior to 403 if there's no field to edit. (such as with your xyzzy example).

@pselkirk
Copy link
Copy Markdown
Contributor Author

Unit testing on github is failing with 'ietf/name/models.py:104: error: Need type annotation for "next_states" [var-annotated]'. Did something change in the settings?

This tracks a change that was made directly in the production database
to fix the immediate cause of ietf-tools#6037.
@rjsparks rjsparks merged commit b5ee9ec into ietf-tools:main Nov 5, 2023
@github-actions github-actions Bot locked as resolved and limited conversation to collaborators Nov 9, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Group edit form destroys group parent for rswg chairs

2 participants