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

Basic LDAP support for domain authentication #1159

Open
wants to merge 40 commits into
base: master
Choose a base branch
from

Conversation

Armored-Dragon
Copy link
Member

@Armored-Dragon Armored-Dragon commented Oct 10, 2024

This is a implementation for using LDAP services as an alternative authentication method to WordPress for domain servers.

Closes #905 as complete.
Closes #906 as complete.

General TODO:

  • Fix LoginDialog.qml style issues.
  • LDAP server groups as roles.
  • LDAPAccount.cpp (better) error handling.
  • LDAPAccount.cpp expose setting server url.
  • Fix LDAP accounts with capital letters can not sign in.
  • Documentation.
  • Include LDAP library.

Out of scope TODO. (A follow up pull request will be opened to address these issues)

Signed-off-by: armored-dragon <[email protected]>
@Armored-Dragon Armored-Dragon self-assigned this Oct 10, 2024
@Armored-Dragon

This comment was marked as outdated.

pull bot and others added 17 commits October 24, 2024 06:32
* mirrors wip

* fix view + projection, texture flipping, billboarding

* wip portals

* wip

* fix cpu frustum culling (hacky?)

* fix mirrors in deferred

* mirrors on models + text

* portals use exit as ignoreItem

* cleanup

* entity tags

* wild guess to handle view correction, hide portalExitID in create when mirrorMode != portal

* let's try this??

* plz

* promising

* fix paramsOffset and view flipping

* portals shouldn't flip

* break when tag found

* fix portal view calculation

* Revert "Mirrors + Portals"

* Revert "Revert "Mirrors + Portals""

* web entity wantsKeyboardFocus property

* fix typo

* move audio zones to zone entity properties

* fix audio zones in create

* set dynamic factory

* new procecural particle entity type

* fix particle intersection

* shorten create labels

* fix 0 update props case

* Ability to smooth model animations

* sound entities

* fix layered simulate items

* fix stereo sound speed

* support non-localOnly sound avatar entities

* add sound url prompt

* support registration point, improve locking

* remove keyboardRasied

* locking attempt #2

* fix keyboardRasied typo

* add default particle props

* add unlit property for shapes

* Merge branch master into protocol_changes

* add ambient light color

* fix create issue

* fix create issue

* add tonemapping props to zones, wip ambient occlusion

* wip ambient occlusion

* it's working!

* remove attachments

* fix non-localOnly positional sounds not updating

* change AO default to HBAO, remove from create

* more graphics options

* fix AO setting + effects in mirrors

* fix AA in mirrors

* alezia's fixes

* fix haze in mirrors

* add comment for SKYBOX_DISTANCE

* new line

* model loading priority updates over time, takes into account out of bounds, avatar entities have higher priority, and fsts can specify to wait for wearables to load before rendering

* add loadPriority to model entities, working on other avatars waitForWearables

* fix build error

* try to fix isServer assert

* fix stats + waitForWearables

* Listen for click instead of release.

* Reverted initial commit. Implemented hack to listen for menu click events.

* Missed some reverts.

* Missed another one.

* Prevent duplicate actions.

* Added extra needed checks.

* Fix without formatting? (#91)

* Hopefully fixed formatting.

* Things can't be too easy.

* Remove google poly

* automated entity property serialization

* cleanup + automate EntityPropertyFlags

* text vertical alignment, use uint8_t for entity property enums, fix text recalculating too often

* fix text size

* Update interface/resources/controllers/keyboardMouse.json

Co-authored-by: HifiExperiments <[email protected]>

* fix component mode serialization

* Fixed mouse look in selfie mode.

* fix text debug assert on invalid or unloaded font

* missed some enums

* fix ADD_GROUP_PROPERTY_TO_MAP

* fix PROP_GRAB_EQUIPPABLE_INDICATOR_URL missing urlPermission

* fix KeyLightPropertyGroup legacy properties

* fix PolyLineEntityItem::getEntityProperties

* comment cmake script

* fix copyright

* Replaced key value with key text.
Added additional comment about the specific delete key used.

* weekly promoted place

Highlight the first place in the list as the weekly promoted place

* Fixed lingering references to `avatarIcon`.

Signed-off-by: armored-dragon <[email protected]>

* Adding icon for "Grab And Equip" section

Adding icon for "Grab And Equip" section

* Add "Grab And Equip" section

Add "Grab And Equip" section for the grabbale and Equipable groups of properties.

* Add files via upload

* Add tooltips for the "Grab and Equip" properties

Add the tooltips for the "Grab and Equip" properties

* Text adjustments for grab.equippable

Text adjustments for grab.equippable

* Make Maturity Filter persisted

Make Maturity Filter persisted and with a default value (Teen & Everyone)

* Adjust the default value for maturity

Adjust the default value for maturity

* move "triggerable" under GRAB & EQUIP

move "triggerable" under GRAB & EQUIP

* Remove hifi-screenshare
Cherry picked and updated from Tivoli dd5b6ea6ee5597a06603e16509640e7ed18106bb

Co-authored-by: Julian Groß <[email protected]>

* Insert placeholder to not break protocol yet.

* Fix incorrectly resolved merge conflict, left too much code.

* Fixes based on review comments on previous PR

* Remove code accidentally re-added during a conflict fix

* bump protocol

* rebuild fonts with full charset (NOT -allglyphs)

* Attempt at fixing Windows master branch builds

* Change minimum angular velocity to a lower one

* Fix Uuid.NULL behavior

---------

Signed-off-by: armored-dragon <[email protected]>
Co-authored-by: HifiExperiments <[email protected]>
Co-authored-by: ksuprynowicz <[email protected]>
Co-authored-by: Dale Glass <[email protected]>
Co-authored-by: HifiExperiments <[email protected]>
Co-authored-by: Julian Groß <[email protected]>
Co-authored-by: armored-dragon <[email protected]>
Co-authored-by: Armored-Dragon <[email protected]>
Co-authored-by: Alezia Kurdis <[email protected]>
Co-authored-by: Maki <[email protected]>
Co-authored-by: Dale Glass <[email protected]>
Signed-off-by: armored-dragon <[email protected]>
Signed-off-by: armored-dragon <[email protected]>
Signed-off-by: armored-dragon <[email protected]>
Signed-off-by: armored-dragon <[email protected]>
Signed-off-by: armored-dragon <[email protected]>
Signed-off-by: armored-dragon <[email protected]>
Signed-off-by: armored-dragon <[email protected]>
@Armored-Dragon Armored-Dragon added the help wanted Extra attention is needed label Oct 25, 2024
@Armored-Dragon
Copy link
Member Author

Help wanted as I have absolutely no idea how to get things to build with CMake.

Removed outdated TODO/FIXMEs.
(Hopefully) fixed describe-settings.json regression.

Signed-off-by: armored-dragon <[email protected]>
Signed-off-by: armored-dragon <[email protected]>
@Armored-Dragon Armored-Dragon marked this pull request as ready for review October 28, 2024 12:01
@JulianGro
Copy link
Member

About OpenLDAP on Windows: While not open-source, the Windows version does have a fully free license, so we could use it. It's a bit annoying that it isn't open-source, but personally I would be fine with having this.

@Armored-Dragon
Copy link
Member Author

About OpenLDAP on Windows: While not open-source, the Windows version does have a fully free license, so we could use it. It's a bit annoying that it isn't open-source, but personally I would be fine with having this.

I would greatly prefer we try and stick to open source only, but if there is not an alternative I won't stand in the way of it. In the future we can probably try and work something out to replace it with a open source version.

@Armored-Dragon
Copy link
Member Author

@ksuprynowicz I'm requesting a review from you since you have experience with LDAP systems. Let me know if I've done something silly!

@JulianGro
Copy link
Member

I wonder how hard it would be to compile this on Windows anyway. My understanding is that we only use the client library, while OpenLDAP is a suite also containing servers and utilities. It looks like we are building pretty much all of that. make also seems available on Windows through VCPKG.
It looks like the server can be disabled using --disable-slapd.
https://bin.linux.pizza/?0798efe40ebcb0d3#3UBwrzp2HhwD9WQRgB2f6NSUChUxU4GyeS4orkm8FDm5

@ksuprynowicz
Copy link
Member

@JulianGro There's an example of how to build it on Windows here:
https://github.com/python-ldap/python-ldap
It involves a pretty big patch file, but seems possible:
https://github.com/cgohlke/python-ldap-build/blob/main/openldap.diff

@@ -49,10 +49,10 @@ class NodePermissions {
NodePermissionsKey getKey() const { return NodePermissionsKey(_id, _rankID); }

// the _id member isn't authenticated/verified and _username is.
void setVerifiedUserName(QString userName) { _verifiedUserName = userName.toLower(); }
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we should keep user names case-insensitive? On LDAP typically DNs are case-insensitive too.

Copy link
Member Author

Choose a reason for hiding this comment

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

I just revered the changes in the case sensitivity commit.

Copy link
Member

@ksuprynowicz ksuprynowicz left a comment

Choose a reason for hiding this comment

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

Awesome!
Almost everything looks good, I only have a question related to possible memory leaks.

domain-server/src/DomainGatekeeper.cpp Outdated Show resolved Hide resolved
domain-server/src/DomainGatekeeper.cpp Show resolved Hide resolved
interface/src/ui/LoginDialog.cpp Outdated Show resolved Hide resolved

if (normalizedType == "ldap") return DependencyManager::get<DomainAccountManager>()->requestAccessToken(username, password, "ldap");
if (normalizedType == "wordpress") return DependencyManager::get<DomainAccountManager>()->requestAccessToken(username, password, "wordpress");

Copy link
Member

Choose a reason for hiding this comment

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

Maybe we should add a warning for when type was neither "ldap" nor "wordpress"?

Copy link
Member Author

@Armored-Dragon Armored-Dragon Jan 30, 2025

Choose a reason for hiding this comment

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

I think it would make more sense to add that warning in the DomainAccountManager. From where LoginDialog is I think there will always be a valid value provided as it is supplied from the user from a drop-down selection element.
If there is an issue in this part of the code, I would think it should have either been prevented in the LinkAccountBody, or it should be caught at the ultimate function trying to be reached.

libraries/networking/src/DomainAccountManager.cpp Outdated Show resolved Hide resolved
libraries/networking/src/DomainAccountManager.cpp Outdated Show resolved Hide resolved
libraries/networking/src/LDAPAccount.cpp Outdated Show resolved Hide resolved
libraries/networking/src/LDAPAccount.cpp Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed NLnet work in progress Do not merge yet
Projects
Status: In Progress
4 participants