-
Notifications
You must be signed in to change notification settings - Fork 145
Fix unsigned authentication request to POST endpoint #167
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
Conversation
1630783
to
a59fe3e
Compare
@knaperek are build checks disabled for PRs in this project? Because the last 5 commits that went into the master branch all have failing tests. |
@RouganStriker have you tested allow_create? I'll check It tomorrow and I'll report here, if something changed and why I coded that way. Regarding bytes/str We know about this issues with latests Django released. We should find a compatibile patch for all the supported django releases Nice shoot |
I verified that the attribute was being set in the AuthenticationRequest. Much appreciated if you could test it out as well since I don't have an instance that uses that attribute. |
thank you @RouganStriker with this we should definitely made the code better in readability and without ambiguity: tested |
a59fe3e
to
c351c9b
Compare
Looks fine to me; I'm not going to pull it into this PR since you already made one for that. And all tests are fixed now with the latest commit. |
@RouganStriker Is there any chance we can get this fixing merged to the master soon. |
I need to revisit this MR, some fixes covered by this PR have been merged in via other PRs. |
c351c9b
to
d601957
Compare
d601957
to
c52061b
Compare
* Fix error casting request_xml to bytes when request_xml is an object
c52061b
to
6ec4b67
Compare
@knaperek could you take a look at this PR? CI checks failed since the master branch is failing as well. |
I see. Py38 seems to have a different ordering on those string chunks. @RouguanStriker I think that this is an important PR for v0.18.2, please resolve conflicts and don't worry about the concurrent PR, that's my business, do the things how you think that they should be done, we're ready for merge |
Regarding the failing tests (and them being disabled), this gets fixed with #186 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Regarding the failing tests (and them being disabled), this gets fixed with #186
Let's complete the revision on that PR, meanwhile this PR can be merged as is, just a final check by his author I think that's needed
thank you @RouganStriker without your help I would not have the possibility to find this bug, I use signed authn requests |
Fixes #168