Skip to content
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

Feide verification #3255

Merged
merged 3 commits into from Aug 9, 2023
Merged

Feide verification #3255

merged 3 commits into from Aug 9, 2023

Conversation

LudvigHz
Copy link
Member

@LudvigHz LudvigHz commented Mar 21, 2023

Fixes ABA-520

TODO

  • Endpoint to get user groups from feide/FS
  • User model updates and logic to get verification status (atm the idea is to only need to verify once, but reverifying to set correct groups would be nice to have)
  • Methods to validate Abakus membership from feide groups, and set correct group memberships etc

@linear
Copy link

linear bot commented Mar 21, 2023

ABA-91 Add feide authentication

As of now we authenticate users using their student mail. This is not ideal since anyone that is a student at ntnu can then theoretically make a profile at abakus.no and get access to all our events.

Instead we should use feide authentication, so we can authenticate not only based on student status, but based on the study a student is enrolled in.

In addition it's important to implement a solution for users who are no longer studying data or komtek and students who are planning on transfering to one of these studies, as it's nice to include them in our linjeforening too (and there's quite some people who fall into this category now that are active in the linjeforening)

Copy link
Member

@ivarnakken ivarnakken left a comment

Choose a reason for hiding this comment

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

Looks clean and good. 💯

What's left to do?

lego/apps/users/models.py Outdated Show resolved Hide resolved
lego/apps/users/models.py Outdated Show resolved Hide resolved
@LudvigHz
Copy link
Member Author

LudvigHz commented Apr 3, 2023

What's left to do?

Most of this PR 😅. I'll update the description with some checkboxes so it's more clear :)

@LudvigHz LudvigHz marked this pull request as ready for review July 29, 2023 14:04
@linear
Copy link

linear bot commented Jul 29, 2023

@LudvigHz
Copy link
Member Author

Slightly more left here to polish it off, but I can take reviews on this now :)

Adds FEIDE verification using OIDC. The user authenticates with our
FEIDE app, and we use their groups API to retrieve the users groups,
including study programmes. This allows us to verify the students
eligibility to become a member in Abakus, and set their grade and
program automatically.
@LudvigHz LudvigHz added new-feature Pull requests that introduce or issues that suggest a new feature priority:high Pull requests that have high priority, and should therefore be prioritized future-was-2-years-ago Pull requests that introduce technologies which should have been "long" ago review-needed Pull requests that need review need-before-2050 Pull requests that have been in progress for far too long labels Jul 31, 2023
@LudvigHz
Copy link
Member Author

A few things left to answer on this:

Unique FEIDE users

Right now, there is no check for the same FEIDE account on multiple users. This means that a single FEIDE user can be used to register several users. This would be quite simple to fix by saving the user ID and un-verifying users if they use the same FEIDE account on another user f.ex.

What to do with current students?

Currently, they keep their grade, but will have is_verified_student() = None. This should mean that any current users can register for events etc. but might be some funky stuff with the verified property (although I don't think it's used anywhere else).

Some options are:

  • Keep it as is. Current students will keep thier grade
  • Use the is_verified property in event registration and inform everyone that they need to re-verify. They will then keep their grade but will need to authenticate with FEIDE to register for events.
  • Finish the grade selection code and remove everyone from their grade. This will mean no changes anywhere else and will ensure everyone will need to re-verify to get their student status back. The downside is that this might be a bit more to implement.

@LudvigHz
Copy link
Member Author

Unique FEIDE users

Right now, there is no check for the same FEIDE account on multiple users. This means that a single FEIDE user can be used to register several users. This would be quite simple to fix by saving the user ID and un-verifying users if they use the same FEIDE account on another user f.ex.

I have now implemented it in such a way that the student_username already used for verification is used again. This time it is retrieved from feide and set on the user. This means that if another user has already taken that username, the verification will fail.

Copy link
Member

@eikhr eikhr left a comment

Choose a reason for hiding this comment

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

lgtm

lego/apps/users/models.py Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Aug 6, 2023

Codecov Report

Patch coverage: 94.04% and project coverage change: -0.14% ⚠️

Comparison is base (2826e4c) 88.29% compared to head (9510962) 88.15%.
Report is 4 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3255      +/-   ##
==========================================
- Coverage   88.29%   88.15%   -0.14%     
==========================================
  Files         662      663       +1     
  Lines       21005    20937      -68     
==========================================
- Hits        18546    18458      -88     
- Misses       2459     2479      +20     
Files Changed Coverage Δ
lego/apps/users/registrations.py 100.00% <ø> (ø)
lego/apps/users/tests/test_models.py 100.00% <ø> (ø)
lego/apps/users/views/oidc.py 83.56% <83.56%> (ø)
lego/apps/users/models.py 95.00% <85.00%> (-0.37%) ⬇️
lego/api/v1.py 100.00% <100.00%> (ø)
lego/apps/users/constants.py 100.00% <100.00%> (ø)
...igrations/0040_user_student_verification_status.py 100.00% <100.00%> (ø)
...ego/apps/users/serializers/student_confirmation.py 71.42% <100.00%> (-28.58%) ⬇️
.../apps/users/tests/test_student_confirmation_api.py 100.00% <100.00%> (ø)
lego/apps/users/validators.py 96.15% <100.00%> (+0.15%) ⬆️

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@LudvigHz LudvigHz merged commit 7e517e9 into master Aug 9, 2023
2 of 3 checks passed
@LudvigHz LudvigHz deleted the feide branch August 9, 2023 11:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
future-was-2-years-ago Pull requests that introduce technologies which should have been "long" ago need-before-2050 Pull requests that have been in progress for far too long new-feature Pull requests that introduce or issues that suggest a new feature priority:high Pull requests that have high priority, and should therefore be prioritized review-needed Pull requests that need review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants