local-it/mitgliederverwaltung
2
3
Fork 0
Code Issues 51 Pull requests 13 Projects 1 Releases Packages 1 Activity Actions

OIDC handling and linking closes #171 #192

Merged
moritz merged 9 commits from feature/oidc_handling into main 2025-11-13 16:36:04 +01:00
Conversation 1 Commits 9 Files changed 21 +3209 -141
moritz commented 2025-11-06 18:42:01 +01:00
Owner
Copy link

Description of the implemented changes

The changes were:

  • Bugfixing
  • New Feature
  • Breaking Change
  • Refactoring

#171

What has been changed?

See docs/oidc-account-linking.md

Definition of Done

Code Quality

  • No new technical depths
  • Linting passed
  • Documentation is added were needed

Accessibility

  • New elements are properly defined with html-tags
  • Colour contrast follows WCAG criteria
  • Aria labels are added when needed
  • Everything is accessible by keyboard
  • Tab-Order is comprehensible
  • All interactive elements have a visible focus

Testing

  • Tests for new code are written
  • All tests pass
  • axe-core dev tools show no critical or major issues
## Description of the implemented changes The changes were: - [x] Bugfixing - [x] New Feature - [ ] Breaking Change - [ ] Refactoring https://git.local-it.org/local-it/mitgliederverwaltung/issues/171 ## What has been changed? See [docs/oidc-account-linking.md](https://git.local-it.org/local-it/mitgliederverwaltung/src/commit/07cb1ab57c68359618228c6614e9770a6f1f1c32/docs/oidc-account-linking.md) ## Definition of Done ### Code Quality - [ ] No new technical depths - [x] Linting passed - [x] Documentation is added were needed ### Accessibility - [ ] New elements are properly defined with html-tags - [ ] Colour contrast follows WCAG criteria - [ ] Aria labels are added when needed - [ ] Everything is accessible by keyboard - [ ] Tab-Order is comprehensible - [ ] All interactive elements have a visible focus ### Testing - [x] Tests for new code are written - [x] All tests pass - [x] axe-core dev tools show no critical or major issues
moritz added 7 commits 2025-11-06 18:42:03 +01:00
refactor and docs
All checks were successful
continuous-integration/drone/push Build is passing
Details
bd79a9b9e1
fix missing translations
All checks were successful
continuous-integration/drone/push Build is passing
Details
a1b0318b1c
moritz changed title from WIP: OIDC handling and linking to WIP: OIDC handling and linking closes #171 2025-11-06 18:42:14 +01:00
moritz force-pushed feature/oidc_handling from a1b0318b1c to 07cb1ab57c 2025-11-06 18:56:35 +01:00 Compare
moritz changed title from WIP: OIDC handling and linking closes #171 to OIDC handling and linking closes #171 2025-11-06 19:02:54 +01:00
requested reviews from simon, carla, rafael 2025-11-06 19:03:18 +01:00
rafael requested changes 2025-11-12 10:53:37 +01:00
lib/accounts/user.ex Outdated
@ -174,0 +188,4 @@
new_email = Map.get(oidc_user_info, "preferred_username")
changeset
|> Ash.Changeset.change_attribute(:oidc_id, oidc_id)
rafael commented 2025-11-12 10:31:39 +01:00
Collaborator
Copy link

Does this do anything? it looks like oidc_id is the same value we're getting out of the changeset a few lines above.

Does this do anything? it looks like `oidc_id` is the same value we're getting out of the changeset a few lines above.
moritz commented 2025-11-13 16:29:44 +01:00
Author
Owner
Copy link

On line 184, oidc_id is taken out of the changeset as an argument, but on line 191 it is set as an attribute. These are two different things in Ash:
Arguments are temporary values that are passed to the action
Attributes are the persisted fields in the data model So the code transforms the argument into an attribute that is stored in the database.

On line 184, oidc_id is taken out of the changeset as an argument, but on line 191 it is set as an attribute. These are two different things in Ash: Arguments are temporary values that are passed to the action Attributes are the persisted fields in the data model So the code transforms the argument into an attribute that is stored in the database.
lib/accounts/user.ex Outdated
@ -174,0 +191,4 @@
|> Ash.Changeset.change_attribute(:oidc_id, oidc_id)
# Update email if it differs from OIDC provider
|> then(fn cs ->
if new_email && to_string(cs.data.email) != new_email do
rafael commented 2025-11-12 10:34:34 +01:00
Collaborator
Copy link

Is the equality check needed? From the ash docs:

Adds a change to the changeset, unless the value matches the existing value.

Is the equality check needed? From the [ash docs](https://hexdocs.pm/ash/Ash.Changeset.html#change_attribute/3): > Adds a change to the changeset, **unless the value matches the existing value.**
moritz marked this conversation as resolved
lib/accounts/user/validations/oidc_email_collision.ex Outdated
@ -0,0 +84,4 @@
end
end
defp handle_existing_user(
rafael commented 2025-11-12 10:46:49 +01:00
Collaborator
Copy link

Removing this function and inlining its code instead would be shorter and could be more readable, even at the cost of an extra level of indentation above.

Removing this function and inlining its code instead would be shorter and could be more readable, even at the cost of an extra level of indentation above.
moritz marked this conversation as resolved
lib/mv_web/live/auth/link_oidc_account_live.ex Outdated
@ -0,0 +25,4 @@
Map.get(session, "oidc_linking_user_info"),
{:ok, user} <- Ash.get(Mv.Accounts.User, user_id) do
# Check if user is passwordless
if passwordless?(user) do
rafael commented 2025-11-12 10:52:40 +01:00
Collaborator
Copy link

Hmm, is there a valid case where a user does not have a password, and also does not have an OIDC id? Intuitively, I'd have assumed this case to result in an error.

Hmm, is there a valid case where a user does not have a password, and also does not have an OIDC id? Intuitively, I'd have assumed this case to result in an error.
moritz commented 2025-11-13 16:27:05 +01:00
Author
Owner
Copy link

It's an edge case but still possible. An admin creates a user without a password so the user can set the password. Then the user login via OIDC. Then the system would get stuck without catching this case.

It's an edge case but still possible. An admin creates a user without a password so the user can set the password. Then the user login via OIDC. Then the system would get stuck without catching this case.
moritz added 1 commit 2025-11-13 16:33:05 +01:00
refactor: small changes from PR review
Some checks reported errors
continuous-integration/drone/push Build was killed
Details
82e2fbb109
moritz force-pushed feature/oidc_handling from 82e2fbb109 to 55fb845855 2025-11-13 16:33:35 +01:00 Compare
moritz merged commit 10e5270273 into main 2025-11-13 16:36:04 +01:00
moritz deleted branch feature/oidc_handling 2025-11-13 16:36:05 +01:00
Sign in to join this conversation.
Reviewers
No reviewers
simon
carla
rafael
Labels
Clear labels   
bug

Something is not working

  
duplicate

This issue or pull request already exists

  
enhancement

New feature

  
help wanted

Need some help

  
high priority

   
invalid

Something is wrong

  
L

5 h

  
low priority

   
M

3 h

  
medium priority

   
needs refinement

   
question

More information is needed

  
S

1 h

  
UX research

   
wontfix

This won't be fixed

No labels
bug
duplicate
enhancement
help wanted
high priority
invalid
L
low priority
M
medium priority
needs refinement
question
S
UX research
wontfix
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
2 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference: local-it/mitgliederverwaltung#192
No description provided.
Delete branch "feature/oidc_handling"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?