-
-
Notifications
You must be signed in to change notification settings - Fork 4k
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
oc_token should be nc_token #3362
Conversation
Signed-off-by: Christoph Wurst <[email protected]>
@ChristophWurst, thanks for your PR! By analyzing the history of the files in this pull request, we identified @nickvergessen, @LukasReschke and @tanghus to be potential reviewers. |
Any noticable impact? |
Dunno, but I hope @LukasReschke does 😉 |
@@ -98,7 +98,7 @@ function __construct($appName, | |||
* @return RedirectResponse | |||
*/ | |||
public function logout() { | |||
$loginToken = $this->request->getCookie('oc_token'); | |||
$loginToken = $this->request->getCookie('nc_token'); |
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.
Please prepare backport of this single change also to all older versions where you changed the cookie name.
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.
okidoke 👷
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.
Nothing too horrible:
The second one should be backported. The first one I'd avoid. @ChristophWurst can you take care of that? THX. |
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.
Tested and works fine 👍
I noticed this inconsistency when I took a look at #3361.
The cookie is set here:
server/lib/private/User/Session.php
Line 795 in a3f8358
@LukasReschke as discussed 😉