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

Vue: authentication JWT #1759

Closed
wants to merge 6 commits into from
Closed

Vue: authentication JWT #1759

wants to merge 6 commits into from

Conversation

matthieuRioual
Copy link
Contributor

@matthieuRioual matthieuRioual commented May 17, 2022

Add authentication progress to vue application through pinia storage.

A login page has been created as a guard route.

Once connected you can access application single page and check your identity or ever log out.

Before adding jwt to vue generated application, make sur you added pinia or it gonna throw an error.

You can test it on this branch with credentials "admin, admin"

@codecov
Copy link

codecov bot commented May 17, 2022

Codecov Report

Merging #1759 (2475994) into main (de9ba2f) will not change coverage.
The diff coverage is 100.00%.

❗ Current head 2475994 differs from pull request most recent head 9ec4442. Consider uploading reports for the commit 9ec4442 to get more accurate results

@@             Coverage Diff              @@
##                main     #1759    +/-   ##
============================================
  Coverage     100.00%   100.00%            
- Complexity      2179      2226    +47     
============================================
  Files            453       458     +5     
  Lines           7903      8067   +164     
  Branches         178       179     +1     
============================================
+ Hits            7903      8067   +164     
Impacted Files Coverage Δ
...rity/jwt/application/VueJwtApplicationService.java 100.00% <100.00%> (ø)
...nerator/client/vue/security/jwt/domain/VueJwt.java 100.00% <100.00%> (ø)
...t/vue/security/jwt/domain/VueJwtDomainService.java 100.00% <100.00%> (ø)
...infrastructure/config/VueJwtBeanConfiguration.java 100.00% <100.00%> (ø)
...wt/infrastructure/primary/rest/VueJwtResource.java 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update de9ba2f...9ec4442. Read the comment docs.

@pascalgrimaud
Copy link
Member

I edited your initial description to link to the original ticket

Comment on lines 26 to 35
assertFileExist(project, "src/main/webapp/app/common/domain/Login.ts");
assertFileExist(project, "src/main/webapp/app/common/primary/login/index.ts");
assertFileExist(project, "src/main/webapp/app/common/primary/login/Login.component.ts");
assertFileExist(project, "src/main/webapp/app/common/primary/login/Login.html");
assertFileExist(project, "src/main/webapp/app/common/primary/login/Login.vue");
assertFileExist(project, "src/main/webapp/app/common/secondary/LoginDTO.ts");
assertFileExist(project, "src/main/webapp/app/common/domain/AuthenticationService.ts");
assertFileExist(project, "src/main/webapp/app/common/domain/JWTStoreService.ts");
assertFileExist(project, "src/main/webapp/app/common/domain/Logger.ts");
assertFileExist(project, "src/main/webapp/app/common/domain/Message.ts");
Copy link
Member

Choose a reason for hiding this comment

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

you can define a common variable, using Constants.MAIN_WEBAPP + app/common


@Test
void shouldAddVueJwt() {
Project project = tmpProjectWithPackageJsonPinia();
Copy link
Member

Choose a reason for hiding this comment

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

do we need a package.json with pinia here, as the next service will add pinia ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since the main method for adding jwt checks if pinia is present int the package json, i had no choice but to add pinia in the file for testing or it would throw an exception because pinia is missing.


private VueJwt() {}

public static final String PACKAGE_JSON = "package.json";
Copy link
Member

Choose a reason for hiding this comment

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

already exist in Constants, no need to redefine it here

import { User } from '@/common/domain/User';
import { Router } from 'vue-router';


Copy link
Member

Choose a reason for hiding this comment

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

prettier: should remove this line

@@ -0,0 +1,26 @@
<div id="welcome">
Copy link
Member

Choose a reason for hiding this comment

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

same, don't understand this page, as there is already a home page, generated by vue/core

@@ -0,0 +1,10 @@
import { Logger } from '@/common/domain/Logger';
Copy link
Member

Choose a reason for hiding this comment

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

maybe it should be included by default in vue/core, instead of this PR ?

@@ -0,0 +1,13 @@
import { Login } from '@/common/domain/Login';

export interface LoginDTO {
Copy link
Member

Choose a reason for hiding this comment

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

as said previously, the name is a little bit strange

@@ -9,13 +11,50 @@ const routes = [
{
path: '/app',
name: 'App',
component: AppVue,
{{#auth}}
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we should do this, as the router already exist in the project, with specific routes from the user. By doing this, you'll erase all the route.

Instead, we should insert the new routes, in an existing file

@pascalgrimaud pascalgrimaud marked this pull request as draft May 18, 2022 10:18
@pascalgrimaud pascalgrimaud changed the title Jwt vue secondary Vue: authentication JWT May 18, 2022
@matthieuRioual matthieuRioual marked this pull request as ready for review May 23, 2022 02:21
@pascalgrimaud pascalgrimaud marked this pull request as draft May 31, 2022 15:11
@@ -0,0 +1,60 @@
import { AppVue } from '@/common/primary/app';
Copy link
Member

Choose a reason for hiding this comment

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

this file should not replace the existing one

Comment on lines 43 to 52
"\t{",
"\tpath: '/login',",
"\tname: 'Login',",
"\tcomponent: LoginVue,",
"\t},",
"\t{",
"\tpath: '/',",
"\tname: 'Homepage',",
"\tcomponent: AppVue,",
"\t},"
Copy link
Member

Choose a reason for hiding this comment

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

don't use tab (\t), use space instead

return List.of("AuthenticationRepository.spec.ts", "restLogin.spec.ts", "UserDTO.spec.ts");
}

public static Map<String, String> appComponent() {
Copy link
Member

Choose a reason for hiding this comment

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

same, don't use tab, prefer using space instead

Then, it's better to use text block

Comment on lines 124 to 137
"\t<div v-if=\"isAuthenticated\">",
"\t\t<p>You are connected as </p>",
"\t\t<div v-if=\"user.username == ''\">",
"\t\t\t<button id=\"identify\" @click.prevent=\"onConnect\">click to see</button>",
"\t\t</div>",
"\t\t<div v-else>",
"\t\t\t<p>{{user.username}}</p>",
"\t\t\t<button id=\"logout\" @click.prevent=\"onLogout\">click to logout</button>",
"\t\t</div>",
"\t</div>",
"\t<div v-else>",
"\t\t<p>You are not connected</p>",
"\t\t<router-link to=\"/login\">Login</router-link>",
"\t</div>",
Copy link
Member

Choose a reason for hiding this comment

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

same about \t

@@ -0,0 +1,90 @@
import { shallowMount, VueWrapper } from '@vue/test-utils';
Copy link
Member

Choose a reason for hiding this comment

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

still don't understand this file, as the App.spec.ts already exists. This context should not generate a new one

@pascalgrimaud
Copy link
Member

another comment: this feature is not tested in the CI, you need to update the generate.sh file

@matthieuRioual matthieuRioual marked this pull request as ready for review June 1, 2022 19:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Vue: account with authentication JWT
2 participants