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

Enhance the workflowAssistApps so that third party links can have a check access url #6994

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

ianwallen
Copy link
Contributor

Enhance the workflowAssistApps so that third party links have a check url which can be used to decide if the link should be displayed to the user or not.

We require this option so that we can have it call a check url on our third party app to decide if the user has access to the link that we added. Otherwise some users are clicking on the link just to get an access denied page.

It adds the appAccessCheckUrl and it will follow this rule.

  • If the appAccessCheckUrl exists then it will call it and check for success. If success then the link will be displayed.
  • If it does return a success then it does an extra check for false values - i.e. 0, no, false.... If that was returned from the call then it will not display the link.
  • And if the appAccessCheckUrl does not exist then it will always be displayed.

Here is a sample that was used for testing.

{
  "mods": {
    "workflowHelper": {
      "enabled": true,
      "workflowAssistApps": [
        {
          "appUrl": "https://www.google.ca/search?q={uuid}",
          "appLabelKey": "testkey",
          "appAccessCheckUrl": ""
        },
        {
          "appUrl": "https://www.google.ca/search?q={uuid}",
          "appLabelKey": "testkey2",
          "appAccessCheckUrl": "https://www.google.ca"
        },
        {
          "appUrl": "https://www.google.ca/search?q={uuid}",
          "appLabelKey": "testkey3",
          "appAccessCheckUrl": "https://www.bad.ca"
        }
      ]
    }
  }
}

It produces the following configuration.

image

And when viewing a metadata record, selecting
image

In this case testkey3 is not displayed because www.bad.ca returns an http 500 error.

isAccessible: '='
},
link: function(scope, element, attrs) {
$http.get(attrs.gnAppUrlAccessibilityCheck)
Copy link
Contributor

Choose a reason for hiding this comment

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

using the HEAD verb would be more appropriate here, otherwise the client may end up downloading a lot of stuff

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I originally used HEAD but then I did not like seen all the error message in the connection logs so I added the option to read the results from the body. If we do a HEAD term then we cannot use the body to check for text like "false, 0,,,"

I'm not sure if it is considered bad practice to check the body text or not? In our organization, we do have a couple API's that simply return an HTTP code 200 with "true" or "false" in the response body.

I can remove the body check and change it to a head request if that is best practice.

… url which can be used to decide if the link should be displayed to the user or not.
@ianwallen ianwallen force-pushed the feature/helper_app_check branch from a007ea1 to 42dce39 Compare April 14, 2023 20:16
@ianwallen ianwallen requested a review from josegar74 April 20, 2023 15:23
@fxprunayre fxprunayre modified the milestones: 4.2.4, 4.2.5 May 10, 2023
@fxprunayre fxprunayre modified the milestones: 4.2.5, 4.2.6 Jul 7, 2023
@jodygarnett
Copy link
Contributor

My feedback after consideration - this approach makes me uncomfortable because it is a baked in SSRF as a feature.

If possible I would love to make a request from the security subsystem for a set of "roles", and change the check to a list of spring-security "roles".

Copy link
Contributor

@jodygarnett jodygarnett left a comment

Choose a reason for hiding this comment

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

Thanks for fixing the test failure

@fxprunayre fxprunayre modified the milestones: 4.2.6, 4.4.1 Oct 4, 2023
@fxprunayre fxprunayre modified the milestones: 4.4.1, 4.4.2 Nov 22, 2023
@fxprunayre fxprunayre modified the milestones: 4.4.2, 4.4.3 Jan 23, 2024
@ianwallen
Copy link
Contributor Author

In GN 4 there is a new page api.
It seems like the page api may be a better approach so I will investigate the page api to see if we can achieve the same results.

@fxprunayre fxprunayre modified the milestones: 4.4.3, 4.4.4 Mar 13, 2024
@ianwallen
Copy link
Contributor Author

Hopefully the same logic can be achieved using the page api's

These PR's may replace this one.

@fxprunayre fxprunayre modified the milestones: 4.4.4, 4.4.5 Apr 16, 2024
@fxprunayre fxprunayre modified the milestones: 4.4.5, 4.4.6 Jun 4, 2024
@fxprunayre fxprunayre modified the milestones: 4.4.6, 4.4.7 Oct 15, 2024
@CLAassistant
Copy link

CLAassistant commented Dec 8, 2024

CLA assistant check
All committers have signed the CLA.

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

Successfully merging this pull request may close these issues.

5 participants