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

Validates client config #1116

Open
wants to merge 25 commits into
base: feature/distributed-demo
Choose a base branch
from

Conversation

atomicgamedeveloper
Copy link
Contributor

Copied from previous PR #1083:

Title

Validates client config.

Fixes #658

Type of Change

  • New feature
  • Bug fix
  • Documentation update
  • Refactoring
  • Security patch
  • UI/UX improvement

Description

This PR will provide a route to validate the client config file. A comprehensive, manual one for developers, and another one that automatically redirects users in case their config is incorrectly setup or URLs are unreachable.
It will also update the client dependencies. Related to issue #658.

Testing

Manual testing for now.

Impact

  • Zod dependency.
  • Once complete, the website access flow will be different if your config is wrong.
  • LayoutPublic is parameterized with a possible Breakpoint instead of always being 'xs'.
  • Additional route of '/verify'

Additional Information

None.

Checklist

  • My code adheres to the coding and style guidelines of the project.
  • I have added tests for all the new code and any changes made to
    existing code.
  • I have made corresponding changes to the documentation.

@atomicgamedeveloper atomicgamedeveloper changed the title Validates client config 3 Validates client config Dec 18, 2024
@prasadtalasila
Copy link
Contributor

@atomicgamedeveloper Is this PR complete w.r.t the changes discussed in PR #1083?

@prasadtalasila prasadtalasila added this to the Release v0.7.0 milestone Dec 19, 2024
@atomicgamedeveloper
Copy link
Contributor Author

We're getting close, though change number 6. requires some ingenuity. Please note my comments under the proposed changes of 1., 4., 6., 8 and 9.

  • 1. The auth logic need not have code for config validation. How about having src/route/config for /verify route? Is there any disadvantage to it? If you think that config shown to the user must be different from the one shown to the developer, you can always have config/user and config/developer (instead of config/verify).

I moved ConfigItems.tsx and VerifyConfig.tsx into a dedicated folder, src/route/config.

  • 2. Please change the src/build/env.js to have the following content.
        if (typeof window !== 'undefined') {
          window.env = {
            REACT_APP_ENVIRONMENT: 'dev',
            REACT_APP_URL: 'http://localhost:4000/',
            REACT_APP_URL_BASENAME: '',
            REACT_APP_URL_DTLINK: '/lab',
            REACT_APP_URL_LIBLINK: '',
            REACT_APP_WORKBENCHLINK_VNCDESKTOP: '/tools/vnc/?password=vncpassword',
            REACT_APP_WORKBENCHLINK_VSCODE: '/tools/vscode/',
            REACT_APP_WORKBENCHLINK_JUPYTERLAB: '/lab',
            REACT_APP_WORKBENCHLINK_JUPYTERNOTEBOOK: '',
            REACT_APP_WORKBENCHLINK_LIBRARY_PREVIEW: '/preview/library',
            REACT_APP_WORKBENCHLINK_DT_PREVIEW: '/preview/digitaltwins',

            REACT_APP_CLIENT_ID: '1be55736756190b3ace4c2c4fb19bde386d1dcc748d20b47ea8cfb5935b8446c',
            REACT_APP_AUTH_AUTHORITY: 'https://gitlab.com/',
            REACT_APP_REDIRECT_URI: 'http://localhost:4000/Library',
            REACT_APP_LOGOUT_REDIRECT_URI: 'http://localhost:4000',
            REACT_APP_GITLAB_SCOPES: 'openid profile read_user read_repository api',
          };
        };
  • 3. There is an infinite loop in the config validation. It seems that the config validation code is making a lot of requests to the server. You can check with the following commands.

      1. yarn clean, yarn install, yarn build, yarn start
      2. Open browser and visit `localhost:4000`
      3. See the terminal log
         Please check for this problem for the developer verify route as well.
    
  • 4. We discussed about showing a simpler message to users. The message to be shown is:
    Invalid Application Configuration. Please contact the administrator of your DTaaS installation.

I assume you want a clean, simple design here, so for now it just displays this message and no concrete config problems, i.e.

Screenshot 2024-12-18 172323

And not

Screenshot 2024-12-18 172148

  • 5. When the https://gitlab.com URL is used, the login button is shown but the browser client is making lot of requests in the background. Please check it using Web developer Tools.

  • 6. The configuration logic is embedded in . The config validation happens only once while the auth validation happens every time for change in a route. So the config logic needs to be brought out of auth route. How about replacing <LayoutPublic> with new <Config> react element for wrapping <SignIn />. If you want, the existing config validation code could be moved from auth route to signin route. In any case, the code structuring has to be such that:

      1. Configuration validation is done only on the signin page.
      2. Two routes: `config/user` and `config/developer` are wrapped using `<LayoutPublic>`
    

I've looked into different ways of having a singular verification of the config. One idea is to wrap the app in a <Config> react element which then caries the context, close to your proposition. Alternatively, you could have an endpoint like config/verify which returns the config if it has been verified, and otherwise makes it and returns from then on out. Either way, I doubt it will be finished before next week.

  • 7. For getValidationResults() method, please see if you can use an Promise.all with array or record iterable.

  • 8. Dependabot updates

This is not started.

  • 9. Testing

The current implementation of SignIn is tested and it doesn't break any other tests. But with a new implementation we need new tests of course. Functions like fetching the different URLs or the getVerifications are not tested either.
Overall, there is still a not insignificant amount of work left, which I doubt will be able to meet the suggested deadline (end of this week).

@prasadtalasila
Copy link
Contributor

I've looked into different ways of having a singular verification of the config. One idea is to wrap the app in a <Config> react element which then caries the context, close to your proposition. Alternatively, you could have an endpoint like config/verify which returns the config if it has been verified, and otherwise makes it and returns from then on out. Either way, I doubt it will be finished before next week.

export const routes = [
  {
    path: '/',
    element: (
      <LayoutPublic containerMaxWidth="md">
        <Config role='user' />
      </LayoutPublic>
    ),
  },
  {
    path: 'config/verify',
    element: (
      <LayoutPublic containerMaxWidth="md">
        <Config role='developer' />
      </LayoutPublic>
    ),
  },
  {
    path: 'signin',
    element: (
      <LayoutPublic>
        <SignIn />
      </LayoutPublic>
    ),
  },
  {
    path: 'login',
    element: (
      <LayoutPublic>
        <SignIn />
      </LayoutPublic>
    ),
  },
  ......
];

Please note that a successful verification of config on "/" route redirects page to "/signin" route.

Another improvement could be to store the result of config validation in redux store and use it in other routes. All other routes can use (reducers) to deal with config changes. The advantages of this method are:

  1. The element need not wrap others
  2. A periodic checks can be done in the background to update the state of config in the redux store. Any page user is on, will go to the config error page upon detection of an invalid config. (This is for future - not to be done in this PR)

@prasadtalasila
Copy link
Contributor

I assume you want a clean, simple design here, so for now it just displays this message and no concrete config problems, i.e.

Screenshot 2024-12-18 172323

Yes. Is the box responsive?

* [ ]  8. Dependabot updates

Please skip it for this PR

* [ ]  9. Testing

The current implementation of SignIn is tested and it doesn't break any other tests. But with a new implementation we need new tests of course. Functions like fetching the different URLs or the getVerifications are not tested either. Overall, there is still a not insignificant amount of work left, which I doubt will be able to meet the suggested deadline (end of this week).

No problem. I am going to do one review of PR now. Please let me know when the PR is ready for next review.

@prasadtalasila
Copy link
Contributor

When I did the following,

>yarn install
>yarn build
>yarn start

The following error comes

image

Please note that the default configuration in public/env.js is being used here.

On the other hand, the following page shows correct result.

image

Copy link
Contributor

@prasadtalasila prasadtalasila left a comment

Choose a reason for hiding this comment

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

@atomicgamedeveloper please see the comments.

client/package.json Outdated Show resolved Hide resolved
client/src/route/config/Verify.tsx Outdated Show resolved Hide resolved
client/src/route/auth/Signin.tsx Outdated Show resolved Hide resolved
client/src/route/auth/Signin.tsx Outdated Show resolved Hide resolved
client/src/route/auth/Signin.tsx Show resolved Hide resolved
client/src/route/config/Verify.tsx Outdated Show resolved Hide resolved
client/src/route/config/Verify.tsx Outdated Show resolved Hide resolved
client/src/route/config/ConfigItems.tsx Outdated Show resolved Hide resolved
client/src/route/config/ConfigItems.tsx Show resolved Hide resolved
);
};

export type validationType = {
Copy link
Contributor

Choose a reason for hiding this comment

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

these members are used in getConfigIcon() as mandatory parameters. I am surprised that tsc has not thrown an error.

atomicgamedeveloper and others added 16 commits December 19, 2024 22:22
Bumps [cross-spawn](https://github.com/moxystudio/node-cross-spawn) from 7.0.3 to 7.0.6.
- [Changelog](https://github.com/moxystudio/node-cross-spawn/blob/master/CHANGELOG.md)
- [Commits](moxystudio/node-cross-spawn@v7.0.3...v7.0.6)

---
updated-dependencies:
- dependency-name: cross-spawn
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <[email protected]>
Bumps [nanoid](https://github.com/ai/nanoid) from 3.3.7 to 3.3.8.
- [Release notes](https://github.com/ai/nanoid/releases)
- [Changelog](https://github.com/ai/nanoid/blob/main/CHANGELOG.md)
- [Commits](ai/nanoid@3.3.7...3.3.8)

---
updated-dependencies:
- dependency-name: nanoid
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <[email protected]>
…n/client/cross-spawn-7.0.6

Bump cross-spawn from 7.0.3 to 7.0.6 in /client
@prasadtalasila
Copy link
Contributor

@atomicgamedeveloper , the codeclimate issues seem to have gone up in this PR. Please check the existing issues vs new ones. Please resolve them.

@prasadtalasila
Copy link
Contributor

@atomicgamedeveloper , I have checked the code changes and they seem fine.

Then I tried the code inside docker container using the docker/compose.dev.yml. This application created by this compose file can be visualised as following.

developer-docker

It shows the invalid configuration on the signin page and the configuration verification page shows the following error.

image

Please note that the docker scenario runs the client behind traefik proxy. How does it effect the functionality?

The existing setup in docker/ has a problem compiling client code into docker image (issue #1101). I've pushed the updated code with a temporary fix into my fork. Please use that code in docker directory for recreating the setup I tested.

@prasadtalasila
Copy link
Contributor

prasadtalasila commented Jan 5, 2025

The codeclimate issues in the new code:
config.tsx
configUtils.ts
WaitAndNavigate.tsx
authRedux.test.tsx
SignIn.test.tsx
PrivateRoute.test.tsx

Some of these files have duplication shown for fewer than 5 lines. You can increase the duplication mass in codeclimate.yml to ignore these duplication issues. Please remember to increase the mass only for typescript and not for others.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: In progress
Development

Successfully merging this pull request may close these issues.

Validate react website configuration
2 participants