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

Credential handling for password-less learner certificate generation #8239

Merged

Conversation

bjester
Copy link
Member

@bjester bjester commented Jul 28, 2021

Summary

  • Fixes URL issue when initiating a sync with remote server
  • Fixes certificate authentication of learner for facility that does not require learners to use passwords
  • Updates sync preparation function to pass through user sync command argument to certificate authentication (@jredrejo)

References

See issues tracked in Notion

Reviewer guidance

  • The server side does not require the learner password-less authentication handling updates for that scenario to be fixed
  • A new Morango integration test ensures Morango passes through the facility credential argument to Kolibri's auth backend:
$ INTEGRATION_TEST=1 pytest -k "test_learner_password" kolibri/core/auth/test/test_morango_integration.py

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

@bjester bjester added the work-in-progress Not ready for review label Jul 28, 2021
@bjester bjester marked this pull request as ready for review July 29, 2021 16:24
@bjester bjester changed the title Credential handling Credential handling for password-less learner certificate generation Jul 29, 2021
@bjester bjester added DEV: backend Python, databases, networking, filesystem... TODO: needs review Waiting for review and removed work-in-progress Not ready for review labels Jul 29, 2021
@bjester bjester added this to the 0.15.0 milestone Jul 29, 2021
# add facility so `FacilityUserBackend` can validate
userargs = {
FacilityUser.USERNAME_FIELD: username,
FACILITY_CREDENTIAL_NAME: dataset_id,
Copy link
Member

Choose a reason for hiding this comment

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

This is being passed as the facility id, but is the dataset_id - is this not facility.dataset_id or is the variable name here just confusing?

return response.url.rstrip("/").replace("api/public/info", "")

# return (scheme, netloc) of url
return urlunparse(parsed_url[:2])
Copy link
Member

Choose a reason for hiding this comment

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

Ah yes, I can see how that caused an issue!

# add facility so `FacilityUserBackend` can validate
userargs = {
FacilityUser.USERNAME_FIELD: username,
FACILITY_CREDENTIAL_KEY: facility_id,
Copy link
Member

Choose a reason for hiding this comment

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

Ahah, yes that answers my question!

@rtibbles rtibbles merged commit 8172f57 into learningequality:release-v0.15.x Jul 29, 2021
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... TODO: needs review Waiting for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants