Add check if modules are same domain #226

dariadomagala-sap · 2018-11-20T14:27:37Z

Description

Changes proposed in this pull request:

Add check if modules are same domain

Related issue(s)

kwiatekus · 2018-11-21T07:30:27Z

core/src/services/routing.js

  }
  return true;
 };

+const getDomainFromUrl = url => {


We should be able to detect multiple SPAs under one domain, i.e:
https://example.com/foo/ <-- root of one SPA
https://example.com/bar/ <-- root of another SPA

I guess we do not have a concept / configuration option for that, since you would be currently only able to detect hash based SPAs, path based are undetectable by only the url. In my opinion this is a followup feature, which requires grooming.

…in-modules

kwiatekus

Tested using tractor module:

      {
        label: 'Tractors',
        pathSegment: 'tractors',
        viewUrl: 'http://localhost:8080',
        keepSelectedForChildren: true,
        viewGroup : 'tractors',
        children : [
          {
            label: 'tractor',
            pathSegment: ':id',
            viewUrl: 'http://localhost:8080/:id',
            context: {
              projectId
            }
          }
        ]
      }

jesusreal

Please see my comments

jesusreal · 2018-12-04T13:59:28Z

core/src/services/routing.js

@@ -98,11 +98,29 @@ export const isNotSameDomain = (config, component) => {
      componentData.previousNodeValues.viewUrl
    );
    const nextUrl = getUrlWithoutHash(componentData.viewUrl);
-    return previousUrl != nextUrl;


I would suggest refactoring this code to a version where we have less returns, to make the code easier to understand and change in the future if needed. Here my suggestion:

export const isNotSameDomain = (config, component) => { if (config.iframe) { const componentData = component.get(); const previousUrl = getUrlWithoutHash( componentData.previousNodeValues.viewUrl ); const nextUrl = getUrlWithoutHash(componentData.viewUrl); if (previousUrl === nextUrl) { return false; } const previousUrlOrigin = getLocation(previousUrl); const nextUrlOrigin = getLocation(nextUrl); if (previousUrlOrigin === nextUrlOrigin) { const previousViewGroup = componentData.previousNodeValues.viewGroup; const nextViewGroup = componentData.viewGroup; if (previousViewGroup && nextViewGroup && previousViewGroup === nextViewGroup) { return false; } } } return true; };

So we only return false if we detect we are in the same domain, otherwise code execution will come to the last line of the function and return true.

Also split the if..else block to initialise some variables only if we need it. I executed the unit tests and they are still passing.

jesusreal · 2018-12-04T14:19:34Z

docs/navigation-configuration.md

@@ -244,7 +246,7 @@ The node parameters are as follows:
 - **keepSelectedForChildren** focuses the navigation on its current hierarchy, omitting the display of children.
 - **loadingIndicator.enabled** shows a loading indicator when switching between micro front-ends. If you have a fast micro front-end, you can disable this feature to prevent flickering of the loading indicator. This parameter is enabled by default.
 - **loadingIndicator.hideAutomatically** disables the automatic hiding of the loading indicator once the micro front-end is loaded. It is only considered if the loading indicator is enabled. It does not apply if the loading indicator is activated manually with the `LuigiClient.uxManager().showLoadingIndicator()` function. If the loading indicator is enabled and automatic hiding is disabled, use `LuigiClient.uxManager().hideLoadingIndicator()` to hide it manually in your micro front-end during the startup. This parameter is enabled by default.
-
+- **viewGroup** defines a group of views in the same domain sharing a common  security context, which allows for example frame re-using for better performance.


There is a space too much in 'common security'

jesusreal · 2018-12-04T14:45:39Z

core/src/services/routing.js

+    return node.viewGroup;
+  } else if (node.parent) {
+    return findViewGroup(node.parent);
+  } else return;


else return would not be needed, since javascript does implicitly that already

jesusreal · 2018-12-04T14:49:19Z

core/src/services/routing.js

@@ -98,11 +98,29 @@ export const isNotSameDomain = (config, component) => {
      componentData.previousNodeValues.viewUrl
    );
    const nextUrl = getUrlWithoutHash(componentData.viewUrl);
-    return previousUrl != nextUrl;


IMHO isNotSameDomain isn't anymore a valid name for the function. My proposal is isNotSameViewGroup. Or as it is a Luigi internal function, we could change the returns and how we use it and rename it to isSameViewGroup. In general I find functions with negation more difficult to understand.

I will refactor the function so it doesn't have negation, but I don't like the isSameViewGroup nam, because in hash routing we don't have viewGroups. Maybe something more like isSameApplication, isSameIFrame?

jesusreal · 2018-12-04T15:08:43Z

core/test/services/routing.spec.js

@@ -797,6 +797,45 @@ describe('Routing', () => {
      previousNodeValues: { viewUrl: config.iframe.src }
    });
    assert.isTrue(routing.isNotSameDomain(config, component));
+


I would rather split this test in several tests, and have all under a describe('FUNCTION NAME', () => {}) block.

This would make also much easier to understand what each assertion is testing

…into same-domain-modules

jesusreal

LGTM after the changes performed.

I would specify in the description of the 2 tests cases for viewGroups "same domain", e.g.:
should return XXX if views have same domain and XXX viewGroups

bszwarc

Once comment :)

docs/navigation-configuration.md

Co-Authored-By: dariadomagala <[email protected]>

…into same-domain-modules

docs/navigation-configuration.md

bszwarc

Added one more comment

Co-Authored-By: dariadomagala <[email protected]>

…in-modules

…into same-domain-modules

Add check if modules are same domain

dariadomagala-sap added bug Something isn't working area/luigi labels Nov 20, 2018

dariadomagala-sap requested review from antiheld, hardl, jesusreal, kwiatekus, maxmarkus, parostatkiem-zz, pekura and y-kkamil as code owners November 20, 2018 14:27

kwiatekus suggested changes Nov 21, 2018

View reviewed changes

dariadomagala-sap added the WIP Work in progress label Nov 21, 2018

Add check if is same domain for path routing

1d0e0e7

dariadomagala-sap force-pushed the same-domain-modules branch from c15ddfb to 1d0e0e7 Compare December 3, 2018 14:13

dariadomagala-sap removed the WIP Work in progress label Dec 3, 2018

Update documentation

e98be32

dariadomagala-sap requested review from bszwarc, kazydek, klaudiagrz and tomekpapiernik as code owners December 4, 2018 09:22

kwiatekus added 2 commits December 4, 2018 11:23

Merge branch 'master' into same-domain-modules

7178e2a

Merge branch 'master' of github.com:kyma-project/luigi into same-doma…

9451880

…in-modules

jesusreal assigned jesusreal and kwiatekus Dec 4, 2018

kwiatekus approved these changes Dec 4, 2018

View reviewed changes

jesusreal suggested changes Dec 4, 2018

View reviewed changes

dariadomagala-sap added 2 commits December 5, 2018 14:42

Rename and refactor the isNotSameDomain function and improve tests

ae25e63

Merge branch 'same-domain-modules' of github.com:dariadomagala/luigi …

a8a21fe

…into same-domain-modules

jesusreal approved these changes Dec 5, 2018

View reviewed changes

bszwarc reviewed Dec 5, 2018

View reviewed changes

docs/navigation-configuration.md Outdated Show resolved Hide resolved

bszwarc and others added 4 commits December 5, 2018 15:36

Update docs/navigation-configuration.md

afec1cf

Co-Authored-By: dariadomagala <[email protected]>

Change tests descriptions

fd1111d

Merge branch 'same-domain-modules' of github.com:dariadomagala/luigi …

b7c7f3d

…into same-domain-modules

Merge branch 'master' into same-domain-modules

9ad1673

bszwarc reviewed Dec 5, 2018

View reviewed changes

docs/navigation-configuration.md Outdated Show resolved Hide resolved

bszwarc reviewed Dec 5, 2018

View reviewed changes

Update docs/navigation-configuration.md

acc5559

Co-Authored-By: dariadomagala <[email protected]>

bszwarc approved these changes Dec 6, 2018

View reviewed changes

kwiatekus added 2 commits December 6, 2018 10:52

Merge branch 'master' of github.com:kyma-project/luigi into same-doma…

7d117fd

…in-modules

Merge branch 'same-domain-modules' of github.com:dariadomagala/luigi …

c577f02

…into same-domain-modules

dariadomagala-sap merged commit 79f7ea3 into SAP:master Dec 6, 2018

dariadomagala-sap deleted the same-domain-modules branch December 6, 2018 10:56

stanleychh pushed a commit to stanleychh/luigi that referenced this pull request Dec 30, 2021

Add check if modules are same domain (SAP#226)

c2d9447

Add check if modules are same domain

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add check if modules are same domain #226

Add check if modules are same domain #226

dariadomagala-sap commented Nov 20, 2018

kwiatekus Nov 21, 2018

maxmarkus Nov 21, 2018

kwiatekus left a comment

jesusreal left a comment

jesusreal Dec 4, 2018

jesusreal Dec 4, 2018

jesusreal Dec 4, 2018

jesusreal Dec 4, 2018

dariadomagala-sap Dec 5, 2018

jesusreal Dec 4, 2018

jesusreal Dec 4, 2018

jesusreal left a comment

bszwarc left a comment

bszwarc left a comment

Add check if modules are same domain #226

Add check if modules are same domain #226

Conversation

dariadomagala-sap commented Nov 20, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kwiatekus left a comment

Choose a reason for hiding this comment

jesusreal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jesusreal left a comment

Choose a reason for hiding this comment

bszwarc left a comment

Choose a reason for hiding this comment

bszwarc left a comment

Choose a reason for hiding this comment