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

Add passwordless login support for the current os user in an app context #11754

Merged

Conversation

rtibbles
Copy link
Member

@rtibbles rtibbles commented Jan 19, 2024

Summary

  • A created os user does not have a password set
  • When you log out of that user, it is not then possible to log back in without creating a password for that user
  • This PR rectifies this by using the retained auth_token in the app context to reauthenticate as the relevant os user

References

Fixes #11717

Reviewer guidance

Run the app devserver.
Open the initialize link.
See that you are logged in automatically.
Log out.
Try to log back in again as the 'os user' user.
Succeed.
Rejoice.


Testing checklist

  • Contributor has fully tested the PR manually
  • If there are any front-end changes, before/after screenshots are included
  • Critical user journeys are covered by Gherkin stories
  • Critical and brittle code paths are covered by unit tests

PR process

  • PR has the correct target branch and milestone
  • PR has 'needs review' or 'work-in-progress' label
  • If PR is ready for review, a reviewer has been added. (Don't use 'Assignees')
  • If this is an important user-facing change, PR or related issue has a 'changelog' label
  • If this includes an internal dependency change, a link to the diff is provided

Reviewer checklist

  • Automated test coverage is satisfactory
  • PR is fully functional
  • PR has been tested for accessibility regressions
  • External dependency files were updated if necessary (yarn and pip)
  • Documentation is updated
  • Contributor is in AUTHORS.md

@rtibbles rtibbles added the TODO: needs review Waiting for review label Jan 19, 2024
@github-actions github-actions bot added DEV: backend Python, databases, networking, filesystem... SIZE: small labels Jan 19, 2024
@marcellamaki marcellamaki self-requested a review January 23, 2024 21:00
@marcellamaki marcellamaki added the P0 - critical Priority: Release blocker or regression label Jan 23, 2024
user = None
if interface.enabled and valid_app_key_on_request(request):
# If we are in app context, then try to get the automatically created OS User
# if it matches the username, without needing a password.
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure how the os_user stuff actually is supposed to be working, but this is just to check that there is an operating system user (presumably the one who is currently logged in/the active os user of the device), with the same name as an existing kolibri user. And if so, just proceed using that, otherwise do normal kolibri authentication?

But, does Kolibri use some other sort of credential (like a password or token) in the os_authentication? I'm asking because I'm wondering if there would be a scenario where there is a os user and a kolibri user with the same username, but some other part of the authentication goes wrong (i.e. OS user resets their password?). It's very hypothetical

Copy link
Member Author

Choose a reason for hiding this comment

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

It's not super hypothetical, as due to syncing, you can have multiple facility users created in a facility with the same username.

The os_user look up, however, is not actually based on the username, but the auth token that is encoded in APP_AUTH_TOKEN_COOKIE_NAME, so all we are doing here is attempting to see if there is an os user with an active auth token, and if the username of the FacilityUser that resolves to matches the username sent to the backend in the API request. In this case, and this case only, we side step the usual authentication procedure, because this user is 'pre-authenticated' by virtue of the app context providing the auth token.

Copy link
Member

@marcellamaki marcellamaki left a comment

Choose a reason for hiding this comment

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

This looks good, and manual QA confirms it works. There is one slightly wonky intermediary state that one can get into in a very particular set of circumstances where the password field shows up briefly before the os login proceeds, but I think that's not blocking, and I'll open a follow up issue for it and then link it here. (see below)

os-user-sign-in.mp4

Follow up issue: #11777

@rtibbles rtibbles merged commit 947e936 into learningequality:release-v0.16.x Jan 23, 2024
34 checks passed
@rtibbles rtibbles deleted the no_password_no_problem branch January 23, 2024 22:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
DEV: backend Python, databases, networking, filesystem... P0 - critical Priority: Release blocker or regression SIZE: small TODO: needs review Waiting for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Manage os user at login/log out
2 participants