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

[WIP] refactoring autoconverted ruby code into ruby code #40

Closed
wants to merge 7 commits into from
Closed
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion Rakefile
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
require "yast/rake"

Yast::Tasks.configuration do |conf|
#lets ignore license check for now
# lets ignore license check for now
conf.skip_license_check << /.*/
end
5 changes: 5 additions & 0 deletions package/yast2-security.changes
Original file line number Diff line number Diff line change
@@ -1,3 +1,8 @@
-------------------------------------------------------------------
Thu Sep 1 11:39:37 UTC 2016 - vpereira@suse.com

- Refactoring autoconverted ruby code into ruby dialect

-------------------------------------------------------------------
Wed Aug 31 11:37:52 UTC 2016 - jreidinger@suse.com

Expand Down
49 changes: 49 additions & 0 deletions rubocop_yast_style.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
# Rubocop style configuration
#
# Following
# https://github.com/SUSE/style-guides/blob/master/Ruby.md

# https://github.com/SUSE/style-guides/blob/master/Ruby.md#strings
Style/StringLiterals:
EnforcedStyle: double_quotes

Style/StringLiteralsInInterpolation:
EnforcedStyle: double_quotes

# Is there any justification for "aligned" which is the default?
Style/MultilineOperationIndentation:
EnforcedStyle: indented

# https://github.com/SUSE/style-guides/blob/master/Ruby.md#arrays
Style/WordArray:
Enabled: false

# align arrows:
# "foo" => true
# "foo_bar" => false
# and also colons:
# foo: true
# foo_bar: false
Style/AlignHash:
EnforcedHashRocketStyle: table
EnforcedColonStyle: table

# no extra indentation for multiline function calls
Style/AlignParameters:
EnforcedStyle: with_fixed_indentation

# no extra indentation for case
Style/CaseIndentation:
IndentWhenRelativeTo: end

# "unless" has a different connotation than "if not"
Style/NegatedIf:
Enabled: false

# use "raise" instead of "fail"
Style/SignalException:
EnforcedStyle: only_raise

# do not force %r
Style/RegexpLiteral:
Enabled: false
Copy link
Member

Choose a reason for hiding this comment

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

thanks for you ruby style guide, but better to reference it. See how other modules do it: e.g. https://github.com/yast/yast-bootloader/blob/master/.rubocop.yml#L3

106 changes: 44 additions & 62 deletions src/clients/security.rb
Original file line number Diff line number Diff line change
Expand Up @@ -30,12 +30,13 @@
# only some calls to the basic functions. The settings are
# initialized, main dialog is called and then settings are
# saved.

module Yast
class SecurityClient < Client
def main
Yast.import "UI"

#**
# **
# <h3> Security configuration

textdomain "security"
Expand All @@ -50,8 +51,6 @@ def main

Yast.include self, "security/wizards.rb"



# the command line description map
@cmdline = {
"id" => "security",
Expand Down Expand Up @@ -89,7 +88,7 @@ def main
}
},
"options" => {
"workstation" => {
"workstation" => {
# command line help text for 'level workstation' option
"help" => _(
"Workstation security level"
Expand Down Expand Up @@ -142,7 +141,7 @@ def main
"mappings" => {
"summary" => [],
"level" => ["workstation", "roaming", "server"],
#FIXME 1,2,3 aliases
# FIXME: 1,2,3 aliases
"set" => [
"passwd",
"crack",
Expand All @@ -158,7 +157,7 @@ def main
# Finish
Builtins.y2milestone("Security module finished")
Builtins.y2milestone("----------------------------------------")
deep_copy(@ret)
deep_copy(@ret)

# EOF
end
Expand All @@ -180,83 +179,66 @@ def SecuritySummaryHandler(options)
def SecurityLevelHandler(options)
options = deep_copy(options)
current = :custom
Builtins.maplist(@Levels) do |key, level|
current = key if level == Security.Settings
end
lvl = ""
if options.key?("workstation")
lvl = "Level1"
elsif options.key?("roaming")
lvl = "Level2"
elsif options.key?("server")
lvl = "Level3"

levels = @Levels.select { |_key, level| level == Security.Settings }

current = if levels.empty?
:custom
else
levels.last # mimic the old implementation
Copy link
Member

Choose a reason for hiding this comment

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

this is in fact wrong, as levels contain pair of [key, level] and old one use only key.
so how code can look like:

current = @Levels.select{ |_key, level| level == Security.Settings }.keys.last || :custom

end

lvl = if options.key?("workstation")
"Level1"
elsif options.key?("roaming")
"Level2"
elsif options.key?("server")
"Level3"
else
:custom # shouldnt :custom and levels all same type string or symbol?
Copy link
Member

Choose a reason for hiding this comment

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

is it valid? what is reason for this change?

end

if current != lvl
Security.Settings = Ops.get(@Levels, lvl, {})
Security.Settings = @Levels.fetch(lvl, {})
Security.modified = true
return true
end
false
Security.modified
Copy link
Member

Choose a reason for hiding this comment

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

this in fact is not same as previous code as it can return true, when something else set Security.modified to true and here it is not set

Copy link
Author

Choose a reason for hiding this comment

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

and why we return always false? there is a reason for that?

Copy link
Member

Choose a reason for hiding this comment

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

I expect that logic is that we return false to indicate that this action do not change anything.

end

# Set value of specific security option
# @return [Boolean] false
# @return [Boolean] successfully modified?
def SecuritySetHandler(options)
options = deep_copy(options)
if Builtins.haskey(options, "passwd") &&
Ops.get_string(options, "passwd", "") !=
Ops.get(Security.Settings, "PASSWD_ENCRYPTION", "")
Ops.set(
Security.Settings,
"PASSWD_ENCRYPTION",
Ops.get_string(options, "passwd", "des")
)

if options.key?("password") &&
Security.Settings["PASSWD_ENCRYPTION"] != options["password"]
Security.Settings["PASSWD_ENCRYPTION"] = options.fetch("password", "des")
Security.modified = true
end
if Builtins.haskey(options, "crack") &&
Ops.get_string(options, "crack", "") !=
Ops.get(Security.Settings, "PASSWD_USE_CRACKLIB", "")
Ops.set(
Security.Settings,
"PASSWD_USE_CRACKLIB",
Ops.get_string(options, "crack", "yes")
)

if options_has_key?("crack") &&
Security.Settings["PASSWD_USE_CRACKLIB"] != options["crack"]
Security.Settings["PASSWD_USE_CRACKLIB"] = options.fetch("crack", "yes")
Security.modified = true
end
if Builtins.haskey(options, "permissions") &&
!Builtins.issubstring(
Ops.get(Security.Settings, "PERMISSION_SECURITY", ""),
Ops.get_string(options, "permissions", "")
)
Ops.set(
Security.Settings,
"PERMISSION_SECURITY",
Ops.add(Ops.get_string(options, "permissions", ""), " local")
)

if options.key?("permissions") &&
Security.Settings["PERMISSION_SECURITY"] != options["permissions"]
Security.Settings["PERMISSION_SECURITY"] = options.fetch("permissions", "local")
Security.modified = true
end

if Builtins.haskey(options, "remember") &&
Ops.get(Security.Settings, "PASSWD_REMEMBER_HISTORY", "0") !=
Ops.get_string(options, "remember", "0")
to_remember = Builtins.tointeger(
Ops.get_string(options, "remember", "0")
)
if to_remember == nil || Ops.less_than(to_remember, 0) ||
Ops.greater_than(to_remember, 400)
# error message
if options.key?("remember") &&
Security.Settings["PASSWD_REMEMBER_HISTORY"] != options["remember"]
if options["remember"].to_i.between?(0, 400)
Security.Settings["PASSWD_REMEMBER_HISTORY"] = options["remember"].to_i
Security.modified = true
else
Report.Error(
_("The number of passwords to remember must be between 0 an 400.")
)
return false
end
Ops.set(
Security.Settings,
"PASSWD_REMEMBER_HISTORY",
Ops.get_string(options, "remember", "0")
)
Security.modified = true
Security.modified
end
Security.modified
end
Expand Down
42 changes: 21 additions & 21 deletions src/clients/security_auto.rb
Original file line number Diff line number Diff line change
Expand Up @@ -56,30 +56,30 @@ def main
@param = {}

# Check arguments
if Ops.greater_than(Builtins.size(WFM.Args), 0) &&
Ops.is_string?(WFM.Args(0))
@func = Convert.to_string(WFM.Args(0))
if Ops.greater_than(Builtins.size(WFM.Args), 1) &&
Ops.is_map?(WFM.Args(1))
@param = Convert.to_map(WFM.Args(1))
if !Yast::WFM.Args.empty?
@func = Yast::WFM.Args[0]
if Yast::WFM.Args.length > 1 && Yast::WFM.Args[1].is_a?(Hash)
Copy link
Member

Choose a reason for hiding this comment

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

in fact second option is enough, because if length is 1 or less, then Yast::WFM.Args[1] returns nil and nil.is_a?(Hash) returns false

@param = WFM.Args[1]
end
end

Builtins.y2debug("func=%1", @func)
Builtins.y2debug("param=%1", @param)

# Create a summary
if @func == "Summary"
case @func
when "Summary"
@summary = Security.Summary
@ret = Ops.get_string(@summary, 0, "")
# Reset configuration
elsif @func == "Reset"
when "Reset"
Security.Import({})
@ret = {}
# Change configuration (run AutoSequence)
elsif @func == "Change"
when "Change"
@ret = SecurityAutoSequence()
# Import Data
elsif @func == "Import"
when "Import"
# Compat
if Builtins.haskey(@param, "encryption")
Ops.set(
Expand All @@ -90,37 +90,37 @@ def main
end
@ret = Security.Import(
Map.KeysToUpper(
Convert.convert(@param, :from => "map", :to => "map <string, any>")
Convert.convert(@param, from: "map", to: "map <string, any>")
Copy link
Member

Choose a reason for hiding this comment

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

in fact whole this convert can be dropeed as implementation do not distinct between map and specialized map.

Copy link
Member

Choose a reason for hiding this comment

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

so simple @ret = Security.Import(Map.KeysToUpper(@param)) can be used

)
)
# Return required packages
elsif @func == "Packages"
when "Packages"
@ret = {}
# Return actual state
elsif @func == "Export"
when "Export"
@ret = Map.KeysToLower(
Convert.convert(
Security.Export,
:from => "map",
:to => "map <string, any>"
from: "map",
to: "map <string, any>"
Copy link
Member

Choose a reason for hiding this comment

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

same here

)
)
# Read current state
elsif @func == "Read"
when "Read"
Yast.import "Progress"
Progress.off
@ret = Security.Read
Progress.on
# Write givven settings
elsif @func == "Write"
when "Write"
Yast.import "Progress"
Security.write_only = true
Progress.off
@ret = Security.Write
Progress.on
elsif @func == "SetModified"
when "SetModified"
@ret = Security.SetModified
elsif @func == "GetModified"
when "GetModified"
@ret = Security.GetModified
else
Builtins.y2error("Unknown function: %1", @func)
Expand All @@ -131,8 +131,8 @@ def main
Builtins.y2milestone("Security auto finished")
Builtins.y2milestone("----------------------------------------")

deep_copy(@ret)

# is it needed?
Copy link
Member

Choose a reason for hiding this comment

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

no, it is not needed as @ret is always assigned and from fresh copy.

deep_copy(@ret)
# EOF
Copy link
Member

Choose a reason for hiding this comment

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

this is in fact wrong as you kill return value. I just said that deep_copy is not needed, so still you need to have here @ret otherwise it return nil, causing bugs.

end
end
Expand Down
2 changes: 1 addition & 1 deletion src/clients/security_summary.rb
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ def main
# Finish
Builtins.y2milestone("Security summary finished")
Builtins.y2milestone("----------------------------------------")
@ret
@ret

# EOF
end
Expand Down
4 changes: 2 additions & 2 deletions src/include/security/complex.rb
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ def initialize_security_complex(include_target)
# @return `next if success, else `abort
def WriteDialog
Wizard.RestoreHelp(Ops.get_string(@HELPS, "write", ""))
Security.AbortFunction = lambda { Security.PollAbort }
Security.AbortFunction = -> { Security.PollAbort }
ret = Security.Write
ret ? :next : :abort
end
Expand Down Expand Up @@ -133,7 +133,7 @@ def MainDialog
Wizard.SetAbortButton(:abort, Label.CancelButton)

ret = nil
while true
loop do
cur = UI.QueryWidget(Id(:rb), :CurrentButton)
ret = UI.UserInput

Expand Down