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

Permit response_type be "code id_token" #328

Merged
merged 10 commits into from
Nov 7, 2018
Merged

Permit response_type be "code id_token" #328

merged 10 commits into from
Nov 7, 2018

Conversation

niwenhao
Copy link
Contributor

Related issue

None

Proposed changes

When hybrid flow with response_type=id_token code, we must permit code and others.
For that, I change the check method from Exact to Has.

Checklist

  • I have read the contributing guidelines
  • 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 [email protected]) from the maintainers to push the changes.
  • I signed the Developer's Certificate of Origin
    by signing my commit(s). You can amend your signature to the most recent commit by using git commit --amend -s. If you
    amend the commit, you might need to force push using git push --force HEAD:<branch>. Please be very careful when using
    force push.
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation within the code base (if appropriate)

Further comments

None.

@aeneasr
Copy link
Member

aeneasr commented Oct 30, 2018

Is PKCE defined for hybrid flows?

@aeneasr
Copy link
Member

aeneasr commented Oct 30, 2018

Could you maybe add a test for this flow? It should be similar to this one and this one.

@niwenhao
Copy link
Contributor Author

niwenhao commented Nov 5, 2018

Sorry for response so late.
In face, I never read the integration test code before, It take some time of me.

Is PKCE defined for hybrid flows?

When callback url was hijacked, PKCE can prevent from obtaining access token. The same thing can happen in hybrid flow and nothing else can prevent it.

Could you maybe add a test for this flow? It should be similar to this one and this one.

I will push it soon.

Thanks.

adamdecaf and others added 10 commits November 6, 2018 11:03
Signed-off-by: Adam Shannon <[email protected]>
Signed-off-by: Wenhao Ni <[email protected]>
Users of this library can easily create the following:

hasher := fosite.BCrypt{}
hasher.Hash(..)

This is a problem because WorkFactor will default to 0 and x/crypto/bcrypt will default that to 4 (See https://godoc.org/golang.org/x/crypto/bcrypt).

Instead this should be some higher cost factor. Callers who need a lower WorkFactor can still lower the cost, if needed.

Signed-off-by: Adam Shannon <[email protected]>
Signed-off-by: Wenhao Ni <[email protected]>
@aeneasr
Copy link
Member

aeneasr commented Nov 7, 2018

Thank you!

budougumi0617 added a commit to budougumi0617/fosite that referenced this pull request May 10, 2019
Signed-off-by: Adam Shannon <[email protected]>
Signed-off-by: Wenhao Ni <[email protected]>
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.

3 participants