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

Untrusted source warning #1152

Merged
merged 12 commits into from
Aug 14, 2024
Merged

Untrusted source warning #1152

merged 12 commits into from
Aug 14, 2024

Conversation

akurinnoy
Copy link
Contributor

@akurinnoy akurinnoy commented Jul 24, 2024

What does this PR do?

This PR adds the ability to warn users that creating workspaces from unknown sources can be dangerous.

Import from a Git repository (updated)
Screen.Recording.2024-08-13.at.15.52.21.mov
Factory link (updated)
Screen.Recording.2024-08-13.at.19.24.32.mov
Samples are always trusted
Screen.Recording.2024-07-26.at.17.52.27.mov

What issues does this PR fix or reference?

resolves eclipse-che/che#23018

Is it tested? How?

  1. Deploy Eclipse Che and run command to patch the CR:
    kubectl patch -n eclipse-che "checluster/eclipse-che" --type=json -p="[{"op": "replace", "path": "/spec/components/dashboard/deployment", "value": {containers: [{image: "quay.io/eclipse/che-dashboard:pr-1152", name: che-dashboard}]}}]"
  2. Import a Git repo URL on the Create Workspace page
    2.1. You should see a warning window
    2.2. Click Cancel to interrupt the creation of a new workspace.
    2.3. Click Continue to continue creating a new workspace.
  3. Use a factory link (other than the one used above)
    3.1. You should see a warning window
    3.2. Click Cancel to interrupt the creation of a new workspace.
    3.3. Click Continue to continue creating a new workspace.

Copy link

openshift-ci bot commented Jul 24, 2024

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@che-bot
Copy link
Contributor

che-bot commented Jul 24, 2024

Click here to review and test in web IDE: Contribute

Signed-off-by: Oleksii Kurinnyi <okurinny@redhat.com>
Copy link

Docker image build succeeded: quay.io/eclipse/che-dashboard:pr-1152

kubectl patch command
kubectl patch -n eclipse-che "checluster/eclipse-che" --type=json -p="[{"op": "replace", "path": "/spec/components/dashboard/deployment", "value": {containers: [{image: "quay.io/eclipse/che-dashboard:pr-1152", name: che-dashboard}]}}]"

Copy link

codecov bot commented Jul 24, 2024

Codecov Report

Attention: Patch coverage is 95.87302% with 13 lines in your changes missing coverage. Please review.

Project coverage is 89.76%. Comparing base (a288cf3) to head (48fd2e1).
Report is 3 commits behind head on main.

Files Patch % Lines
...tend/src/components/UntrustedSourceModal/index.tsx 96.77% 5 Missing ⚠️
...rontend/src/components/WorkspaceProgress/index.tsx 90.90% 4 Missing ⚠️
...rd-frontend/src/components/ImportFromGit/index.tsx 95.45% 2 Missing ⚠️
...rkspaceProgress/CreatingSteps/Initialize/index.tsx 96.42% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1152      +/-   ##
==========================================
+ Coverage   89.46%   89.76%   +0.30%     
==========================================
  Files         415      416       +1     
  Lines       42461    42749     +288     
  Branches     2831     2876      +45     
==========================================
+ Hits        37987    38374     +387     
+ Misses       4443     4348      -95     
+ Partials       31       27       -4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link

Docker image build succeeded: quay.io/eclipse/che-dashboard:pr-1152

kubectl patch command
kubectl patch -n eclipse-che "checluster/eclipse-che" --type=json -p="[{"op": "replace", "path": "/spec/components/dashboard/deployment", "value": {containers: [{image: "quay.io/eclipse/che-dashboard:pr-1152", name: che-dashboard}]}}]"

Copy link

Docker image build succeeded: quay.io/eclipse/che-dashboard:pr-1152

kubectl patch command
kubectl patch -n eclipse-che "checluster/eclipse-che" --type=json -p="[{"op": "replace", "path": "/spec/components/dashboard/deployment", "value": {containers: [{image: "quay.io/eclipse/che-dashboard:pr-1152", name: che-dashboard}]}}]"

Signed-off-by: Oleksii Kurinnyi <okurinny@redhat.com>
Copy link

Docker image build succeeded: quay.io/eclipse/che-dashboard:pr-1152

kubectl patch command
kubectl patch -n eclipse-che "checluster/eclipse-che" --type=json -p="[{"op": "replace", "path": "/spec/components/dashboard/deployment", "value": {containers: [{image: "quay.io/eclipse/che-dashboard:pr-1152", name: che-dashboard}]}}]"

@akurinnoy akurinnoy marked this pull request as ready for review July 26, 2024 15:25
@akurinnoy akurinnoy self-assigned this Jul 26, 2024
@ibuziuk ibuziuk requested a review from dmytro-ndp July 29, 2024 13:46
@dmytro-ndp
Copy link
Contributor

dmytro-ndp commented Jul 30, 2024

@akurinnoy : here is the PR validation result:
Test scenario from PR description passed as expected using quay.io/eclipse/che-dashboard:pr-1152 on Eclipse Che Next deployed to OCP 4.16:
screencast-bpconcjcammlapcogcnnelfmaeghhagj-2024.07.30-16_28_15.webm;

Extra use cases to consider:

  1. It seems to be redundant to show the untrusted source warning popup when run empty workspace.
  2. Assuming that the popup granted workspace creation, not workspace start, it seems misleading to show the untrusted source warning popup again when restarting the workspace failed to start or when using the VS Code Editor > Restart Workspace command.
    Additionally, the popup seemed to be ignored at the time.
    See screencasts:
  1. Message (within this session) is not so informative, IMHO. Did it mean authentication session?
    Screenshot from 2024-07-30 17-15-11

  2. Untrusted source warning popup option Do not ask me again didn't prevent new popup in case of "Existing workspace found" error:
    screencast-bpconcjcammlapcogcnnelfmaeghhagj-2024.07.30-17_23_17.webm

Signed-off-by: Oleksii Kurinnyi <okurinny@redhat.com>
Copy link
Contributor

@olexii4 olexii4 left a comment

Choose a reason for hiding this comment

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

LGTM

Signed-off-by: Oleksii Kurinnyi <okurinny@redhat.com>
Comment on lines 44 to 57
// const mockGet = jest.fn().mockReturnValue('all');
// jest.mock('@/services/session-storage', () => {
// return {
// __esModule: true,
// default: {
// get: (...args: unknown[]) => mockGet(...args),
// },
// // enum
// SessionStorageKey: {
// TRUSTED_SOURCES: 'trusted-sources', // 'all' or 'repo1,repo2,...'
// },
// };
// });

Copy link
Member

Choose a reason for hiding this comment

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

should we delete or fix it ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this are leftovers from the prev implementation, I'll delete them in the next commit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Copy link
Member

@ibuziuk ibuziuk left a comment

Choose a reason for hiding this comment

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

Great job with test coverage, my only comment is that semantically we should consider * vs all

@dmytro-ndp
Copy link
Contributor

dmytro-ndp commented Aug 13, 2024

@akurinnoy, @ibuziuk : I have verified new version of PR (Eclipse Che Next + quay.io/eclipse/che-dashboard@sha256:bb6c15af82f9ac20743455b4d2fb9e78e18101b5298e0b9acf55a92f18e4cf15) and have a few questions:

  1. Could you please, confirm that it is expected that Dashboard stopped displaying "Untrusted source warning" popup for any other workspace URLs after setting Do not ask me again: true?
screencast-bpconcjcammlapcogcnnelfmaeghhagj-2024.08.13-20_26_23.webm
  1. It was needed to refresh the browser's tab with the 'Create Workspace' page for the new setting Do not ask me again: true to take effect. It was expected that this change would automatically affect all other open browser tabs.

  2. Not sure if it was expected that User Dashboard didn't show "Untrusted source warning" popup when I tried to create workspace from one of non-empty sample displayed at "Create Workspace" page.

Note: I have also verified "Untrusted source warning" popup with different users - Do not ask me again: true settings worked correctly and didn't affect another user.

@akurinnoy
Copy link
Contributor Author

@dmytro-ndp thanks for the feedback!

  1. Could you please, confirm that it is expected that Dashboard stopped displaying "Untrusted source warning" popup for any other workspace URLs after setting Do not ask me again: true?

Yep, this is expected.

  1. It was needed to refresh the browser's tab with the 'Create Workspace' page for the new setting Do not ask me again: true to take effect. It was expected that this change would automatically affect all other open browser tabs.

If you mean inactive browser tabs then yes, this is the expected behavior. Automatically affecting inactive dashboard tabs would be a good improvement, but it's outside this issue's scope.
Otherwise, this is a bug.

  1. Not sure if it was expected that User Dashboard didn't show "Untrusted source warning" popup when I tried to create workspace from one of non-empty sample displayed at "Create Workspace" page.

This behavior is expected. I tend to think that samples are trustworthy sources, but I’m OK to discuss it and tune this behavior.
cc @ibuziuk

@akurinnoy
Copy link
Contributor Author

Updated confirmation modal:

Screenshot 2024-08-14 at 14 28 03

Copy link

Docker image build succeeded: quay.io/eclipse/che-dashboard:pr-1152

kubectl patch command
kubectl patch -n eclipse-che "checluster/eclipse-che" --type=json -p="[{"op": "replace", "path": "/spec/components/dashboard/deployment", "value": {containers: [{image: "quay.io/eclipse/che-dashboard:pr-1152", name: che-dashboard}]}}]"

Copy link

openshift-ci bot commented Aug 14, 2024

@akurinnoy: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/v14-dashboard-happy-path f150deb link true /test v14-dashboard-happy-path

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

Copy link
Contributor

@olexii4 olexii4 left a comment

Choose a reason for hiding this comment

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

LGTM

@olexii4
Copy link
Contributor

olexii4 commented Aug 14, 2024

@akurinnoy BTW: ci/prow/v14-dashboard-happy-path job has two failed tests:

...
  2 failing
  1) The SmokeTest userstory 
       Create workspace from factory:https://github.com/che-incubator/quarkus-api-example.git
         Create and open new workspace from factory:https://github.com/che-incubator/quarkus-api-example.git:
     TimeoutError: Wait timed out after 40197ms
      at /tmp/e2e/node_modules/selenium-webdriver/lib/webdriver.js:907:17
      at processTicksAndRejections (node:internal/process/task_queues:95:5)
  2) "after each" hook: deleteWorkspaceOnFailedTest for "Create and open new workspace from factory:https://github.com/che-incubator/quarkus-api-example.git":
     Error: Request failed with status code 400
     ...

Copy link
Contributor

@dmytro-ndp dmytro-ndp left a comment

Choose a reason for hiding this comment

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

@akurinnoy, @ibuziuk: thank you the feedback and taking PR review notices into account.
I have tested new changes (quay.io/eclipse/che-dashboard@sha256:ea0380eb2faa5c24be6fb01717d93016f35714db574be2bfd1aaffd9a575cf2f) and Untrusted source warning popup looked more clear and worked as expected, IMHO.

user.webm

and then

kubeadmin.webm

Copy link

openshift-ci bot commented Aug 14, 2024

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: akurinnoy, dmytro-ndp, ibuziuk, olexii4

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@akurinnoy akurinnoy merged commit 295ecd5 into main Aug 14, 2024
15 of 16 checks passed
@akurinnoy akurinnoy deleted the untrusted-source-warning branch August 14, 2024 15:22
@devstudio-release
Copy link

Build 3.17 :: dashboard_3.x/526: Console, Changes, Git Data

@devstudio-release
Copy link

Build 3.16 :: dashboard_3.16/10: Console, Changes, Git Data

@devstudio-release
Copy link

@devstudio-release
Copy link

@devstudio-release
Copy link

@devstudio-release
Copy link

@devstudio-release
Copy link

Build 3.17 :: get-sources-rhpkg-container-build_3.x/7440: FAILURE

dashboard : 3.x ::
; copied to quay

@devstudio-release
Copy link

Build 3.16 :: get-sources-rhpkg-container-build_3.16/20: FAILURE

dashboard : 3.16 ::
; copied to quay

@dmytro-ndp
Copy link
Contributor

Note: "Trust Author" popup displayed as expected when I had created workspace out of UI - by using factory URL, e.g.
https://eclipse-che.apps.ocp416-dnochev.crw-qe.com/dashboard/#/https://github.com/devfile/developer-images
Screenshot from 2024-08-14 23-41-35

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.

As a User, I want to be warned that creating a workspace from unknown source could be dangerous
7 participants