Java upload tab #2945

Chang-CH · 2024-04-16T09:34:00Z

Description

Allows users to bypass compiler + type checker in local development by uploading class files to run directly.

Supports multiple file uploads.

Type of change

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
This change requires a documentation update
Code quality improvements

How to test

Compile java file(s) with javac *.java -target 8 -source 8 and upload

Checklist

I have tested this code
I have updated the documentation

…o java-jvm-tab

coveralls · 2024-04-16T09:50:48Z

Pull Request Test Coverage Report for Build 9049753998

Details

10 of 64 (15.63%) changed or added relevant lines in 8 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage decreased (-0.05%) to 32.062%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
src/pages/playground/Playground.tsx	1	2	50.0%
src/commons/workspace/WorkspaceActions.ts	2	4	50.0%
src/commons/sagas/PlaygroundSaga.ts	0	3	0.0%
src/commons/workspace/WorkspaceReducer.ts	0	6	0.0%
src/commons/sideContent/content/SideContentUpload.tsx	2	21	9.52%
src/commons/utils/JavaHelper.ts	0	23	0.0%

Totals
Change from base Build 8971907944:	-0.05%
Covered Lines:	5021
Relevant Lines:	14700

💛 - Coveralls

RichDom2185

Will this be safe (considering one can upload any file)? Any potential social engineering attack vector? Is it possible to access the JS environment from Java (and e.g. steal browser cookies)?

Chang-CH · 2024-04-18T04:42:50Z

Will this be safe (considering one can upload any file)? Any potential social engineering attack vector? Is it possible to access the JS environment from Java (and e.g. steal browser cookies)?

At the moment most of the standard library is not yet implemented, there are no native functions that access network.

Ideally we only need the run tab, but due to the limitations of the type checker and compiler this was requested as a feature.

martin-henz · 2024-04-18T05:03:21Z

Yes, maybe this feature should only be available in the command line tool and not in sourceacademy.org? I agree that for now it's ok. Maybe we include it for the project presentation next week and then remove it?

Chang-CH · 2024-04-18T06:55:14Z

Yes, maybe this feature should only be available in the command line tool and not in sourceacademy.org? I agree that for now it's ok. Maybe we include it for the project presentation next week and then remove it?

I am planning to use localhost for presentation. I think mei tin is using the localhost approach as well. Is there a way we only enable it for dev and not production? do we have feature flags?

RichDom2185 · 2024-04-18T10:13:19Z

Is there a way we only enable it for dev and not production?

Not sure, does process.env.NODE_ENV work?

do we have feature flags?

No we don't (at least not yet for now…)

Chang-CH · 2024-04-18T13:49:14Z

Is there a way we only enable it for dev and not production?

Not sure, does process.env.NODE_ENV work?

looks like it does, added the check.

RichDom2185

Hi, thanks for working on this! I have some comments below:

RichDom2185 · 2024-04-18T15:29:02Z

src/commons/sideContent/content/SideContentUpload.tsx

+class SideContentUpload extends React.Component<OwnProps> {
+  public render() {
+    return (
+      <div>
+        <p>
+          Bypass the compiler and type checker by uploading class files to run in the JVM.
+          <br />
+          <br />
+          Only .class files are accepted. Code in the editor will be ignored when running while this
+          tab is active.
+          <br />
+          <br />
+          Compile the files with the following command:
+          <code>javac *.java -target 8 -source 8</code>
+          <br />
+          <br />
+          Avoid running class files downloaded from unknown sources.
+          <br />
+          <br />
+          <strong>Main class must be named Main and uploaded as Main.class.</strong>
+        </p>
+        <input
+          type="file"
+          multiple
+          accept=".class"
+          onChange={e => {
+            const ret: { [key: string]: string } = {};
+            const promises = [];
+            for (const file of e.target.files ?? []) {
+              if (file.name.endsWith('.class')) {
+                promises.push(
+                  getBase64(file, (b64: string) => {
+                    ret[file.name] = b64;
+                  })
+                );
+              }
+            }
+            Promise.all(promises).then(() => {
+              this.props.onUpload(ret);
+            });
+          }}
+        />
+      </div>
+    );
+  }
+}


We are trying to move away from React component classes into functional components instead. Could you refactor this?

RichDom2185 · 2024-04-18T15:29:50Z

src/commons/sideContent/content/SideContentUpload.tsx

+        <p>
+          Bypass the compiler and type checker by uploading class files to run in the JVM.
+          <br />
+          <br />
+          Only .class files are accepted. Code in the editor will be ignored when running while this
+          tab is active.
+          <br />
+          <br />
+          Compile the files with the following command:
+          <code>javac *.java -target 8 -source 8</code>
+          <br />
+          <br />
+          Avoid running class files downloaded from unknown sources.
+          <br />
+          <br />
+          <strong>Main class must be named Main and uploaded as Main.class.</strong>
+        </p>


Semantically, does it make sense to have everything in a single paragraph with so many line breaks, instead of multiple p/pre tags?

RichDom2185 · 2024-04-18T15:30:37Z

src/commons/sideContent/content/SideContentUpload.tsx

+          multiple
+          accept=".class"
+          onChange={e => {
+            const ret: { [key: string]: string } = {};


This type to represent a file is used a lot. Can we move this to a type alias to ensure single source of truth?

RichDom2185 · 2024-04-18T15:31:55Z

src/commons/sideContent/content/SideContentUpload.tsx

+
+const makeUploadTabFrom = (
+  location: SideContentLocation,
+  onUpload: (files: { [key: string]: any }) => void


Or perhaps this type could be aliased instead (or both).

RichDom2185 · 2024-04-18T15:32:28Z

src/commons/utils/JavaHelper.ts

@@ -106,7 +91,34 @@ export async function javaRun(
    }
  };

-  if (isUsingCse) return await runJavaCseMachine(javaCode, targetStep, context);
+  // #endregion


Can we remove this comment?

RichDom2185 · 2024-04-18T15:33:48Z

src/commons/workspace/WorkspaceReducer.ts

+      const { workspaceLocation } = action.payload;
+      if (workspaceLocation === 'playground' || workspaceLocation === 'sicp') {
+        state[workspaceLocation].usingUpload = action.payload.usingUpload;
+      }
+    })
+    .addCase(uploadFiles, (state, action) => {
+      const { workspaceLocation } = action.payload;


workspaceLocation should not be accessed via destructuring of action.payload. Refer to the other functions for how (i.e. getWorkspaceLocation).

* Memoize change handler * Fix layout and UI bugs * Use Blueprint file input instead of HTML input component

RichDom2185

Hi, I've fixed some of the bugs and typings as per my above comments but I still have some comments below:

Also, could you resolve the merge conflicts?

Thanks!

RichDom2185 · 2024-04-21T16:26:27Z

src/commons/sideContent/content/SideContentUpload.tsx

+/**
+ * This component is responsible for uploading Java class files to bypass the compiler.
+ */
+const SideContentUpload: React.FC<Props> = ({ onUpload, workspaceLocation }) => {


The workspaceLocation prop is not used. Is this intentional?

RichDom2185 · 2024-04-21T16:26:51Z

src/commons/sideContent/content/SideContentUpload.tsx

+      <FileInput
+        inputProps={{ multiple: true, accept: '.class' }}
+        onInputChange={handleFileUpload}
+      />


Can we keep track of the state by updating the text prop? See https://blueprintjs.com/docs/#core/components/file-input

Not quite sure what you mean by this. Do we want to show some kind of text to indicate what files are uploaded like "4 files uploaded"?

Added this change, if that is what you meant.

…nto java-jvm-tab

RichDom2185

LGTM, thanks and sorry for the delayed review!

* add class file upload tab * unused code param * fix imports * update reducer pattern * update upload text * fix: tsc missing properties * disable upload tab on prod * fix comments * Fix component typing * Fix incorrect workspace location in reducer * Refactor SideContentUpload component * Memoize change handler * Fix layout and UI bugs * Use Blueprint file input instead of HTML input component * Fix typing * remove location prop * Show upload count * Fix format * Improve typing * Update typing --------- Co-authored-by: Richard Dominick <[email protected]>

Chang-CH added 11 commits April 9, 2024 19:36

add class file upload tab

2d6cc43

unused code param

8f32331

fix imports

4326e60

Merge branch 'master' into java-jvm-tab

2db67e4

update reducer pattern

e472f84

Merge branch 'master' into java-jvm-tab

bbdeefd

Merge branch 'master' into java-jvm-tab

f6b4e6c

Merge branch 'master' into java-jvm-tab

3d32bcc

Merge branch 'java-jvm-tab' of github.com:source-academy/frontend int…

015942d

…o java-jvm-tab

update upload text

9b9bf5d

fix: tsc missing properties

33bc3ea

RichDom2185 reviewed Apr 18, 2024

View reviewed changes

Chang-CH added 2 commits April 18, 2024 21:44

disable upload tab on prod

da17a09

Merge branch 'master' into java-jvm-tab

5e04dab

Chang-CH marked this pull request as ready for review April 18, 2024 14:18

RichDom2185 requested changes Apr 18, 2024

View reviewed changes

fix comments

ea34e4d

Chang-CH requested a review from RichDom2185 April 21, 2024 01:53

RichDom2185 added 3 commits April 22, 2024 00:04

Fix component typing

7fb6cea

Fix incorrect workspace location in reducer

35fdc53

Refactor SideContentUpload component

7164191

* Memoize change handler * Fix layout and UI bugs * Use Blueprint file input instead of HTML input component

RichDom2185 requested changes Apr 21, 2024

View reviewed changes

RichDom2185 and others added 2 commits April 22, 2024 01:08

Fix typing

dc5d627

Merge branch 'master' into java-jvm-tab

ca4082f

Chang-CH and others added 9 commits April 22, 2024 12:02

remove location prop

450932f

Show upload count

d8a0ff4

Merge branch 'master' into java-jvm-tab

9416965

Merge branch 'master' of https://github.com/source-academy/frontend i…

08be256

…nto java-jvm-tab

Merge branch 'master' into java-jvm-tab

0f2edd1

Merge branch 'master' of https://github.com/source-academy/frontend i…

06d9e93

…nto java-jvm-tab

Fix format

740c8c0

Merge branch 'master' of https://github.com/source-academy/frontend i…

e799c09

…nto java-jvm-tab

Improve typing

277292d

RichDom2185 approved these changes May 12, 2024

View reviewed changes

RichDom2185 enabled auto-merge (squash) May 12, 2024 07:25

RichDom2185 disabled auto-merge May 12, 2024 07:27

Update typing

f5178e4

RichDom2185 enabled auto-merge (squash) May 12, 2024 07:28

RichDom2185 merged commit bbe8abe into master May 12, 2024
8 checks passed

RichDom2185 deleted the java-jvm-tab branch May 12, 2024 07:34

Java upload tab #2945

Java upload tab #2945

Uh oh!

Conversation

Chang-CH commented Apr 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Type of change

How to test

Checklist

Uh oh!

coveralls commented Apr 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 9049753998

Details

💛 - Coveralls

Uh oh!

RichDom2185 left a comment

Choose a reason for hiding this comment

Uh oh!

Chang-CH commented Apr 18, 2024

Uh oh!

martin-henz commented Apr 18, 2024

Uh oh!

Chang-CH commented Apr 18, 2024

Uh oh!

RichDom2185 commented Apr 18, 2024

Uh oh!

Chang-CH commented Apr 18, 2024

Uh oh!

RichDom2185 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RichDom2185 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Chang-CH Apr 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RichDom2185 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Chang-CH commented Apr 16, 2024 •

edited

Loading

coveralls commented Apr 16, 2024 •

edited

Loading

RichDom2185 left a comment •

edited

Loading

Chang-CH Apr 22, 2024 •

edited

Loading