Add access control tab content

[derek-ho]

Description

Add access control tab content to datasource page

Issues Resolved

[List any issues this PR will resolve]

Check List

• New functionality includes testing.
  • All tests pass, including unit test, integration test and doctest
• New functionality has been documented.
  • New functionality has javadoc added
  • New functionality has user manual doc added
• Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

[derek-ho]

Assumes security plugin is installed and user has permission to access it (will be addressed with the permission PR)

[ps48]

What happens if security plugin is not installed. Are we not showing the whole data connections plugin? or Do we have a fallback view?

[derek-ho]

I think current plan is to show as much as possible:

• if users have access to datasources APIs, we show them the page, and if they do not then we will show Fallback UI
• if users have access to datasource APIs, but do not have access to security plugin or security plugin is not installed, we will not be able to show the dropdown with pre-defined roles, but we will still allow them to manually type in the roles. Created a follow up issue to avoid scope-creep: [FEATURE] Access Control flow if Security Plugin is not installed #1022. Will not implement manually type to create roles in this PR, but in follow up PR.

[derek-ho]

Placeholder to update the state (this will be replaced with API call to POST, and then showing the list will be the result of the API call to GET)

[codecov]

Codecov Report

Merging #992 (8fd7211) into feature/flint-UI (24cdd19) will decrease coverage by 0.04%.
Report is 1 commits behind head on feature/flint-UI.
The diff coverage is n/a.

@@                 Coverage Diff                  @@
##           feature/flint-UI     #992      +/-   ##
====================================================
- Coverage             43.90%   43.87%   -0.04%     
====================================================
  Files                   319      323       +4     
  Lines                 18796    18862      +66     
  Branches               4581     4591      +10     
====================================================
+ Hits                   8253     8275      +22     
- Misses                 9987    10030      +43     
- Partials                556      557       +1     

Flag	Coverage Δ
dashboards-observability	43.87% <ø> (-0.04%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

see 8 files with indirect coverage changes

[ps48]

I think we can make it more readable or change it to a function.

const changeMode = () => { 
     mode === 'view' ? setMode('edit') : setMode('view');
}
onClick = {changeMode}

OR 

onClick = { () => setMode(mode === 'view' ? 'edit' : 'view')}

[derek-ho]

Done in: fc96304, thanks!

[ps48]

Minor, but Ideally types here should be something like: React.Dispatch<React.SetStateAction<Array<{ label: string }>>

[derek-ho]

Done in: fc96304, thanks! I didn't know this

[ps48]

Minor, this can be changed to mode === 'edit' && <view/> no need to use null.

[derek-ho]

Done in: fc96304, thanks!

[pjfitzgibbons]

Please turn this into a formal React FC.
This is a quality issue for us throughout our plugin.

[derek-ho]

I tried to address this in: fc96304, please let me know if I understood what you meant correctly.

[pjfitzgibbons]

Please do not call these components "...FlexItem". The component's purpose is not a consequence of it's inclusion in a flex-group.

[derek-ho]

Agree. Done in: fc96304, thanks for the suggestion.

[pjfitzgibbons]

Consider setting this value to a component-constant (AccessControlDetails). You have several of these conditions - they can all be collected into one place... thereby reducing cyclomatic complexity.

Containing these conditions into one location will also raise the visibility of refactoring / simplification opportunities.

[derek-ho]

Not quite sure I understand what you are suggesting/how it would be cleaner than having a state variable. Can you point me to some examples I can look at?

[pjfitzgibbons]

Consider moving this condition to a component-constant. Name it something that tells the story about what it is / why it's there.

We are left to imagine the meaning of selectedRadio === QUERY_RESTRICT... often the wrong meaning.

[derek-ho]

Not quite sure I am following what you are suggesting here. The issue is that right now, the radio groups and modifying the radio groups need to be passed down from the parent to this specific component, but this component also needs to know some specifics about the radio groups in order to render correctly. The reason I need to do this is because the parent component needs to know the state of this child component to fire off the API call to update the data connection (it renders the component that is connected to the firing of this call). What are your thoughts around how we should approach this given these limitations?

[pjfitzgibbons]

I think the story here is "Access Levels".

In the "parent" radio-gruop :

<EuiRadioGroup
  options={acessLevelOptions}
  idSelected={selectedAccessLevel}
  onChange={setSelectedAccessLevel}
...

Then the condition of the next component is way more instructive :

{ selectedAccessLevel === QUERY_RESTRICT && (< the content>) }

(no need to use a tertiary here... render() fn knows how to deal with embedded nulls.

[pjfitzgibbons]

I think the story here is "Access Levels".

In the "parent" radio-gruop :

<EuiRadioGroup
  options={acessLevelOptions}
  idSelected={selectedAccessLevel}
  onChange={setSelectedAccessLevel}
...

Then the condition of the next component is way more instructive :

{ selectedAccessLevel === QUERY_RESTRICT && (< the content>) }

(no need to use a tertiary here... render() fn knows how to deal with embedded nulls.

[pjfitzgibbons]

Without re-throwing, you're effectively swallowing all other errors except 403. Is that really what we want?
If any/ever error should be met with setHasAccess(false), then just set it without condition.
This might be a case to wrap this in a function so you can tell a story checkStatusForbiddenOrThrow(err).

[pjfitzgibbons]

I still very much recommend renaming to availableRoles, selectedRoles, setSelectedRoles
Future implemention might be checkbox list.

[pjfitzgibbons]

selectedAccessLevel