feature: non-humanities users handling and access control #548

tymees · 2023-10-05T15:33:37Z

Fixes #509

Adds a new Faculty model, using a M2M to User. I did end up also needing a proxy model, as SAML can only use processors defined on the User model. I did not switch Django itself to said model, only SAML is using it.

The faculty model is set-up to extend further; one idea I have is automatically redirecting FSW users to their committee. But I'm lazy, so this can become our next backlog issue :)

I decided to only ever add faculties to a user, instead of constantly replacing the entire set with the current SAML-provided set. It's debatable if that's a good idea, I mostly did it to give us the ability to ad-hoc add people to certain faculties.

Also implemented were 2 (well, 2 with 2 variants) ways to check 'membership'. A straight copy of GroupsRequiredMixin for views, and a simple utility function for other uses. Both come in a 'generic' variant, and a preset 'humanities' variant.
It's a bit overkill, but might come in handy if we even need to support REBO as well.

A banner was added to warn non-humanities users, and the archive was locked behind that mixin (and the menu items also dissapear themselves)

The last change is a controversial one; I moved the constants from settings.py into their own file. Mostly because I dislike our monolithic settings files, as it makes deployment annoying (in terms of keeping settings in sync). In the future, I want the 'dev settings' and the 'deployed settings' use the same files more, making the file we need to configure on the server smaller and thus easier to keep in sync.

tymees · 2023-10-05T15:36:16Z

Testing:

Have your SAML configured; update any existing local SAML settings with the new attribute map
Create or update an user in the dev IdP, to have a 'Organisational Unit Name' entry with as name 'Faculteit Geesteswetenschappen'
- Also, maybe add some other users with other faculties. (I was testing with 'Faculteit Pythonwetenschappen' at some point)
- The hosted dev IdP for acc/test servers has a 'fsw' user btw
Make sure your dev IdP provides that field as 'uuLegacyDepartment'[1]
Log in using said users
See if it all works ;)

[1] Open your SP settings, your attribute map should look like this:

{
  "uid": "uuShortID",
  "mail": "mail",
  "givenName": "givenName",
  "sn": "uuPrefixedSn",
  "organizationalUnitName": "uuLegacyDepartment"
}

miggol

Very nice work. Tested with a local IdP with verious random faculties. The proxy user solution is elegant and easy to understand.

I think moving the constants out of settings.py is an improvement. However, I'm not as much a fan of the humanities faculty being so hard-coded. It feels fragile considering all the effort that's gone into making this framework django-admin-friendly.

I've made a PR to this branch with my suggestion. Using an "is_privileged" flag on faculties gives us more flexibility to change access on the fly. Though I admit the implications aren't substantial either way.

My other comments are also minor so I'm happily approving this branch! Feel free to entertain my suggestion or not

miggol · 2023-10-10T20:23:07Z

fetc/saml_settings.example.py

@@ -36,8 +36,7 @@
    'mail':     ('email',),
    'givenName': ('first_name',),
    'uuPrefixedSn':  ('last_name',),
-    # TODO: create an attribute on the user model to store this value
-    # 'uuLegacyDepartment':  (),
+    'uuLegacyDepartment':  ('process_faculty', ),


This method ended up being called process_faculties(), best to update it here as well ;)

miggol · 2023-10-11T10:02:16Z

main/admin.py

+
+
+class CustomUserAdmin(UserAdmin):
+    inlines = [FacultyInline]


miggol · 2023-10-11T10:06:04Z

main/templates/main/index.html

+                </p>
+                <p class="mb-0">
+                    {% blocktrans trimmed %}
+                        Als je vermoed dat dit incorrect is, neem dan contact op met <a href="mailto:[email protected]">[email protected]</a>.


vermoed should be vermoedt

miggol · 2023-10-11T11:22:12Z

Oh, just one final meta comment! I noticed this PR has one commit touching 16 files at once. I don't always step through commits when reviewing, but it does make the process easier sometimes.

When I'm ready to make a PR, I like to reset my index and re-commit my changes in related chunks. This also has some QA benefits for me. Just another suggestion.

tymees · 2023-10-11T11:36:12Z

I'll address the comments sometime this week;

Regarding the fragility of the hardcoded constant; Yeah, I agree that it isn't ideal.

Your solution would make things better in that sense, but will also make things slightly less fine-grained. We can only say 'this faculty is slightly more special than others'.

I admit, at the moment this does not matter, as we only have the one faculty we care about atm. However, I'd like to propose an alternative:

Add a new field internal_name (or something) that's a choices-limited field (atm the choices would probably be 'humanities' and 'other'). Then, instead of the constant we can refer to the model-specific choices where needed. It would make the whole thing django-admin-friendly, and still allow easy extension to other 'special' faculties in the future.

Oh, just one final meta comment! I noticed this PR has one commit touching 16 files at once. I don't always step through commits when reviewing, but it does make the process easier sometimes.

When I'm ready to make a PR, I like to reset my index and re-commit my changes in related chunks. This also has some QA benefits for me. Just another suggestion.

That's a valid point. I'll try to make more individual commits; for example, this PR could've been split it up in 3 commits. (Adding the base faculty logic, adding the warning and adding the access-controls to the views).

However, in general I do prefer larger commits in the long-term, as it makes it easier to understand using git-blame why something was changed. (Otherwise, you end up picking through multiple commits sometimes). Something I find myself doing on a semi-regular basis. (Also, it makes the 'move acc to prod' PR's clearer about what was actually done)

Maybe a good 'having your cake and eat it too' solution would be to start using 'squash-merges' in GH? That way, all the 'address PR comments' commits would also be merged into the one commit. Those make stuff harder to understand as wel...
(And GH also adds the PR number by default to the squashed commit message, so you can also easily find the discussion for those changes, all in one commit)

Okay, maybe I'm getting ahead of the meeting next week now...

EdoStorm96

I've also tested this successfully. In general this implementation seems clear to me, so I also approve. Nice work, Ty!

I also like moving the constants to a separate file.

miggol · 2023-10-11T14:32:06Z

Maybe a good 'having your cake and eat it too' solution would be to start using 'squash-merges' in GH? That way, all the 'address PR comments' commits would also be merged into the one commit. Those make stuff harder to understand as wel...

Didn't we already switch to this? For me, squash and merge is the default option when merging PR's. I'm a fan.

Add a new field internal_name (or something) that's a choices-limited field (atm the choices would probably be 'humanities' and 'other'). Then, instead of the constant we can refer to the model-specific choices where needed. It would make the whole thing django-admin-friendly, and still allow easy extension to other 'special' faculties in the future.

That would be even better: strikes a good balance between future-proofing and overengineering. The overengineering side of me kind of wants a way to allow individual users into the archive without polluting their profile with a fake faculty. But that's really overthinking things at this point.

tymees · 2023-10-11T14:48:45Z

Didn't we already switch to this? For me, squash and merge is the default option when merging PR's. I'm a fan.

Eh, I don't recall we've ever agreed to do this. (I didn't even know it was an option GH offered until Xander pointed it out to me).
But in that case, great :)

That would be even better: strikes a good balance between future-proofing and overengineering. The overengineering side of me kind of wants a way to allow individual users into the archive without polluting their profile with a fake faculty. But that's really overthinking things at this point.

Awesome, I'll implement this with the other comments.

And to ease your concerns about pollution;
I don't consider manually adding someone to the Humanities faculty is wrong; this should only happen for researchers working on humanities research and, by extension, are actually working for humanities. SAP might disagree, but well, that is bound to happen from time to time.

tymees · 2023-10-16T09:57:39Z

Okay, added the new internal name thing and I think I addressed your comments!

miggol

Approved!

I wish there was a way to use the inline ManyToMany widget as the default widget on both sides of the relationship. (is there?)

The "hold ctrl" SelectMultiple widget always bugs me in how one misclick while editing a model could, for example, wipe all membership of a faculty.

I think the CheckBoxSelectMultiple as the widget for users in FacultyAdmin could be a mild improvement. But I don't know how bad it gets with a massive amount of options. If you agree, feel free to set it to checkboxes without re-requesting review.

Otherwise still totally good for a merge. On to SAML in prod!

tymees · 2023-10-16T16:11:30Z

Yeah, the default select-multiple is pretty horrible.

I've changed the Faculty admin to use the same widget used for groups in the user admin; which is pretty easy to do turns out lol.
Honestly, I think this is the superior widget for M2M relations. Sadly, it will only work on the model defining the M2M (for some reason, there is no underlying DB reason). So we can't use it on the user admin page for faculties.

Luckily, the biggest fault of the widget we use there now is that it is very ugly....

feature: non-humanities users handling and access control

3d7d5cb

tymees requested review from miggol and EdoStorm96 October 5, 2023 15:37

miggol approved these changes Oct 11, 2023

View reviewed changes

EdoStorm96 approved these changes Oct 11, 2023

View reviewed changes

tymees added 3 commits October 16, 2023 11:55

feature: use an internal name for faculty checks

2ce19b2

fix: typo

64498f1

fix: use correct method name

4d52d87

tymees requested a review from miggol October 16, 2023 09:57

fix: add missing no-op migration to please the CI gods

ffc6bed

miggol approved these changes Oct 16, 2023

View reviewed changes

feature: use nicer m2m widget for faculty admin

59cb68c

tymees merged commit c0d7940 into develop Oct 16, 2023
2 checks passed

tymees deleted the feature/faculty-access-control branch October 16, 2023 16:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feature: non-humanities users handling and access control #548

feature: non-humanities users handling and access control #548

tymees commented Oct 5, 2023 •

edited

Loading

tymees commented Oct 5, 2023 •

edited

Loading

miggol left a comment

miggol Oct 10, 2023

miggol Oct 11, 2023

miggol Oct 11, 2023

miggol commented Oct 11, 2023

tymees commented Oct 11, 2023 •

edited

Loading

EdoStorm96 left a comment

miggol commented Oct 11, 2023

tymees commented Oct 11, 2023

tymees commented Oct 16, 2023

miggol left a comment

tymees commented Oct 16, 2023 •

edited

Loading

feature: non-humanities users handling and access control #548

feature: non-humanities users handling and access control #548

Conversation

tymees commented Oct 5, 2023 • edited Loading

tymees commented Oct 5, 2023 • edited Loading

miggol left a comment

Choose a reason for hiding this comment

miggol Oct 10, 2023

Choose a reason for hiding this comment

miggol Oct 11, 2023

Choose a reason for hiding this comment

miggol Oct 11, 2023

Choose a reason for hiding this comment

miggol commented Oct 11, 2023

tymees commented Oct 11, 2023 • edited Loading

EdoStorm96 left a comment

Choose a reason for hiding this comment

miggol commented Oct 11, 2023

tymees commented Oct 11, 2023

tymees commented Oct 16, 2023

miggol left a comment

Choose a reason for hiding this comment

tymees commented Oct 16, 2023 • edited Loading

tymees commented Oct 5, 2023 •

edited

Loading

tymees commented Oct 5, 2023 •

edited

Loading

tymees commented Oct 11, 2023 •

edited

Loading

tymees commented Oct 16, 2023 •

edited

Loading