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 server_test.go so it works + make code flow test pass with no secret #230

Conversation

jarlandre
Copy link

@jarlandre jarlandre commented Jan 18, 2023

  1. Remove query encoding of urls for redirect uri in server_test.go to make tests successfully hit the /oauth2 callback endpoint
  2. Then fix the token requests in the auth code flow tests
  3. Then fix the manager to allow for no client_secret for auth code flow with pkce where code verifier is at least 43 chars. EDIT: i see there might not be need to check its length and only its existence. Its already validated properly. Merely checking that that code verifier is not empty string will be enough to ensure that the request will fail without the proper code verifier.
  • Remove length check on code verifier and only check if its a non-empty string
  • actually we need to differentiate between confidential and public clients, which this server library cannot, afaik. EDIT: new draft PR here Introducing public field to client models #233

the pkce spec states that code verifier should be minimum 43 chars (#230 (comment)), should this be verified earlier in the process?

@jarlandre
Copy link
Author

☝️ @LyricTian

@jarlandre jarlandre force-pushed the jman/tryToFixServerTestsThatFailsSilently branch from 837a562 to e733fa0 Compare January 18, 2023 22:06
@jarlandre jarlandre force-pushed the jman/tryToFixServerTestsThatFailsSilently branch 2 times, most recently from 1ae7ccc to a327e93 Compare January 19, 2023 06:36
@jarlandre jarlandre force-pushed the jman/tryToFixServerTestsThatFailsSilently branch from a327e93 to 5504dd7 Compare January 19, 2023 06:37
@jarlandre jarlandre changed the title undo query escape for url so test actually tests fix server_test.go so that it doesnt fail silently and fix code to make code flow pass with no secret Jan 19, 2023
@jarlandre jarlandre changed the title fix server_test.go so that it doesnt fail silently and fix code to make code flow pass with no secret fix server_test.go so that it works and fix code flow pass with no secret Jan 19, 2023
@jarlandre jarlandre changed the title fix server_test.go so that it works and fix code flow pass with no secret fix server_test.go so that it works and fix code flow to pass with no secret Jan 19, 2023
@jarlandre
Copy link
Author

jarlandre commented Jan 19, 2023

quote:

code_verifier — The code verifier should be a high-entropy cryptographic random string with a minimum of 43 characters and a maximum of 128 characters.

@jarlandre jarlandre changed the title fix server_test.go so that it works and fix code flow to pass with no secret fix server_test.go so that it works + fix code flow to pass with no secret Jan 19, 2023
@jarlandre jarlandre changed the title fix server_test.go so that it works + fix code flow to pass with no secret fix server_test.go so it works + make code flow test pass with no secret Jan 19, 2023
@jarlandre
Copy link
Author

./go.test.sh
ok  	github.com/go-oauth2/oauth2/v4	0.203s	coverage: 42.9% of statements
?   	github.com/go-oauth2/oauth2/v4/errors	[no test files]
?   	github.com/go-oauth2/oauth2/v4/example/client	[no test files]
?   	github.com/go-oauth2/oauth2/v4/example/server	[no test files]
ok  	github.com/go-oauth2/oauth2/v4/generates	0.219s	coverage: 77.8% of statements
ok  	github.com/go-oauth2/oauth2/v4/manage	0.211s	coverage: 61.8% of statements
?   	github.com/go-oauth2/oauth2/v4/models	[no test files]
ok  	github.com/go-oauth2/oauth2/v4/server	0.237s	coverage: 53.6% of statements
ok  	github.com/go-oauth2/oauth2/v4/store	2.314s	coverage: 87.2% of statements

@DennisMuchiri
Copy link

@jarlandre why was this not merged?

@jarlandre
Copy link
Author

because it war replaced by another PR .. see mentions

@jarlandre
Copy link
Author

should have rebased and kept it in this PR i guess .. live and learn

@shynome
Copy link
Contributor

shynome commented Mar 13, 2023

here is a tip: how to work with pcke , you need change the ClientInfoHandler to resolve invalid_client error

srv.SetClientInfoHandler(server.ClientFormHandler)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants