kolibri icon indicating copy to clipboard operation
kolibri copied to clipboard

Add string prompt to log in with existing username or create an account

Open LianaHarris360 opened this issue 1 year ago • 5 comments

Summary

This pull request introduces the necessary string to prompt user to sign in, which improves the user interface when inputting an existing username.

Added String: Sign in instead?

https://github.com/learningequality/kolibri/assets/46411498/f051f98a-1220-47c6-b7d5-59890bb30d4a

Note: This pull request introduces changes to the component UserCredentialsForm which is used within the components SelectSuperAdminAccountForm and LodJoinFacility.

However these changes are only necessary for LodJoinFacility. The Sign in instead? link only displays if errorsCaught includes the error USERNAME_ALREADY_EXISTS, which is set in the component LodJoinFacility but does not get set in SelectSuperAdminAccountForm.

References

Closes #11882

Reviewer guidance

  1. Initiate a new Kolibri setup by creating a user account to add to an existing facility.
  2. Input a username that already exists within the facility.

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

LianaHarris360 avatar Jun 18 '24 20:06 LianaHarris360

Looks like all code changes are approved - so @pcenov and @radinamatic take it away!

rtibbles avatar Jun 28 '24 18:06 rtibbles

Hi @LianaHarris360 for some reason I am always getting the following error in the console when attempting to create a new user regardless of whether that user already exists or doesn't exist:

SyntaxError: Unexpected token '<', " <!DOCTYPE "... is not valid JSON

And the following request response: Response.txt

https://github.com/learningequality/kolibri/assets/79847249/997be89b-6ca8-4799-afa4-dde8d596baba

Logs: Logs.zip

pcenov avatar Jul 01 '24 13:07 pcenov

My guess here is that the csrf_exempt decorator and the csrf_protect decorator are not quite working properly together: https://github.com/learningequality/kolibri/blob/develop/kolibri/core/auth/api.py#L807

To fix this, I would suggest we tweak the inheritance slightly - to something like this:

class BaseSignUpViewSet(viewsets.GenericViewSet, CreateModelMixin):

    serializer_class = FacilityUserSerializer

    def check_can_signup(self, serializer):
        facility = serializer.validated_data["facility"]
        if (
            not facility.dataset.learner_can_sign_up
            or not facility.dataset.full_facility_import
        ):
            raise PermissionDenied("Cannot sign up to this facility")

    def perform_create(self, serializer):
        self.check_can_signup(serializer)
        serializer.save()
        data = serializer.validated_data
        authenticated_user = authenticate(
            username=data["username"],
            password=data["password"],
            facility=data["facility"],
        )
        login(self.request, authenticated_user)


@method_decorator(csrf_protect, name="dispatch")
class SignUpViewSet(BaseSignUpViewSet):
    pass

@method_decorator(csrf_exempt, name="dispatch")
class PublicSignUpViewSet(BaseSignUpViewSet):
    """
    Identical to the SignUpViewset except that it does not login the user.
    This endpoint is intended to allow a FacilityUser in a different facility
    on another device to be cloned into a facility on this device, to facilitate
    moving a user from one facility to another.

    It also allows for historic serializer classes in the case that we
    make an update to our implementation, and we want to keep the API stable.
    """

    legacy_serializer_classes = []

    def create(self, request, *args, **kwargs):
        exception = None
        serializer_kwargs = dict(data=request.data)
        serializer_kwargs.setdefault("context", self.get_serializer_context())
        for serializer_class in [
            self.serializer_class
        ] + self.legacy_serializer_classes:
            serializer = serializer_class(**serializer_kwargs)
            try:
                serializer.is_valid(raise_exception=True)
                break
            except Exception as e:
                exception = e
        if exception:
            raise exception
        self.perform_create(serializer)
        headers = self.get_success_headers(serializer.data)
        return Response(
            serializer.data, status=status.HTTP_201_CREATED, headers=headers
        )

    def perform_create(self, serializer):
        self.check_can_signup(serializer)
        serializer.save()

That should ensure that the public sign up viewset is definitely not CSRF protected.

rtibbles avatar Jul 01 '24 14:07 rtibbles

Hi @pcenov could you check to see if you are still experiencing that error?

LianaHarris360 avatar Jul 01 '24 17:07 LianaHarris360

Hi @LianaHarris360 - yes, I'm still getting the same error.

pcenov avatar Jul 02 '24 14:07 pcenov

@pcenov were you testing with the asset from this PR for both sides of the learner creation? I tested with the asset from this PR for both the LOD that was creating a user and the server it was creating it on, and I had no issues. The fix I suggested @LianaHarris360 apply here was needed on the server side, rather than locally.

rtibbles avatar Jul 02 '24 23:07 rtibbles

Yeah, I was using the asset from this PR on both sides but as I was in a hurry might have tried to create the user on one of the other servers that I was running... Anyways - after carefully checking today I confirm that this is fixed indeed, thanks @LianaHarris360!

pcenov avatar Jul 03 '24 08:07 pcenov