Skip to content
This repository has been archived by the owner on Apr 17, 2023. It is now read-only.

ldap: fixed a couple of bugs around SSL support #1817

Merged
merged 1 commit into from
May 11, 2018

Conversation

mssola
Copy link
Collaborator

@mssola mssola commented May 10, 2018

This commit fixes a couple of bugs present in both master and 2.3:

  1. We didn't implement some options that needed to be passed to the LDAP
    backend to fully support SSL connections. This has been addressed
    also in the configuration, but without breaking existing
    installations (e.g. the method attribute from 2.3 has been left
    untouched). This will be addressed in later commits of the master
    branch (so in 2.4 users should adapt to this change).
  2. We were relying on Devise's translations for failures, but some of
    them were not available. This has been addressed and improved: the
    error message will be more on point and more informative to end
    users.

There is still room for improvement, but we can do it in later commits:
let's keep this commit to the point so it can be cherry-picked into the
2.3 branch.

Fixes #1746
Fixes #1774

bsc#1073232

Signed-off-by: Miquel Sabaté Solà [email protected]

@mssola mssola requested a review from vitoravelino May 10, 2018 15:31
@mssola
Copy link
Collaborator Author

mssola commented May 10, 2018

Note that I've left lots of TODOs here and there, as reminders of changes that I've planned (plus some other ones). I'll do it in other PRs, since now I want a commit that can be cherry picked into the 2.3 branch.

I haven't tested this commit on top of the 2.3 branch yet, so don't merge it (but you can review it meanwhile).

@mssola mssola force-pushed the ldap-fixes branch 2 times, most recently from fd8df5c to e42dedf Compare May 11, 2018 07:39
This commit fixes a couple of bugs present in both master and 2.3:

1. We didn't implement some options that needed to be passed to the LDAP
   backend to fully support SSL connections. This has been addressed
   also in the configuration, but without breaking existing
   installations (e.g. the `method` attribute from 2.3 has been left
   untouched). This will be addressed in later commits of the master
   branch (so in 2.4 users should adapt to this change).
2. We were relying on Devise's translations for failures, but some of
   them were not available. This has been addressed and improved: the
   error message will be more on point and more informative to end
   users.

There is still room for improvement, but we can do it in later commits:
let's keep this commit to the point so it can be cherry-picked into the
2.3 branch.

Fixes SUSE#1746
Fixes SUSE#1774

bsc#1073232

Signed-off-by: Miquel Sabaté Solà <[email protected]>
@mssola
Copy link
Collaborator Author

mssola commented May 11, 2018

Tests are passing if I apply this patch into the 2.3 branch. Moreover, I've tested it locally and it also works, so I'd say it's ready to be reviewed and merged whenever you say so 😉

Copy link
Contributor

@vitoravelino vitoravelino left a comment

Choose a reason for hiding this comment

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

I don't have knowledge around LDAP but tests are passing and code wise LGTM.

@vitoravelino vitoravelino merged commit 1e5161e into SUSE:master May 11, 2018
@mssola mssola deleted the ldap-fixes branch May 11, 2018 13:14
@mssola
Copy link
Collaborator Author

mssola commented May 11, 2018

Cherry picked into the v2.3 branch.

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

Successfully merging this pull request may close these issues.

translation missing: en.devise.failure.user.ldap_bind_failed LDAP not working with Portus 2.3.1
2 participants