Skip to content

Conversation

@babariviere
Copy link

@babariviere babariviere commented Feb 20, 2019

@ericchiang
Copy link
Contributor

Thanks! This is interesting. I'll need some time to review the RFC to review this PR.

@stpabhi
Copy link

stpabhi commented Mar 12, 2019

@babariviere Any plans on updating other storage options? I created a fork to implement k8s storage and others (wip) here stpabhi@6c184bb

@babariviere
Copy link
Author

@stpabhi I plan to implement Redis storage, I may do it this week

@stpabhi
Copy link

stpabhi commented Mar 12, 2019

I meant etcd and k8s storage options for pkce. I don’t see the code for them in this PR.

@babariviere
Copy link
Author

Oh sorry, I have misunderstood. I will do it today

@babariviere
Copy link
Author

@stpabhi it's done if you want to use it

@stpabhi
Copy link

stpabhi commented Mar 12, 2019

@babariviere thanks, we have already forked dex and implemented pkce internally. I’m interested in getting it done upstream rather than branching out.

@stpabhi
Copy link

stpabhi commented Mar 12, 2019

I have few suggestions on the PR.

  • Add code challenge methods to discovery
  • Add check for unsupported code challenge methods here
  • Add tests and update conformance test.

@babariviere
Copy link
Author

I will add test later

Copy link
Contributor

@srenatus srenatus left a comment

Choose a reason for hiding this comment

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

Thanks a lot, this is a nice contribution.

The comments inline aside, can we add a test or two? ;)

Update: I've seen this to late

I will add test later

@Teeed
Copy link
Contributor

Teeed commented Sep 13, 2019

Adding link to issue here: #1114 (I have just implemented PKCE by my own because of issue #1114 had no PR assigned).

@mackenzie-orange
Copy link

What's the status of this PR? Looks like the downstream branch got deleted :/.

@babariviere
Copy link
Author

Yes, sorry. I have to close the source as they were some code written for my company.
I will recreate it if necessary.

@Teeed
Copy link
Contributor

Teeed commented Jan 29, 2020

It is sad that this was not merged year (!) ago. As I can see code is still available in this PR. I have mine implementation of PKCE laying around if necessary (if this PR could not be merged for some reason).

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.

6 participants