-
Notifications
You must be signed in to change notification settings - Fork 0
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
Enable authentication with suomi.fi credentials #19
Conversation
5710812
to
c12a878
Compare
8acdb83
to
784b38f
Compare
784b38f
to
fecd6a0
Compare
4 roles / levels have been defined:
Role 1 will access to the whole site (collection management, global statistics and system administration) The local customized lane management does not exist yet. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The role 3 (3. Paikalliskirjaston työntekijä/rekisteröijä - Local librarian user / registering agent) should be replaced with the role 4 (4. Paikalliskirjaston työntekijä/työryhmä - Local librarian user / work group).
Role 2 and role 4 won't have access to the system administration menu.
Role 4 won't have access to statistics.
if ekirjasto_role == "orgadmin": | ||
return AdminRole.SYSTEM_ADMIN | ||
elif ekirjasto_role == "admin": | ||
return AdminRole.SITEWIDE_LIBRARY_MANAGER |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The local library manager can access the collection menu (aka left menu) with localized statistics and localized custom lanes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As for Role 2 (aka manager-all
, aka admin
), users with this role can currently edit the library settings. So based on your comment we should add some logic to disallow that for the circulation manager-all
role?
edit: based on discussion offline I will look into whether it makes more sense to add new roles or use extra rules for existing roles.
edit: The extra authorization rules are now in place. Decided to keep using the existing roles instead of creating new ones for E-kirjasto.
api/admin/controller/sign_in.py
Outdated
return AdminRole.SYSTEM_ADMIN | ||
elif ekirjasto_role == "admin": | ||
return AdminRole.SITEWIDE_LIBRARY_MANAGER | ||
elif ekirjasto_role == "registrant": |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The registrant or registering agent does not have access to the UI. The role should be replaced by the equivalent to "Paikalliskirjaston työntekijä/työryhmä - Local librarian user / work group". It would have access to the left menu as well without the localized statistics, and with localized custom lanes (when available).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I'm understanding this correctly, we might be missing a role for the work group on the E-kirjasto side? See the updated table in the pull request description.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Access for registrant now removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, exactly, a role is missing for the work group.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
New librarian role now included
fecd6a0
to
725bd98
Compare
.outerjoin( | ||
AdminCredential | ||
) # Finland, don't return externally authenticated admins | ||
.filter(AdminCredential.admin_id.is_(None)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also realized that we probably don't want to show password authenticated users to externally authenticated users.
if ekirjasto_role == "orgadmin": | ||
return AdminRole.SYSTEM_ADMIN | ||
elif ekirjasto_role == "admin": | ||
return AdminRole.SITEWIDE_LIBRARY_MANAGER |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As for Role 2 (aka manager-all
, aka admin
), users with this role can currently edit the library settings. So based on your comment we should add some logic to disallow that for the circulation manager-all
role?
edit: based on discussion offline I will look into whether it makes more sense to add new roles or use extra rules for existing roles.
edit: The extra authorization rules are now in place. Decided to keep using the existing roles instead of creating new ones for E-kirjasto.
6b88bbd
to
5daf1a1
Compare
Value will be either "password" or "external" depending on how the user authenticated themselves.
5daf1a1
to
0bcc317
Compare
Description
Implement admin authentication via external E-kirjasto authentication application.
This implementation assumes that:
Role mapping
sysadmin
system-admin
orgadmin
manager-all
admin
registrant
librarian-all
librarian
customer
Restrictions for externally authenticated users
The following extra authorization rules are put in place for E-kirjasto (i.e. externally authenticated) users.
/admin/individual_admins
response. Thus, under the admin UI “System Configuration” panel, E-kirjasto users are not listed.Other implementation details:
The E-kirjasto user ID is used to populate the
email
field for admin data model. This ID will also be shown in the admin UI in place of the e-mail. We have the user'sgiven_name
andfamily_name
on the backend so with some extra implementation would be possible to show that in the UI instead of the ID.The original email+password based login is still available. Also, the initial admin UI "setting up" mode is still there for creating an initial system-admin user without relying on E-kirjasto API.
New environment value
ADMIN_EKIRJASTO_AUTHENTICATION_URL
added for configuring the admin authentication URL.In the test environment, a
state
parameter can be used to test different types of admin users. The available values are::T0000
: Failure:T0001
: Success, Turku 853, Unverified, customer:T0002
: Success, Turku 853, Verified, customer:T0003
: Success, Helsinki 091, Verified, customer:T0004
: Success, Empty municipality, Verified, customer:T0005
: Success, Helsinki 091, Verified, customer, born 2010:T0006
: Success, Helsinki 091, Verified, registrant:T0007
: Success, Helsinki 091, Verified, admin:T0008
: Success, Helsinki 091, Verified, orgadmin:T0009
: Success, Turku 853, Verified, sysadmin:T0010
: Success, Turku 853, Verified, librarianThis state can be passed to circulation admin sign in page with a query param. For example, use http://localhost:6500/admin/sign_in?state=:T0008 to log in as a system admin.
Motivation and Context
https://jira.lingsoft.fi/browse/SIMPLYE-216 - Enable the authentication of librarians to the administration console with their suomi.fi credentials
Checklist