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 string prompt to log in with existing username or create an account #12310

Merged

Conversation

LianaHarris360
Copy link
Member

@LianaHarris360 LianaHarris360 commented Jun 18, 2024

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?

SignInInstead.mp4

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 LianaHarris360 added the TAG: user strings Application text that gets translated label Jun 18, 2024
@github-actions github-actions bot added APP: User Re: User app (sign-in, sign-up, user profile, etc.) DEV: frontend SIZE: very small labels Jun 18, 2024
@LianaHarris360 LianaHarris360 added the work-in-progress Not ready for review label Jun 19, 2024
@github-actions github-actions bot added APP: Setup Wizard Re: Setup Wizard (facility import, superuser creation, settings, etc.) SIZE: small labels Jun 19, 2024
@LianaHarris360 LianaHarris360 added TODO: needs review Waiting for review and removed work-in-progress Not ready for review labels Jun 19, 2024
@LianaHarris360 LianaHarris360 requested review from AlexVelezLl and nucleogenesis and removed request for radinamatic June 25, 2024 20:19
Copy link
Member

@rtibbles rtibbles left a comment

Choose a reason for hiding this comment

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

A couple of string things I noticed - looks like the redirect has been updated to go via the wizard state, which seems important per the above!

Copy link
Member

@AlexVelezLl AlexVelezLl left a comment

Choose a reason for hiding this comment

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

LGTM! =D
Just a little non blocker question 👐.

@LianaHarris360 LianaHarris360 requested a review from rtibbles June 27, 2024 14:53
@rtibbles rtibbles dismissed their stale review June 28, 2024 18:44

Changes addressed!

@rtibbles
Copy link
Member

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

@pcenov
Copy link
Member

pcenov commented Jul 1, 2024

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

2024-07-01_16-35-52.mp4

Logs:
Logs.zip

@rtibbles
Copy link
Member

rtibbles commented Jul 1, 2024

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.

@github-actions github-actions bot added the DEV: backend Python, databases, networking, filesystem... label Jul 1, 2024
@LianaHarris360
Copy link
Member Author

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

@pcenov
Copy link
Member

pcenov commented Jul 2, 2024

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

@rtibbles
Copy link
Member

rtibbles commented Jul 2, 2024

@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.

Copy link
Member

@rtibbles rtibbles 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 good, I have manually confirmed the behaviour, and the regression that @pcenov had noted is not extant when both Kolibris are running the updated asset.

@rtibbles rtibbles merged commit c65262f into learningequality:develop Jul 2, 2024
30 checks passed
@pcenov
Copy link
Member

pcenov commented Jul 3, 2024

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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
APP: Setup Wizard Re: Setup Wizard (facility import, superuser creation, settings, etc.) APP: User Re: User app (sign-in, sign-up, user profile, etc.) DEV: backend Python, databases, networking, filesystem... DEV: frontend SIZE: small SIZE: very small TAG: user strings Application text that gets translated TODO: needs review Waiting for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Needs decisions: not ready for assignment] Update the UX in the setup wizard when a username already exists
4 participants