#582 - Improvement in guards #1891
Conversation
dheepak-aot
changed the title
dheepak-aot
requested review from
andrewsignori-aot,
ann-aot,
sh16011993,
guru-aot,
andrepestana-aot and
JasonCTang
| } | ||
| return true; | ||
| } | ||
| // If the user is not an admin, check if the route is allowed for non admin user. |
There was a problem hiding this comment.
I know that we only have 2 types of institution user for now but if we add a new one, we'll need to come here and change the comment. Maybe a more generic comment here would suit it better.
| @@ -93,7 +93,10 @@ export const institutionRoutes: Array<RouteRecordRaw> = [ | |||
| }, | |||
| meta: { | |||
| clientType: ClientIdType.Institution, | |||
| userTypes: [InstitutionUserTypes.admin, InstitutionUserTypes.user], | |||
| institutionUserTypes: [ | |||
There was a problem hiding this comment.
We could have a constant for the institutionUserTypes as it repeats many times.
| /** | ||
| * Route meta properties. | ||
| */ | ||
| interface RouteMeta { |
There was a problem hiding this comment.
it will be like a common meta interface, right? just wondering can have derived interface from a base interface for each client type when required, because last 3 are only for institution
There was a problem hiding this comment.
I also would like to have separate interfaces for different client types. But unfortunately RouteMeta is the interface that the router's meta is typed to and we are suppose to use only one single interface.
Are you are saying that for the sake of readability if we should create like this? if so do not see a benefit of creating it as it(RouteMetaInstitution) could never be used anywhere.
interface RouteMetaInstitution {
institutionUserTypes?: InstitutionUserTypes[];
allowOnlyLegalSigningAuthority?: boolean;
allowOnlyBCPublic?: boolean;
}
interface RouteMeta extends RouteMetaInstitution {
clientType: ClientIdType;
requiresAuth?: boolean;
}
There was a problem hiding this comment.
Does it accepts only an interface or can it be a type like interface1 | interface2 | interface3?
There was a problem hiding this comment.
Only interface AFAIK.
| // Define error handling on router error. | ||
| router.onError((error: unknown) => { | ||
| console.error(error); | ||
| throw error; |
There was a problem hiding this comment.
In our discussion we were saying that we don't want the router to stuck in case of an error right, instead return something
There was a problem hiding this comment.
As discussed, I wrongly assumed that the catch block in the previous code had the re-direction, but it was not.
So just kept the code inside catch as is and added to on error. @andrewsignori-aot @andrepestana-aot @guru-aot @sh16011993 fyi.
There was a problem hiding this comment.
So, do we need it at all? I am assuming that an error will be logged either way, right?
There was a problem hiding this comment.
Then I can remove the this for now and added it when required if that is ok.
| } | ||
|
|
||
| /** | ||
| * Validate the access of user to the given route. |
sources/packages/web/src/composables/institution/useInstitutionAuth.ts
Outdated
Show resolved
Hide resolved
sources/packages/web/src/composables/institution/useInstitutionState.ts
Outdated
Show resolved
Hide resolved
| const institutionUserDetails = store.getters[ | ||
| "institution/myDetails" | ||
| ] as UserStateForStore; | ||
| const authorizations = |
There was a problem hiding this comment.
just for my info why store.state.institution.authorizationsState are replaced
| @@ -8,7 +8,7 @@ | |||
| class="navigation-btn float-left" | |||
| > | |||
| <v-btn | |||
| v-if="isAuthenticated" | |||
| v-if="isAuthenticatedInstitutionUser" | |||
There was a problem hiding this comment.
Thanks for the isAuthenticatedInstitutionUser 👍
|
Kudos, SonarCloud Quality Gate passed!
|
Backend Unit Tests Coverage Report
|
E2E Workflow Workers Coverage Report
|
E2E Queue Consumers Coverage Report
|
E2E SIMS API Coverage Report
|
| setInstitutionSetupUser( | ||
| context: ActionContext<InstitutionLocationState, RootState>, | ||
| ): void { | ||
| context.commit("setInstitutionSetupUser", true); |
Improvement in Guards
The following improvements have been implemented to the Guards.
Defining the interface to route meta to have it typed.
Refactoring the logic of validating institution user access to the institution routes. According the updated logic, the institution user types must be specified in the route and missing to do so will reject the access for all user types.
Route meta property
allowOnlyBCPubliccreated for routes to be allowed only for BCPublic institutions. (Implementation of this logic is not part of the ticket.)Route meta property
allowOnlyLegalSigningAuthorityis added and is used to authorize only user with role legal signing authority to access the routes withallowOnlyLegalSigningAuthorityset to true.useInstitutionAuthrefactored to use the store getters and added additional properties.isAuthenticatedInstitutionUsercreated inuseInstitutionAuthto include the verification of isActive status of the user. And due to which the following nav buttons does not appear during create institutions.As per the Vue documentation, Global before guards are replaced with Global Resolve guards.
UserStateForStoreas the interface properties were not in sync with data stored in state.