Command to protect text using crypto implemented #104

jan-kowalczyk-wttech · 2023-03-14T11:15:01Z

No description provided.

krystian-panek-vmltech · 2023-03-14T11:25:23Z

pkg/crypto.go

+	defer func(bodyReader io.ReadCloser) {
+		err := bodyReader.Close()
+		if err != nil {
+			log.Errorf("%s > cloud not close reader: %s", c.instance.ID(), err)


please recall my thoughts regarding context sensitive logging;

imagine me being a qa and seeing in console error z"local_author > cloud not close reader"` - this gives me nothing informative

hmm and IMO you overcomplicated reading response; please check other places in which I am unmarshalling response, for example: https://github.com/wttech/aemc/blob/main/pkg/osgi_event_manager.go#L26

Thanks, this UnmarshalJSON error checker looks cleaner, but I have question, why are you not closing your reader? I think, It can generate leaks.

hmmm you are right, I need to check few other places then as well

but checking closing IMO is redundant / is a tradeoff as 3rd party (web server) will not have a chance to unintentionally fail well working program on our side

hmmm I want to avoid repetitions of low-level code (boilerplate) and hide it in utility methods like fmtx.UnmarshalXX so we should rather put closing inside such methods instead of doing what you are doing

krystian-panek-vmltech · 2023-03-14T11:28:00Z

pkg/crypto.go

+	err = fmtx.UnmarshalJSON(bodyReader, &body)
+
+	if err != nil {
+		log.Errorf("%s > error during unmarshaling crypto response body: %s", c.instance.ID(), err)


please align the style of the error message to used in other functionalities

krystian-panek-vmltech

please address comments

krystian-panek-vmltech · 2023-03-14T11:30:59Z

cmd/aem/crypto.go

@@ -81,11 +81,16 @@ func (c *CLI) cryptoProtectCmd() *cobra.Command {
 				c.Error(err)
 				return
 			}
-			c.SetOutput("value", protectedValue)
+			c.SetOutput("protected", protectedValue)


I prefer naming style topic_trait which stands for not protectedValue but valueProtected because then vars are grouped logically / the same topic has the same string prefix;

how about valueProtected here; or just value as it was designed?

For me valueProtected or value is also fine, just I thought to use same convention as in crypto response, just to make it consistent

you should follow own conventions instead of being influenced by convention coming from outside to make your tool users less confused

krystian-panek-vmltech · 2023-03-14T13:01:30Z

pkg/crypto.go

+)
+
+const (
+	EncryptJsonPath = "/system/console/crypto/.json"


Do not introduce additional names for same thing. Why not CryptoProtectPath

krystian-panek-vmltech · 2023-03-14T13:02:13Z

pkg/crypto.go

+	response, err := requestFormData.Post(EncryptJsonPath)
+
+	if err != nil {
+		return "", fmt.Errorf("%s > Error while encrypting text using crypto: %s", c.instance.ID(), err)


Do not capitalize text. Do not add dot in the end

Why this starts with "error while..." but other msgs with "cannot". Align the style used

I missed this error :)

krystian-panek-vmltech · 2023-03-14T13:04:06Z

pkg/crypto.go

+	response, err := requestFormData.Post(EncryptJsonPath)
+
+	if err != nil {
+		return "", fmt.Errorf("%s > Error while encrypting text using crypto: %s", c.instance.ID(), err)


Why this starts with "error while..." but other msgs with "cannot". Align the style used

krystian-panek-vmltech · 2023-03-14T13:05:44Z

pkg/crypto.go

+	defer func(bodyReader io.ReadCloser) {
+		err := bodyReader.Close()
+		if err != nil {
+			log.Errorf("%s > cannot close crypto response body reader: %s", c.instance.ID(), err)


From the end user perspective response body reader is a technical detail that you should not mention. But mention what is happening understandable for that end user who is not seeing a code while running the tool

krystian-panek-vmltech · 2023-03-14T13:06:52Z

pkg/crypto.go

+	var body CryptoResponseBody
+
+	if err = fmtx.UnmarshalJSON(bodyReader, &body); err != nil {
+		return "", fmt.Errorf("%s > cannot unmarshal crypto response body: %s", c.instance.ID(), err)


Unmarshal is too techy word again. "Parse response" I am using in other places. Please align the style

krystian-panek-vmltech · 2023-03-14T15:11:48Z

cmd/aem/crypto.go

@@ -86,6 +86,11 @@ func (c *CLI) cryptoProtectCmd() *cobra.Command {
 		},
 	}
 	cmd.Flags().StringP("value", "v", "", "Value to protect")
-	cmd.MarkFlagRequired("value")
+	err := cmd.MarkFlagRequired("value")


nope; just use _ / see other code occurrences

krystian-panek-vmltech · 2023-03-14T15:12:11Z

pkg/crypto.go

+)
+
+const (
+	CryptoProtectPath = "/system/console/crypto/.json"


krystian-panek-vmltech · 2023-03-14T15:13:00Z

pkg/crypto.go

@@ -13,6 +19,10 @@ type Crypto struct {
 	keyBundleSymbolicName string
 }

+type CryptoResponseBody struct {


it's more like CryptoProtectResult

krystian-panek-vmltech · 2023-03-14T15:14:06Z

pkg/crypto.go

-	return value, nil // TODO implement this
+	var form = map[string]any{"datum": value}
+	log.Infof("%s > Protecting text using crypto.", c.instance.ID())
+	requestFormData := c.instance.http.RequestFormData(form)


redundant variable; keep the style of other features having snippet like yours

krystian-panek-vmltech · 2023-03-14T15:16:00Z

pkg/crypto.go

+
+	bodyReader := response.RawBody()
+
+	defer func(bodyReader io.ReadCloser) {


this defer is not needed here.

ReadCloser means that when reader/unmarshaller ends reading then the object implementing this interface will also automatically close the source so you don't need to take care;

just pass response.RawBody() directly to the UnmarshalJSON

krystian-panek-vmltech · 2023-03-15T09:28:28Z

pkg/crypto.go

@@ -70,5 +79,20 @@ func (c Crypto) Setup(hmacFile string, masterFile string) (bool, error) {
 }

 func (c Crypto) Protect(value string) (string, error) {
-	return value, nil // TODO implement this
+	log.Infof("%s > Protecting text using crypto.", c.instance.ID())


do not capitalize words; it's a general go style

krystian-panek-vmltech · 2023-03-15T09:28:47Z

pkg/crypto.go

@@ -70,5 +79,20 @@ func (c Crypto) Setup(hmacFile string, masterFile string) (bool, error) {
 }

 func (c Crypto) Protect(value string) (string, error) {
-	return value, nil // TODO implement this
+	log.Infof("%s > Protecting text using crypto.", c.instance.ID())
+	requestFormData := c.instance.http.RequestFormData(map[string]any{"datum": value})


inline 83 & 84 line (remove requestFormData var

extra vars are fine when they are reused or are introducing better readability; I don't see any of these here

krystian-panek-vmltech · 2023-03-15T09:29:57Z

pkg/crypto.go

+	response, err := requestFormData.Post(CryptoProtectPath)
+
+	if err != nil {
+		return "", fmt.Errorf("%s > cannot encrypt text using crypto: %s", c.instance.ID(), err)


special : %w here when you are passing err to Errorf / for supporting stack traces, etc

jan-kowalczyk-wttech linked an issue Mar 14, 2023 that may be closed by this pull request

Implement crypto protect command #93

Closed

krystian-panek-vmltech reviewed Mar 14, 2023

View reviewed changes

krystian-panek-vmltech requested changes Mar 14, 2023

View reviewed changes

krystian-panek-vmltech reviewed Mar 14, 2023

View reviewed changes

krystian-panek-vmltech requested changes Mar 14, 2023

View reviewed changes

krystian-panek-vmltech reviewed Mar 14, 2023

View reviewed changes

pkg/crypto.go

)

const (

CryptoProtectPath = "/system/console/crypto/.json"

Copy link

Contributor

krystian-panek-vmltech Mar 14, 2023

Choose a reason for hiding this comment

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

fine :)

krystian-panek-vmltech reviewed Mar 14, 2023

View reviewed changes

krystian-panek-vmltech and others added 7 commits March 14, 2023 22:55

Reader-closer impr

7cd1f28

Fix

b808104

Command to protect text using crypto implemented

aa09234

error message style aligned

e683296

output key renamed from protected to value

7e841e5

additional log fixes

13fbc10

code review fixes

3608087

jan-kowalczyk-wttech force-pushed the 93-implement-crypto-protect-command branch from 4135dc6 to 3608087 Compare March 15, 2023 08:38

krystian-panek-vmltech reviewed Mar 15, 2023

View reviewed changes

krystian-panek-vmltech approved these changes Mar 15, 2023

View reviewed changes

code review fixes

f0f8086

jan-kowalczyk-wttech merged commit dd76b10 into main Mar 15, 2023

jan-kowalczyk-wttech deleted the 93-implement-crypto-protect-command branch March 15, 2023 09:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Command to protect text using crypto implemented #104

Command to protect text using crypto implemented #104

jan-kowalczyk-wttech commented Mar 14, 2023

krystian-panek-vmltech Mar 14, 2023

krystian-panek-vmltech Mar 14, 2023

jan-kowalczyk-wttech Mar 14, 2023

krystian-panek-vmltech Mar 14, 2023 •

edited

Loading

krystian-panek-vmltech Mar 14, 2023 •

edited

Loading

krystian-panek-vmltech Mar 14, 2023 •

edited

Loading

krystian-panek-vmltech Mar 14, 2023

krystian-panek-vmltech left a comment

krystian-panek-vmltech Mar 14, 2023 •

edited

Loading

jan-kowalczyk-wttech Mar 14, 2023 •

edited

Loading

krystian-panek-vmltech Mar 14, 2023 •

edited

Loading

krystian-panek-vmltech Mar 14, 2023

krystian-panek-vmltech Mar 14, 2023 •

edited

Loading

krystian-panek-vmltech Mar 14, 2023

jan-kowalczyk-wttech Mar 14, 2023

krystian-panek-vmltech Mar 14, 2023

krystian-panek-vmltech Mar 14, 2023

krystian-panek-vmltech Mar 14, 2023

krystian-panek-vmltech Mar 14, 2023 •

edited

Loading

krystian-panek-vmltech Mar 14, 2023

krystian-panek-vmltech Mar 14, 2023

krystian-panek-vmltech Mar 14, 2023

krystian-panek-vmltech Mar 14, 2023

krystian-panek-vmltech Mar 15, 2023

krystian-panek-vmltech Mar 15, 2023

krystian-panek-vmltech Mar 15, 2023 •

edited

Loading

krystian-panek-vmltech Mar 15, 2023 •

edited

Loading


		bodyReader := response.RawBody()

		defer func(bodyReader io.ReadCloser) {

Command to protect text using crypto implemented #104

Command to protect text using crypto implemented #104

Conversation

jan-kowalczyk-wttech commented Mar 14, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

krystian-panek-vmltech Mar 14, 2023 • edited Loading

Choose a reason for hiding this comment

krystian-panek-vmltech Mar 14, 2023 • edited Loading

Choose a reason for hiding this comment

krystian-panek-vmltech Mar 14, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

krystian-panek-vmltech left a comment

Choose a reason for hiding this comment

krystian-panek-vmltech Mar 14, 2023 • edited Loading

Choose a reason for hiding this comment

jan-kowalczyk-wttech Mar 14, 2023 • edited Loading

Choose a reason for hiding this comment

krystian-panek-vmltech Mar 14, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

krystian-panek-vmltech Mar 14, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

krystian-panek-vmltech Mar 14, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

krystian-panek-vmltech Mar 15, 2023 • edited Loading

Choose a reason for hiding this comment

krystian-panek-vmltech Mar 15, 2023 • edited Loading

Choose a reason for hiding this comment

krystian-panek-vmltech Mar 14, 2023 •

edited

Loading

krystian-panek-vmltech Mar 14, 2023 •

edited

Loading

krystian-panek-vmltech Mar 14, 2023 •

edited

Loading

krystian-panek-vmltech Mar 14, 2023 •

edited

Loading

jan-kowalczyk-wttech Mar 14, 2023 •

edited

Loading

krystian-panek-vmltech Mar 14, 2023 •

edited

Loading

krystian-panek-vmltech Mar 14, 2023 •

edited

Loading

krystian-panek-vmltech Mar 14, 2023 •

edited

Loading

krystian-panek-vmltech Mar 15, 2023 •

edited

Loading

krystian-panek-vmltech Mar 15, 2023 •

edited

Loading