Skip to content

Conversation

@mbenadda
Copy link
Contributor

@mbenadda mbenadda commented Mar 30, 2020

Hi,

I've encountered a problem where responses would be treated as though login had failed when they contained a Success status code.

I was able to track it down to ticket verification picking up status codes ending with ':Success'. This works fine when the response is namespaced (eg the code is ns2:Success) but not otherwise (the code is Success).

As far as I know, namespacing of the soap response is not necessary. If the response is not namespaced, the ":" in the string matcher will cause the client to fail authentication when
it had succeeded.

I think we can safely remove the : in the expected status code end substring to support non namespaced responses.

@codecov
Copy link

codecov bot commented Mar 30, 2020

Codecov Report

Merging #39 into master will not change coverage by %.
The diff coverage is 0.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master      #39   +/-   ##
=======================================
  Coverage   66.97%   66.97%           
=======================================
  Files           1        1           
  Lines         215      215           
=======================================
  Hits          144      144           
  Misses         71       71           
Impacted Files Coverage Δ
cas.py 66.97% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bbae7b9...9e8576d. Read the comment docs.

Namespacing of the soap response is not necessary. If the response
is not namespaced, the status code would be eg. `"Success"` instead
of `"ns2:Success"` and cause the client to fail authentication when
it had succeeded.

We can remove the `:` in the expected status code end substring
to support non namespaced responses.
@mbenadda mbenadda force-pushed the fix-saml-ticket-verification branch from db8275c to 9e8576d Compare March 30, 2020 16:43
@mingchen
Copy link
Contributor

Thanks for your contribution!

@mbenadda
Copy link
Contributor Author

mbenadda commented Jun 4, 2020

Hey @mingchen! I hope you are alright.

Could we possibly merge this? I'm not in a hurry as I made a fork of python-cas to use in the meantime, but it would be nice if we could get this in 😅

@mingchen mingchen merged commit e68a2a7 into python-cas:master Jun 5, 2020
@mingchen
Copy link
Contributor

mingchen commented Jun 5, 2020

Merged. Thanks @mbenadda.

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.

2 participants