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

fix: don't refresh AuthenticatedAt for pin login (PS-333) #6

Merged
merged 1 commit into from
Jun 17, 2024

Conversation

maoanran
Copy link

@maoanran maoanran commented Jun 3, 2024

Related issue(s)

Checklist

  • I have read the contributing guidelines.
  • I have referenced an issue containing the design document if my change
    introduces a new feature.
  • 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 the approval (please contact
    [email protected]) 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.

Further Comments

@maoanran maoanran changed the title fix: Don't refresh current session is the session is active (PS-333) fix: don't refresh current session is the session is active (PS-333) Jun 3, 2024
@maoanran maoanran changed the title fix: don't refresh current session is the session is active (PS-333) fix: don't refresh current session if the session is active (PS-333) Jun 3, 2024
selfservice/strategy/pin/login_test.go Show resolved Hide resolved
session/session.go Outdated Show resolved Hide resolved
@maoanran maoanran changed the title fix: don't refresh current session if the session is active (PS-333) fix: don't refresh AuthenticatedAt for pin login (PS-333) Jun 7, 2024
Copy link

@splaunov splaunov left a comment

Choose a reason for hiding this comment

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

Looks good.
Make sure all tests pass with the command:

go test -p 1 -tags sqlite,json1 -count=1 -failfast -timeout 30m ./...

@maoanran maoanran marked this pull request as ready for review June 10, 2024 11:53
@maoanran maoanran requested a review from aeneasr as a code owner June 10, 2024 11:53
@maoanran
Copy link
Author

Looks good. Make sure all tests pass with the command:

go test -p 1 -tags sqlite,json1 -count=1 -failfast -timeout 30m ./...

Thanks, there are some test failed, but not sure if it is my environment issue, do you know the failures?

--- FAIL: TestSettingsStrategy (1.04s)
    strategy_helper_test.go:246: Environment did not provide Ory Hydra, starting fresh.
    strategy_helper_test.go:270:
        	Error Trace:	/Users/anma/repo/kratos/selfservice/strategy/oidc/strategy_helper_test.go:270
        	            				/Users/anma/repo/kratos/selfservice/strategy/oidc/strategy_settings_test.go:60
        	Error:      	Received unexpected error:
        	            	API error (400): invalid tag format

        	            	github.com/ory/dockertest/v3.(*Pool).RunWithOptions
        	            		/Users/anma/go/pkg/mod/github.com/ory/dockertest/[email protected]/dockertest.go:413
        	            	github.com/ory/kratos/selfservice/strategy/oidc_test.newHydra
        	            		/Users/anma/repo/kratos/selfservice/strategy/oidc/strategy_helper_test.go:252
        	            	github.com/ory/kratos/selfservice/strategy/oidc_test.TestSettingsStrategy
        	            		/Users/anma/repo/kratos/selfservice/strategy/oidc/strategy_settings_test.go:60
        	            	testing.tRunner
        	            		/opt/homebrew/Cellar/go/1.22.2/libexec/src/testing/testing.go:1689
        	            	runtime.goexit
        	            		/opt/homebrew/Cellar/go/1.22.2/libexec/src/runtime/asm_arm64.s:1222
        	Test:       	TestSettingsStrategy
FAIL
FAIL	github.com/ory/kratos/selfservice/strategy/oidc	19.275s
--- FAIL: TestOAuth2Provider (1.11s)
    op_helpers_test.go:153:
        	Error Trace:	/Users/anma/repo/kratos/selfservice/strategy/password/op_helpers_test.go:153
        	            				/Users/anma/repo/kratos/selfservice/strategy/password/op_login_test.go:206
        	Error:      	Received unexpected error:
        	            	API error (400): invalid tag format

        	            	github.com/ory/dockertest/v3.(*Pool).RunWithOptions
        	            		/Users/anma/go/pkg/mod/github.com/ory/dockertest/[email protected]/dockertest.go:413
        	            	github.com/ory/kratos/selfservice/strategy/password_test.newHydra
        	            		/Users/anma/repo/kratos/selfservice/strategy/password/op_helpers_test.go:134
        	            	github.com/ory/kratos/selfservice/strategy/password_test.TestOAuth2Provider
        	            		/Users/anma/repo/kratos/selfservice/strategy/password/op_login_test.go:206
        	            	testing.tRunner
        	            		/opt/homebrew/Cellar/go/1.22.2/libexec/src/testing/testing.go:1689
        	            	runtime.goexit
        	            		/opt/homebrew/Cellar/go/1.22.2/libexec/src/runtime/asm_arm64.s:1222
        	Test:       	TestOAuth2Provider
FAIL
FAIL	github.com/ory/kratos/selfservice/strategy/password	4.619s

@splaunov
Copy link

Try to pull hydra with this command before running tests

docker pull oryd/hydra:v2.2.0@sha256:6c0f9195fe04ae16b095417b323881f8c9008837361160502e11587663b37c09

@maoanran
Copy link
Author

Thanks, the tests passed.

@maoanran maoanran merged commit 5c81ff5 into feature/pin-login Jun 17, 2024
20 of 22 checks passed
@maoanran maoanran deleted the PS-333 branch June 17, 2024 14:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants