fix: add new message when refresh parameter is true #1560

nanikjava · 2021-07-15T13:34:34Z

PR for fixing #1117

I have read the contributing guidelines.
I am following the
contributing code guidelines.
I have read the security policy.
I confirm that this pull request does not address a security
vulnerability. If this pull request addresses a security. vulnerability, I
confirm that I got green light (please contact
security@ory.sh) from the maintainers to push
the changes.
I have added tests that prove my fix is effective or that my feature
works.
I have added or changed the documentation.

aeneasr

Thank you! This looks really good already! There are a few more things we need to do to get it merged.

The CI is failing because some files are formatted incorrectly. To format them, run:

$ make format
$ git commit -a -m "styles: format code"
$ git push

Please also add a test around here

kratos/selfservice/strategy/password/login_test.go

Line 465 in 1c84205

    
           assert.Empty(t, gjson.GetBytes(body, "ui.nodes.#(attributes.name==password).attributes.value").String(), "%s", body)

verifying that the message is set in the payload :)

Thank you!

aeneasr · 2021-07-16T12:15:52Z

session/handler.go

 //       500: jsonError
+//       401: jsonError


Revert order

aeneasr · 2021-07-16T12:17:04Z

text/message_verification.go

@@ -8,7 +8,8 @@ import (
 const (
 	InfoSelfServiceVerification           ID = 1070000 + iota // 1070000
 	InfoSelfServiceVerificationEmailSent                      // 1070001
-	InfoSelfServiceVerificationSuccessful                     // 1060002
+	InfoSelfServiceVerificationSuccessful                     // 1070002


Since this is in the login UI, please put it in message_login and give it a descriptive name

nanikjava · 2021-07-17T02:17:30Z

Thank you! This looks really good already! There are a few more things we need to do to get it merged.

The CI is failing because some files are formatted incorrectly. To format them, run:
$ make format
$ git commit -a -m "styles: format code"
$ git push
Please also add a test around here

kratos/selfservice/strategy/password/login_test.go

Line 465 in 1c84205

assert.Empty(t, gjson.GetBytes(body, "ui.nodes.#(attributes.name==password).attributes.value").String(), "%s", body)

verifying that the message is set in the payload :)

Thank you!

@aeneasr

Thanks for the comment. Will fix it up and add the test.

Frankly, I'm still confused of the use case of this fix, as just started to get myself down and dirty with the code :)

The fix will include message Please confirm this action by verifying that it's you. when the ?refresh=true flag is set. Does this mean that regardless of what the state of the login is as long as the URL include the ?refresh=true flag that message will be passed back ?

I've updated the PR. Thanks

codecov · 2021-07-17T04:48:09Z

Codecov Report

Merging #1560 (e9eb4d1) into master (60d848d) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #1560      +/-   ##
==========================================
+ Coverage   74.17%   74.19%   +0.01%     
==========================================
  Files         257      257              
  Lines       12557    12565       +8     
==========================================
+ Hits         9314     9322       +8     
  Misses       2624     2624              
  Partials      619      619

Impacted Files	Coverage Δ
selfservice/flow/login/handler.go	`67.96% <100.00%> (+0.50%)`	⬆️
text/message_login.go	`71.69% <100.00%> (+3.61%)`	⬆️
x/nosurf.go	`92.59% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 64c90bf...e9eb4d1. Read the comment docs.

nanikjava · 2021-07-21T23:08:41Z

@aeneasr any feedback on the PR ? Thanks.

aeneasr · 2021-07-23T14:45:30Z

@nanikjava sorry for the long response time, I didn't have time to review things because it was a really busy week.

The fix will include message Please confirm this action by verifying that it's you. when the ?refresh=true flag is set. Does this mean that regardless of what the state of the login is as long as the URL include the ?refresh=true flag that message will be passed back ?

Yes, exactly, that would be the case. I think it's an o.k. workaround for now.

There is still an open item, could you please address my comment:

#1560 (comment)

Thank you for your hard work! :)

nanikjava · 2021-07-24T06:48:28Z

@nanikjava sorry for the long response time, I didn't have time to review things because it was a really busy week.

The fix will include message Please confirm this action by verifying that it's you. when the ?refresh=true flag is set. Does this mean that regardless of what the state of the login is as long as the URL include the ?refresh=true flag that message will be passed back ?

Yes, exactly, that would be the case. I think it's an o.k. workaround for now.

Ok cool. Thanks for confirming about the behaviour.

There is still an open item, could you please address my comment:

#1560 (comment)

Thank you for your hard work! :)

Fixed.

nanikjava · 2021-07-24T14:34:29Z

@aeneasr Checked the E2E test and looks like it has 1 test case failing using cockroachdb not sure what is the problem.

Tried running locally by executing the run.sh inside test/e2e/run.sh but not able to run completely as it keeps on timing out after the following

waiting for 6 resources: http-get://127.0.0.1:4434/health/ready, http-get://127.0.0.1:4455/health, http-get://127.0.0.1:4446/, http-get://127.0.0.1:4456/health, http-get://127.0.0.1:4457/, http-get://127.0.0.1:4437/mail
waiting for 5 resources: http-get://127.0.0.1:4434/health/ready, http-get://127.0.0.1:4455/health, http-get://127.0.0.1:4456/health, http-get://127.0.0.1:4457/, http-get://127.0.0.1:4437/mail
waiting for 4 resources: http-get://127.0.0.1:4434/health/ready, http-get://127.0.0.1:4455/health, http-get://127.0.0.1:4456/health, http-get://127.0.0.1:4457/
waiting for 3 resources: http-get://127.0.0.1:4434/health/ready, http-get://127.0.0.1:4455/health, http-get://127.0.0.1:4457/
waiting for 2 resources: http-get://127.0.0.1:4434/health/ready, http-get://127.0.0.1:4457/
waiting for 1 resources: http-get://127.0.0.1:4457/

aeneasr · 2021-07-27T17:39:26Z

text/message_verification.go

@@ -48,6 +47,15 @@ func NewVerificationEmailSent() *Message {
 	}
 }

+func NewVerificationConfirmation() *Message {


Please move this to message_login - this file should stay unmodified. message_verification refers to the self-service verification (e.g. email verification flow). Thank you :)

@aeneasr Thanks for the feedback. Changes has been moved to message_login. Hope I did it right this time :)

…-1117

aeneasr · 2021-07-30T08:44:52Z

Thank you! I added some changes but overall this looks very good! Ready to be merged :)

Closes ory#1117 Co-authored-by: aeneasr <3372410+aeneasr@users.noreply.github.com>

aeneasr requested changes Jul 16, 2021

View reviewed changes

fix: add new message when refresh parameter is true

6d5dbed

fix: add test cases and modify code based on PR feedback

83fc62e

nanikjava force-pushed the f-fix-1117 branch from 2d84202 to 83fc62e Compare July 17, 2021 03:57

nanikjava changed the title ~~[WIP] fix: add new message when refresh parameter is true~~ fix: add new message when refresh parameter is true Jul 17, 2021

nanikjava requested a review from aeneasr July 17, 2021 03:58

fix: remove InfoSelfServiceVerificationSuccessful as not needed

54a5027

aeneasr requested changes Jul 27, 2021

View reviewed changes

nanikjava and others added 3 commits July 28, 2021 18:57

Merge branch 'master' of github-nanikjava:nanikjava/kratos into f-fix…

441366b

…-1117

fix: move the new message to message_login.go

13d9096

chore: code review

e9eb4d1

aeneasr approved these changes Jul 30, 2021

View reviewed changes

aeneasr merged commit 0525623 into ory:master Jul 30, 2021

harnash pushed a commit to Wikia/kratos that referenced this pull request Aug 3, 2021

fix: add new message when refresh parameter is true (ory#1560)

852e205

Closes ory#1117 Co-authored-by: aeneasr <3372410+aeneasr@users.noreply.github.com>

beezzoo mentioned this pull request Jul 31, 2022

[Snyk] Security upgrade node-fetch from 2.6.1 to 3.2.10 beezzoo/kratos#9

Open

Rutam21 mentioned this pull request Jul 31, 2022

[Snyk] Security upgrade node-fetch from 2.6.1 to 3.2.10 Rutam21/kratos#11

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: add new message when refresh parameter is true #1560

fix: add new message when refresh parameter is true #1560

nanikjava commented Jul 15, 2021

aeneasr left a comment

aeneasr Jul 16, 2021

aeneasr Jul 16, 2021

nanikjava commented Jul 17, 2021 •

edited

codecov bot commented Jul 17, 2021 •

edited

nanikjava commented Jul 21, 2021

aeneasr commented Jul 23, 2021

nanikjava commented Jul 24, 2021

nanikjava commented Jul 24, 2021

aeneasr Jul 27, 2021

nanikjava Jul 28, 2021

aeneasr commented Jul 30, 2021

fix: add new message when refresh parameter is true #1560

fix: add new message when refresh parameter is true #1560

Conversation

nanikjava commented Jul 15, 2021

aeneasr left a comment

Choose a reason for hiding this comment

aeneasr Jul 16, 2021

Choose a reason for hiding this comment

aeneasr Jul 16, 2021

Choose a reason for hiding this comment

nanikjava commented Jul 17, 2021 • edited

codecov bot commented Jul 17, 2021 • edited

Codecov Report

nanikjava commented Jul 21, 2021

aeneasr commented Jul 23, 2021

nanikjava commented Jul 24, 2021

nanikjava commented Jul 24, 2021

aeneasr Jul 27, 2021

Choose a reason for hiding this comment

nanikjava Jul 28, 2021

Choose a reason for hiding this comment

aeneasr commented Jul 30, 2021

nanikjava commented Jul 17, 2021 •

edited

codecov bot commented Jul 17, 2021 •

edited