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.

Sign up for GitHub

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

Jump to bottom

GUACAMOLE-1881: Adding new standard token LDAP_DOMAIN by extracting from user credentials #931

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

GUACAMOLE-1881: Adding new standard token LDAP_DOMAIN by extracting from user credentials #931

wants to merge 1 commit into from

Conversation

josnabattula
Copy link

@josnabattula josnabattula commented Nov 15, 2023 •
edited

When user log-in into Guacamole when multiple LDAP's enabled with one of the configuration in ldap-server.yml
match-usernames: - DOMAIN1\\(.*) - (.*)@domain1\.example\.net
In this pull request we are extracting domain name into custom variable LDAP_DOMAIN. this can be used for connections

@josnabattula josnabattula deleted the jira/1881 branch January 15, 2024 23:47
@josnabattula josnabattula restored the jira/1881 branch January 15, 2024 23:50
@josnabattula josnabattula reopened this Jan 15, 2024
@josnabattula
Copy link
Author

@mike-jumper @necouchman could you look into my pull request, as I have move the changes into LDAP module as per the comments.

necouchman
necouchman requested changes Feb 9, 2024
View reviewed changes
Copy link
Contributor

@necouchman necouchman left a comment

Choose a reason for hiding this comment

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

A couple of more tweaks and I think it'll be ready to go.

...le-auth-ldap/src/main/java/org/apache/guacamole/auth/ldap/AuthenticationProviderService.java Outdated Show resolved Hide resolved
...le-auth-ldap/src/main/java/org/apache/guacamole/auth/ldap/AuthenticationProviderService.java Outdated Show resolved Hide resolved
@josnabattula
Copy link
Author

@necouchman I have resolved your comments.

@josnabattula
Copy link
Author

@necouchman @mike-jumper I have resolved all the comments. Can I get your tick?

@josnabattula
Copy link
Author

@necouchman I have resolved your comments. please have a review. thanks.

@josnabattula
Copy link
Author

@necouchman if you have a minute, could you have look over this again, resolved all comments. Good to get this merged, waiting for this feature for a wee bit of time.

@josnabattula
Copy link
Author

@mike-jumper I have resolved your comments, could you review when you get a minute. Thanks.

necouchman
necouchman requested changes Apr 7, 2024
View reviewed changes
Copy link
Contributor

@necouchman necouchman left a comment

Choose a reason for hiding this comment

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

Overall I think the code is okay, but there are a few style fix-ups to do, here, to make it consistent with the rest of the code

...le-auth-ldap/src/main/java/org/apache/guacamole/auth/ldap/AuthenticationProviderService.java Outdated Show resolved Hide resolved
...le-auth-ldap/src/main/java/org/apache/guacamole/auth/ldap/AuthenticationProviderService.java Show resolved Hide resolved
...le-auth-ldap/src/main/java/org/apache/guacamole/auth/ldap/AuthenticationProviderService.java Show resolved Hide resolved
...le-auth-ldap/src/main/java/org/apache/guacamole/auth/ldap/AuthenticationProviderService.java Outdated Show resolved Hide resolved
...le-auth-ldap/src/main/java/org/apache/guacamole/auth/ldap/AuthenticationProviderService.java Outdated Show resolved Hide resolved
@necouchman
Copy link
Contributor

@josnabattula You need to remove the merge commits from this PR - there are currently two:
ca241ab
26f2116

Also, you might want to consider squashing the others together into a smaller number - 9 commits seems a bit much for 60 lines of code change ;-).

@josnabattula josnabattula changed the title GUACAMOLE-1881: Adding new standard tokens userid and domain name GUACAMOLE-1881: Adding new standard token LDAP_DOMAIN by extracting from user credentials Apr 9, 2024
@josnabattula
Copy link
Author

@necouchman I have removed merge commits and squashed other commits also.

necouchman
necouchman approved these changes Apr 9, 2024
View reviewed changes
Copy link
Contributor

@necouchman necouchman left a comment

Choose a reason for hiding this comment

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

I'm good with it. @mike-jumper, any further changes you'd like to see?

fanovilla
fanovilla approved these changes Apr 11, 2024
View reviewed changes
Copy link

@fanovilla fanovilla left a comment

Choose a reason for hiding this comment

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

looks good Josna!

@josnabattula
Copy link
Author

@mike-jumper When you get some time, could you review this pull request? thanks.

@josnabattula
Copy link
Author

@mike-jumper Can you review this request, we are waiting for this feature for a while. thanks.

@josnabattula
Copy link
Author

@mike-jumper could you review this pull request, I have resolved your comments. thanks.

@mike-jumper
Copy link
Contributor

Yes, I'll take a look.

@mike-jumper mike-jumper mentioned this pull request May 15, 2024
Open
@josnabattula
Copy link
Author

@mike-jumper I can close this pull request once superseded by this PR #987 is merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@necouchman necouchman necouchman approved these changes

@fanovilla fanovilla fanovilla approved these changes

@mike-jumper mike-jumper Awaiting requested review from mike-jumper

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
4 participants
@josnabattula @necouchman @mike-jumper @fanovilla