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

[9.x] Added provider check to cookie authentication #1282

Closed
wants to merge 1 commit into from
Closed

[9.x] Added provider check to cookie authentication #1282

wants to merge 1 commit into from

Conversation

billriess
Copy link
Contributor

@billriess billriess commented May 25, 2020

This PR attempts to add provider validation for cookie-based authentication. This needs to be tested locally, as it may re-introduce #1243

@RahulDey12
Copy link

I have opened a new PR regarding this #1283

@driesvints driesvints changed the title Added provider check to cookie authentication [9.x] Added provider check to cookie authentication May 26, 2020
@taylorotwell
Copy link
Member

I have two PRs open for this now but neither has any description of what is being fixed or addressed. We need a lot more detail 😅

@RahulDey12
Copy link

@taylorotwell I've Updated the description of #1283

@taylorotwell
Copy link
Member

OK. I don't understand. Are you offering that PR as an alternative to this? Is it complimentary to this?

@taylorotwell
Copy link
Member

@billriess also need more explanation on this. What is it fixing? How does it fix it? Why might it re-introduce a bug?

@taylorotwell
Copy link
Member

I'll wait for @RahulDey12 to submit a PR with passing tests.

@billriess
Copy link
Contributor Author

billriess commented May 26, 2020

Sorry, @taylorotwell I submitted this quickly because @driesvints asked me to look into it and I was a bit rushed on time. I can update the description later. This should be an easy fix but just needed manual testing which I didn't have time to set up originally.

@driesvints
Copy link
Member

I'll try to take a look at this myself as soon as I have Cashier Paddle wrapped up.

@driesvints
Copy link
Member

I'm gonna let this one go. We've only gotten one request to add multi auth to the fresh api token middleware so I don't think it's worth investing time into it atm.

@RahulDey12
Copy link

Do you have any plan to achieve this then I can do it.

@driesvints
Copy link
Member

Not at this time sorry.

@billriess
Copy link
Contributor Author

@driesvints did you run into issues when you tested it?

@driesvints
Copy link
Member

I haven't tested this sorry. Wanted to focus on other things. We've only gotten one request so far for this so we didn't feel that it would warrant to put in more time at this moment.

@billriess
Copy link
Contributor Author

billriess commented Jun 6, 2020 via email

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