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

JWT Cookie Authentication #437

Merged
merged 98 commits into from
Sep 22, 2020
Merged

JWT Cookie Authentication #437

merged 98 commits into from
Sep 22, 2020

Conversation

bastihav
Copy link
Collaborator

@bastihav bastihav commented Sep 7, 2020

Description

Implements #186, #289

I would like to wait for the test branch before merging and testing this.
Should not influence any production code though.

To run this, webpack will need to spin up a reverse proxy with TLS encryption. Use the following command to generate a localhost certificate (browser will tell you that it is untrusted)

openssl req -x509 -nodes -days 1024 -newkey rsa:2048 -keyout certs/localhost.key -out certs/localhost.crt -extensions 'v3_req' -subj '/C=DE/ST=NRW/L=Paderborn/CN=localhost'

Caution: highly WIP. Currently using different certs to debug pipeline:

      - name: Create CA certificate
        run: openssl req -x509 -newkey rsa:4096 -days 365 -nodes -keyout certs/ca-key.pem -out certs/ca-cert.pem -subj '/C=DE/ST=NRW/L=Paderborn/CN=ca-localhost'
      - name: Create certificate request
        run: openssl req -newkey rsa:4096 -nodes -keyout certs/server-key.pem -out certs/server-req.pem -subj '/C=DE/ST=NRW/L=Paderborn/CN=localhost'
      - name: Sign certificate request
        run: openssl x509 -req -in certs/server-req.pem -days 60 -CA certs/ca-cert.pem -CAkey certs/ca-key.pem -CAcreateserial -out certs/server-cert.pem

Reason for this PR

Changes in this PR

  • remove login data from store, change authentication to use httpOnly cookies, add new login and logout methods

Type of change (remove all that don't apply)

  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

  • Manually tested all use cases this could alter

Test Configuration:

  • OS: Windows

  • Browser: Chrome

  • Frontend: (remove all that don't apply)

    • Development build
  • Backend: (remove all that don't apply)

    • deployed experimental build

Checklist: (remove all that don't apply)

  • I added corresponding E2E tests (especially for bugfixes)
  • I added corresponding unit tests (if API code)
  • I have performed a self-review of my own code
  • My changes generate no new linting warnings or console warnings
  • I have updated the package.json version if this is a new release candidate (remove is it is not)
  • I have updated the CHANGELOG.md to include any changes made in this PR (add WIP to the top, if there is none already)
  • I have changed the API endpoint back to development

Copy link

@janniclas janniclas left a comment

Choose a reason for hiding this comment

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

I just added a few general remarks and thoughts to the code, which came up when I read it. These are meant for u as feedback of what I thought when reading it, so feel free to do with these comments as u see fit.
In general I think the code quality is very high and I didn't see any obvious bugs.

Few further remarks:

  • on the About page the html tag should probably use the rel attribute (https://web.dev/external-anchors-use-rel-noopener/)

  • if u are loged in, open another tab, visit the same page, logout on one of these pages, the other isn't redirected to the login page. The information isn't loaded anymore so that's fine but I would expect a redirect. Also on the Settings page the change password component is still displayed and can actualy be used to relogin by providing a password which isn't expected behaviour I believe.

  • lot's of unused imports, maybe clean that up at some point :)

src/api/AuthenticationHelper.ts Outdated Show resolved Hide resolved
src/api/AuthenticationHelper.ts Outdated Show resolved Hide resolved
src/api/AuthenticationManagement.ts Outdated Show resolved Hide resolved
src/api/AuthenticationManagement.ts Show resolved Hide resolved
src/api/Common.ts Show resolved Hide resolved
src/api/CourseManagement.ts Outdated Show resolved Hide resolved
tests/e2e/helpers/NavigationHelper.ts Outdated Show resolved Hide resolved
vue.config.js Show resolved Hide resolved
vue.config.js Show resolved Hide resolved
@bastihav bastihav merged commit d05266b into develop Sep 22, 2020
@bastihav bastihav deleted the feature/jwt branch September 22, 2020 10:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants