-
Notifications
You must be signed in to change notification settings - Fork 74
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
Fixes #24230: Authentication providers and role mapping settings should be exposed #5408
Conversation
…gs should be exposed Fixes #24230: Authentication providers and role mapping settings should be exposed
af6a824
to
9a18100
Compare
… settings should be exposed Fixes #24230: Authentication providers and role mapping settings should be exposed
PR updated with a new commit |
def update(updated: Vector[UserInfo]): ConnectionIO[Int] = { | ||
// never update the managedBy of an existing user which may have been provided by another origin | ||
val sql = | ||
"""insert into users (id, creationdate, status, managedby, name, email, lastlogin, statushistory, otherinfo) | ||
values (?,?,?,?,? , ?,?,?,?) | ||
on conflict (id) do update | ||
set (creationdate, status, managedby, name, email, lastlogin, statushistory, otherinfo) = | ||
(EXCLUDED.creationdate, EXCLUDED.status, EXCLUDED.managedby, EXCLUDED.name, EXCLUDED.email, EXCLUDED.lastlogin, EXCLUDED.statushistory, EXCLUDED.otherinfo)""" | ||
set (creationdate, status, name, email, lastlogin, statushistory, otherinfo) = | ||
(EXCLUDED.creationdate, EXCLUDED.status, EXCLUDED.name, EXCLUDED.email, EXCLUDED.lastlogin, EXCLUDED.statushistory, EXCLUDED.otherinfo)""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The update of the managedby
field can break available information about the provider of existing users.
We should not be allowed to replace existing users' managedby
value because it is subject to very specific user-management logic e.g. a user can exist in the users static configuration file but also be managed by an external provider
…mapping settings should be exposed Fixes #24230: Authentication providers and role mapping settings should be exposed
PR updated with a new commit |
…d role mapping settings should be exposed Fixes #24230: Authentication providers and role mapping settings should be exposed
PR updated with a new commit |
final case class AuthBackendProviderProperties private ( | ||
hasAdditionalRoles: Boolean, | ||
overridesRoles: Boolean, | ||
hasModifiablePassword: Boolean |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the logic here is not clear (or I don't understand it).
LDAP does not have updatable password by rudder: they are managed by the LDAP directory, out of Rudder domain of responsibility.
But if we have a configuration with provider = ldap, file
, ie first trying LDAP, then trying file, then we want to let the user password for the file case be updatable.
But file
is not defined as an AuthBackendProviderProperties
. Why so ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe the hasModifiablePassword
should be renamed canProviderBeMigratedToFile
because that is what happens with LDAP...
I didn't really want to introduce this boolean, but its sole purpose is to allow the UI to "force" a password for an external provider (which LDAP is), as in this Elm code : https://github.com/clarktsiory/rudder-plugins/blob/bug_24219/display_user_detail_using_information_from_database/user-management/src/main/elm/sources/View.elm#L144.
Indeed with provider = ldap, file
we want to let the user password for the file case be updatable, but we don't want it to be updatable with provider = oidc, file
(unless I was misunderstanding the spec), so a boolean attribute for making distinction between both would be actually needed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
file
is always handled very specifically even in the UI, so I didn't want to abstract all the properties that make the distinction between file and other providers, and I guess there are plenty...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you can completely have provider = oidc, file
with updatable passwords safe oidc provisioning is set to true (for at least one provider, because I don't think can know from which oidc provider users come... Which is perhaps a problem in fact - for that case, and for the case where a new OIDC provider is added or an old one removed...).
Ok, appart that new interesting subject: an oidc provider without auto-provisioning is almost exactly like LDAP conceptually: user are declared in rudder-users.xml
, but the authentication is done by some other entity.
And LDAP could, one day, with lots of incentives, get an auto-provisioning mode.
ApplicationLogger.error( | ||
s"Backend provider properties for '${name}' are not consistent: backend does not enables providing roles but roles overrides is set to true. " + | ||
s"Overriding roles will not be enabled." | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you need to do that kind of check, then it means that you are not enforcing "Make invalid states unrepresentable", which we really want to enforce as much as possible.
You likely want an ADT like:
sealed trait RoleExtension
object RoleExtension {
case object None extends RoleExtension
case object NoOverride extends RoleExtension
case object WithOverride extends RoleExtention
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Plus with such an AST, it's easy to build an order relationship (for ex, add a priority index sealed trait RoleExtension { def index: Int }
and compare on it for the priority rules, or take the max, etc)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 thanks for the suggestion, I would only need to move the existing definition from the user-management plugin to here in Rudder : Normation/rudder-plugins@b8cd1a9#diff-50791aeada8e5190976dae763b438a4d990a8a3bbcf23b7e063a7c9ac11274c0R94-R101
Even with the feedbacks, I will merge that so that it's easier to iterate (it becomes complicated with severa PR here and on plugin). Opening an other ticket to track needed enhancements. |
https://issues.rudder.io/issues/24230
We need to know providers properties e.g. : Do they override or extend the user roles defined in the
users.xml
file ? Can they write password in the users file ?These properties will be used in user-management plugin to handle some providers logic regarding roles and also for the logic of the UI : Normation/rudder-plugins#668