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

Other entity login #2143

Merged
merged 30 commits into from
Feb 8, 2024
Merged

Other entity login #2143

merged 30 commits into from
Feb 8, 2024

Conversation

TheSlimvReal
Copy link
Collaborator

@TheSlimvReal TheSlimvReal commented Dec 22, 2023

closes: #2132 #1685 #1513

  • UserSecurityComponent is entity agnostic
  • generalize $currentUser default value -> assign full entity ID and support "foreign" entity types in display/edit entity components -> This comes with the limitation that our default filter (e.g. notes of child) will not include this "foreign" references (e.g. when a child has been added as author of a note)
  • generalize Todo completed by
  • generalize Todo and roll call filter for current user by still looking in same property but check with full and short ID match to also support "foreign" entities (maybe we just do Refactor getId() function #1526 and skip this)
  • fix list paginator to also work with other logged in entitites -> store in local storage for now
  • Make username (exact_username attribute in keycloak) access token claim optional and use user ID instead for local DB names
  • Use human readable name (not exact_username) claim for offline user selection
  • Adjust Keycloak email message -> fix: Improved Keycloak email content ndb-setup#28

Copy link

Deployed to https://pr-2143.aam-digital.net/

Copy link
Member

@sleidig sleidig left a comment

Choose a reason for hiding this comment

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

Code looks nice 👍
I guess this required the getId() PR to be merged first?

Tested

  • pagination
  • todo completion

not tested

  • user security component with Keycloak BE
  • todo completion with non-user user entity

  • Profile (menu bottom) shows an error in demo mode: undefined (reading 'loadUserInfo')
  • demo mode SecurityComponent still queries keycloak (and fails) - should we prevent that?
  • complete todo shows error in details popup and related-todos list: Cannot read properties of undefined (reading 'toString')

# Conflicts:
#	src/app/core/basic-datatypes/entity/display-entity/display-entity.component.ts
#	src/app/core/common-components/entity-form/entity-form.service.spec.ts
#	src/app/core/common-components/entity-select/entity-select.component.spec.ts
#	src/app/core/common-components/entity-select/entity-select.component.ts
#	src/app/core/entity/default-datatype/view.directive.ts
#	src/app/core/session/session-service/session-manager.service.ts
#	src/app/features/todos/todo-completion/display-todo-completion/display-todo-completion.component.html
#	src/app/features/todos/todo-completion/display-todo-completion/display-todo-completion.component.spec.ts
#	src/app/features/todos/todo-completion/display-todo-completion/display-todo-completion.component.ts
@TheSlimvReal TheSlimvReal marked this pull request as ready for review February 6, 2024 13:18
@TheSlimvReal TheSlimvReal requested a review from sleidig February 6, 2024 13:18
@TheSlimvReal
Copy link
Collaborator Author

@sleidig I did not change anything about the failing Keycloak requests in the demo mode yet. I am not sure what we actually want there. Someone added the account manager role to the demo admin user which enables the component. If we just remove this role, the errors disappear. Or do we want to showcase something there in the demo mode?

@sleidig
Copy link
Member

sleidig commented Feb 6, 2024

@sleidig I did not change anything about the failing Keycloak requests in the demo mode yet. I am not sure what we actually want there. Someone added the account manager role to the demo admin user which enables the component. If we just remove this role, the errors disappear. Or do we want to showcase something there in the demo mode?

I thought we've linked the user list to that role, but apparently it is using admin_app. So what you are saying makes sense - can't think of a need for account_manager in demo mode.

Copy link
Member

@sleidig sleidig left a comment

Choose a reason for hiding this comment

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

Further Testing:

  • user profile (bottom left):
    • the username field gets overwritten by my Lastpass - although I guess it is disabled? (maybe something for another day ...)
    • can we add a link to the Entity the "User profile" (i.e. entity) this is represented as in our system? (maybe simply with display-entity? / edit-entity)
  • creating new user accounts:
    • the email template says "Your email has been linked to the Aam Digital user: User:Aadi". This will for most cases now be very cryptic - and otherwise might confuse people about what to put into the login form. Shall we take out that sentence and just say "An account for the Aam Digital system has been created for your email. By confirming your account below, you can set a password and then log in using this email as your username."?
    • new user is linked and using correct entity to prefill notes
    • config the UserSecurity component for child details and creating user account from there works
  • tasks & notes (with pre-filled current user's entity)
    • works for notes + tasks with User and Child entity, as expected
    • works without errors if no entity for the user account
    • task completion works with any entity type

: await this.entityMapperService.load<E>(type, id);

if (!entity) {
throw Error(`Entity not found`);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
throw Error(`Entity not found`);
throw Error(`Entity ${id} not found`);

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I have added the ID logging to the surrounding function.

@TheSlimvReal
Copy link
Collaborator Author

TheSlimvReal commented Feb 7, 2024

@sleidig For LastPass there are two options. I recommend checking "Don't overwrite fields that are already filled" ("Account" > "Extensions Settings" > "General" at the bottom), which will also disable this at other locations (e.g. Keycloak UI).
There is also the option to explicitly tell LastPass to ignore this field

@TheSlimvReal
Copy link
Collaborator Author

@sleidig Thanks for the review.
Please see my comment above regarding LastPass. I have also adjusted the Keycloak templates (Aam-Digital/ndb-setup#28). Please have a look at the UI or the linked entity and feel free to adjust this.

@TheSlimvReal TheSlimvReal linked an issue Feb 7, 2024 that may be closed by this pull request
@TheSlimvReal TheSlimvReal merged commit 081a23f into master Feb 8, 2024
6 of 7 checks passed
@TheSlimvReal TheSlimvReal deleted the other_entity_login branch February 8, 2024 09:40
@aam-digital-ci
Copy link
Collaborator

🎉 This PR is included in version 3.32.0-master.1 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

@aam-digital-ci aam-digital-ci added the released on @master managed by CI (semantic-release) label Feb 8, 2024
@aam-digital-ci
Copy link
Collaborator

🎉 This PR is included in version 3.32.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

@aam-digital-ci aam-digital-ci added the released managed by CI (semantic-release) label Feb 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
released on @master managed by CI (semantic-release) released managed by CI (semantic-release)
Projects
None yet
3 participants