-
Notifications
You must be signed in to change notification settings - Fork 24
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 #22349: Update user plugin to manage update custom roles #536
Fixes #22349: Update user plugin to manage update custom roles #536
Conversation
file <- getUserFilePath | ||
parsedFile <- IOResult.attempt(ConstructingParser.fromFile(file.toJava, preserveWS = true)) | ||
userXML <- IOResult.attempt(parsedFile.document().children) | ||
toUpdate = (userXML \\ "authentication").head |
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 don't think this is 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.
👍
@@ -105,23 +105,23 @@ update msg model = | |||
AddUser result -> | |||
case result of | |||
Ok username -> | |||
(model, postReloadConf model ) | |||
(model, getUsersConf model ) |
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.
we don't need to ask for a reload, it's the role of the backend to ensure backend logic. In fact, the the API return value should be enough to update the UI as wanted.
@@ -56,15 +58,15 @@ object UserManagementLogger extends Logger { | |||
|
|||
object Serialisation { | |||
|
|||
implicit class AuthConfigSer(auth: UserDetailList) { | |||
implicit class AuthConfigSer(auth: ValidatedUserList) { |
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.
changes in rudder core
def toJson: JValue = { | ||
val encoder: String = PassEncoderToString(auth) | ||
val authBackendsProvider = RudderConfig.authenticationProviders.getConfiguredProviders().map(_.name).toSet | ||
|
||
val jUser = auth.users.map { | ||
case (_, u) => | ||
val (rs, custom) = { | ||
UserManagementService.computeRoleCoverage(Role.values, u.authz.authorizationTypes).getOrElse(Set.empty).partition { | ||
UserManagementService.computeRoleCoverage(RudderRoles.getAllRoles.runNow.values.toSet, u.authz.authorizationTypes).getOrElse(Set.empty).partition { |
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.
need to ask for all roles including custom roles now
def compareRights(r: Role): Option[Role] = { | ||
if (r.name == "no_rights") { | ||
def compareRights(r: Role, as: Set[AuthorizationType]): Option[Role] = { | ||
if (r == Role.NoRights) { |
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.
we have type, use them. We should not revert to string comparison appart for very good cases (check at system boudaries: parsing, serialisation, etc)
None | ||
} else if (authzs.contains(AuthorizationType.AnyRights)) { |
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.
missing that case, which is now necessary given changes in rudder core
Fixes #22349: Update user plugin to manage update custom roles
PR updated with a new commit |
val resources: Either[UserConfigFileError, UserFile] = UserFileProcessing.getUserResourceFile() | ||
resources match { | ||
case Left(err) => | ||
Left(err) |
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.
when you see that you write case Left(err) => Left(err)
, you want to just map
case cr if cr == roleAuthzNames => Some(r) | ||
case cr if cr.nonEmpty => | ||
val test = cr.flatMap(RoleToRights.parseAuthz) | ||
if (test.isEmpty) { |
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.
test can't be empty, there is a check on that in the pattern match (not exactly, but there is no need to parse either, we just have to avoid to map(_.id)
and remains with the types)
} | ||
} | ||
} | ||
|
||
if (authzs.isEmpty || roles.isEmpty || authzs.exists(_.id == "no_rights")) { | ||
if (authzs.isEmpty || roles.isEmpty || authzs.contains(AuthorizationType.NoRights)) { |
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.
keep the types! More info, more efficient, avoid useless parse/serialise roundtrip with errors to manage
…m roles Fixes #22349: Update user plugin to manage update custom roles
PR updated with a new commit |
} | ||
computeRoleCoverage(Set(Role.User), Set(AuthorizationType.Compliance.Read)) must beEqualTo( | ||
Some(Set(Role.forAuthz(AuthorizationType.Compliance.Read))) | ||
) |
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.
Test need to be kept as small as possible, else the intent is lost. There is a lot of builtin comparators, use them
https://issues.rudder.io/issues/22349
There is a lot of things to change in the way that plugin works, but in that PR I limited to what is the minimum to make it compatible with Rudder 7.3 changes + an auto-backup of
rudder-user.xml
file in case of problem - because we didn't have anything for that, and it would make rudder broken.So, the changes related to API changes in core are:
RudderRoles
)Box
toZIO
(reflecting changes in backend + general direction of where we are going)Other changes that were needed:
rudder-users.xml
file. At least, let the user know that he is not hopeless.getUserList
to force refresh UI, which seemed to be the wanted logic, but even that should be removed: the API return of POST should send the wanted updated model, and UI just handle that, which will remove more useless roundtrip to backend.What is not done but need to be done to have a "user management plugin with custom roles":