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

Fix session/cookie related issues introduced with the privilege separation patches #473

Closed
wants to merge 2 commits into from

Conversation

simo5
Copy link
Contributor

@simo5 simo5 commented Feb 16, 2017

Fixes two bugs opened recently about double cookies being returned and ccache removal

Removing the ccache goes too far as it will cause unrelated sessions to
fail as well, this is a problem for accounts used to do unattended
operations and that may operate in parallel.

Fixes https://fedorahosted.org/freeipa/ticket/6682

Signed-off-by: Simo Sorce <simo@redhat.com>
This removes one of the 2 identical copies of the ipa_session cookie

Fixes https://fedorahosted.org/freeipa/ticket/6676

Signed-off-by: Simo Sorce <simo@redhat.com>
@abbra
Copy link
Contributor

abbra commented Feb 16, 2017

LGTM

@abbra abbra self-assigned this Feb 16, 2017
Copy link
Member

@tiran tiran left a comment

Choose a reason for hiding this comment

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

minor style issue


logout(ccache_name)
setattr(context, 'logout_cookie', '')
Copy link
Member

Choose a reason for hiding this comment

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

why not context.logout_cookie = ''? setattr is only needed when the first or second argument is variable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

for consistency, all manipulations of context happen via setattr/getattr/delattr

@abbra abbra added the ack Pull Request approved, can be merged label Feb 16, 2017
@MartinBasti MartinBasti added the pushed Pull Request has already been pushed label Feb 17, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ack Pull Request approved, can be merged pushed Pull Request has already been pushed
Projects
None yet
4 participants