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

LdapExplorer front end #7683

Merged
merged 50 commits into from Jul 7, 2023

Conversation

VakarisZ
Copy link
Contributor

@VakarisZ VakarisZ commented May 31, 2023

Description

  1. Changes the "Authentication rules" in "Authentication Sources" UI page to utilize the LDAPExplorer. This allows fetching LDAP parameters dynamically, without the need to manually verify their existence.
  2. (In-progress) Fetches ldap parameters to be used as filters

Impacts

Look at "Authentication rule" "Conditions".

Code / PR Dependencies

Depends on #7634

NEW Package(s) required

Added packages related to the new webpack version not supporting polyfill

Issue

? internal ?

Delete branch after merge

YES

Checklist

(REQUIRED) - [yes, no or n/a]

  • Document the feature
  • Add unit tests
  • Add acceptance tests (TestLink)

After merge

Create an issue for replacing these UI inputs with ldap attribute selector:
image
Also, point out that we need to remove
image

@VakarisZ VakarisZ force-pushed the Feature/ldapexplorer_front_end branch from 8cd05ee to 603ce03 Compare June 5, 2023 13:01
@VakarisZ VakarisZ force-pushed the Feature/ldapexplorer_front_end branch from 97a68a1 to 0eccb89 Compare June 22, 2023 14:34
@VakarisZ VakarisZ marked this pull request as ready for review June 22, 2023 14:55
@extrafu extrafu added this to the PacketFence-13.0 milestone Jun 27, 2023
@@ -80,6 +81,7 @@ const props = {
}

const setup = (props, context) => {
const conditionsComponent = inject(ProvidedKeys.conditionsComponent, components.FormGroupConditions)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is there a better way to pass the different conditions component?

Comment on lines +74 to +83
const item = await Promise.resolve(state.cache[id]).then(cache => _.cloneDeep(cache))
return filterOutLdapOptions(response, item.type)
} else {
const item = await api.item(id).then(item => {
commit('ITEM_REPLACED', item)
return _.cloneDeep(item)
}).catch((err) => {
commit('ITEM_ERROR', err.response)
throw err
})
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 is basically getAuthenticationSource, but I couldn't find a way to reuse it. We should be able to just do this.getAuthenticationSource if we use defineStore from pinia

@VakarisZ VakarisZ force-pushed the Feature/ldapexplorer_front_end branch from f2ecbfe to 1f3dd90 Compare June 29, 2023 11:30
Copy link
Contributor

@satkunas satkunas left a comment

Choose a reason for hiding this comment

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

I think we should disable the "Add LDAP Condition" button if any of host, port, bind dn or password is empty.

We should also add :text to all the above inputs to indicate something like "This field is required for LDAP conditions".

Comment on lines 21 to 27
BaseInput,
BaseInputChosenMultiple,
BaseInputChosenOne,
BaseInputGroupMultiplier,
BaseInputNumber,
BaseInputPassword,
BaseInputRange
Copy link
Contributor

Choose a reason for hiding this comment

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

None of these except BaseInputChosenOne are returned in valueComponent, so the imports are not needed

pfComponentType() {
return pfComponentType
},
},
Copy link
Contributor

Choose a reason for hiding this comment

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

computed declaration is using Vue2 pattern, can pfComponentType() be moved to setup?

Comment on lines 39 to 40
default: () => {
},
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove the \n in the middle of {}

Copy link
Contributor

Choose a reason for hiding this comment

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

More cases below. Please change all instances.

let parsedEntries = new Set()
for (let i = 0; i < ldapEntries.length; i++) {
let value = ldapEntries[i][ldapAttribute]
if (_.isArray(value)){
Copy link
Contributor

Choose a reason for hiding this comment

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

Array.isArray() is good enough here, no need to create deps on lodash where they're not needed

const searchInput = ref("")
const localValidator = inject('schema')
Copy link
Contributor

Choose a reason for hiding this comment

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

Use single quotes

Copy link
Contributor

Choose a reason for hiding this comment

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

More cases below. Instead of cluttering the review, update all JS files to single quotes where possible.

@satkunas
Copy link
Contributor

satkunas commented Jun 29, 2023

The (+) icon-button should be re-added to each row of a condition (either PF or LDAP), and re-add support for CTRL+click to clone the row.

@satkunas
Copy link
Contributor

We need some kind of indicator on each row to quickly distinguish between pf and ldap conditions.

IMO a <b-badge> with either "pf" or "ldap" in the left-most column (w/ index) would be simplest.

@VakarisZ
Copy link
Contributor Author

I think we should disable the "Add LDAP Condition" button if any of host, port, bind dn or password is empty.

I don't see what we gain by disabling the button. If we want a form where the workflow is 1. configure ldap connection 2. check connection 3. if connected proceed, we should make this a multipart form (gray out whole rules section or hide it). If we gray out a button we have to explain why it's grayed out. The dropdown select already has a good place to do it.
Also, bind dn and password can be empty if the ldap server is configured to accept anonymous bind?

We should also add :text to all the above inputs to indicate something like "This field is required for LDAP conditions".

The fields are required anyways. The users should go down filling out the required fields. Ideally, the bind dn and password should be up the list, right after the required values.

@satkunas
Copy link
Contributor

Also, bind dn and password can be empty if the ldap server is configured to accept anonymous bind?

I didn't realize these could be empty. Disregard my comments for the required fields.

By default the values are ints, but they get converted to string upon save/modification. This creates inconsistency to what the back end receives
@satkunas satkunas force-pushed the Feature/ldapexplorer_front_end branch from 811931d to 0087957 Compare July 6, 2023 19:29
@satkunas
Copy link
Contributor

satkunas commented Jul 6, 2023

@VakarisZ

I was able to rebase w/ devel (inc ldapexplorer_back_end) to get the backend service running, but the httpd.admin_dispatcher is not proxying /api/v1/ldap/search and instead returns a 404

Unless I'm mistaken, there appears to be a missing directive in conf/caddy-services/api.conf, this was likely missed in the backend PR, and it needs to copied to conf/caddy-services/api.conf.example (.example are renamed w/ fresh install)

I tried the following, but wasn't able to verify that it works

diff --git a/caddy-services/api.conf b/caddy-services/api.conf
index 6f0f543..3b70c71 100644
--- a/caddy-services/api.conf
+++ b/caddy-services/api.conf
@@ -49,6 +49,11 @@
     transparent
   }

+  # proxy LDAP explorer
+  proxy /api/v1/ldap/ {$PF_SERVICES_URL_PFLDAPEXPLORER} {
+    transparent
+  }
+

Please adjust devel so the endpoint works and rebase this PR. I'll try this PR again after it's updated.

@satkunas satkunas merged commit c484f98 into inverse-inc:devel Jul 7, 2023
1 check passed
satkunas added a commit that referenced this pull request Jan 11, 2024
* Core changes to implement LdapExplorer UI input

* fix validation

* Fix validation of search input

* Fix ldap search input states

* Extract multiselect style to a separate file

* Move and cleanup ldap search UI files

* Fix linting errors

* Remove trailing ; in LdapSearchInput.vue

* Add npm requirements for latest node and webpack-dev-server

* Core changes to fetch attributes from an ldap server

* Add lodash javascript package for convenience functions it offers

* Extract value to select value conversion function

* Add a draggable component that has static buttons

* Differentiate ldap attribute fetching based on server type

* Revert changes to BaseRuleCondition.vue related to ldap

* Implement the core UI for ldap conditions

* Add UI for ldap attribute selector

* Debounce ldap search input

* Add loading prop to multiselectFacade.vue

* Fix minor issues with ldap condition UI

* Add error feature to ldap attribute selector UI

* Make LDAP attribute selector check connection based on form

* Use quiet requests in background ldap searches

* Add infrastructure to test the OpenLdap

* Add openLdap server attribute extraction logic

* Fix server parameter in LdapSearchInput.vue

* Fix initial value bug in the LdapSearchInput.vue

* Add not connected error to ldap value search

* Improve ldapMultiselectFacade to ignore options when no connection

* Adjust ldap search to parse array values

* Extract common ldap request function between ldap server clients

* Rename some ldap UI files and variables

* Fix eslint errors in ldap code

* Extract useLdapAttributes into composable

* Inject ldap condition only in ldap authentication sources

* Remove ldap condition from GoogleWorkspaceLDAP

* Add ldap selector to Edir conditions

* Fix a bug that sends form with int values if they are unmodified

By default the values are ints, but they get converted to string upon save/modification. This creates inconsistency to what the back end receives

* Add a common array for supported form types by ldap explorer

* Fix a bug where unsupported forms were trying to use ldap explorer

* Add a key for provided conditions component

* Filter out ldap options in packetfence conditions

* Add missing translations related to ldap explorer

* Add clone condition button in ldap conditions

* Change the ldap condition to be able to clone and append conditions

* Add badges to differentiate conditions by type

* Reformat and improve style of ldap explorer UI

* Extract ldap error to a separate component

* Show ldap error in all cases when connection fails

* force update semver to address syyk vulnerability

---------

Co-authored-by: Darren Satkunas <dsatkuna@akamai.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants