Documentation for drupal_get_token() should mention that it doesn't work correctly for anonymous users

Thanks for filing this issue -- it does seem like a good thing to document.

I think the documentation you've written is clear.... I do have a question though. Is the user session really different on each page request even for anonymous users, or is that only true if they have disabled cookies?

If it is correct, the documentation needs to be re-wrapped -- no documentation line should exceed 80 characters.

And we would also need to put this into Drupal 8. The function exists in D8 (even though it is deprecated, it still has documentation). And we should also put the docs into: \Drupal\core\Access\CsrfTokenGenerator::get(), which is waht that deprecated function ends up calling.

The generated token stays identical for the same $value - otherwise, the function would defeat its purpose.

However, the situation for users that do not have a session is more hairy. I'm not sure whether the function can be used for users without a session in the first place (unless it auto-starts a session).

That needs investigation, and possibly a functional fix.

In any case, the docs should not talk about authenticated/anonymous users. The key requirement and differentiation is a user session.

Yeah, technically it is about users who don't have a persistent session. All authenticated users have a persistent session, and nearly all anonymous users do not have a persistent session. So I think it still makes sense to talk about anonymous users in some way... but this part "... and the user session is different on every page request for anonymous users..." is not technically quite accurate.

So we probably need to reword this. Maybe something like:

Note that this function generates a token using the user session. For most anonymous users, the user session is different on every page request, so this function will not generate a consistent token that can be used for validation on the next request.

Is that more accurate and also clear?

We also need to have the docs on
\Drupal\core\Access\CsrfTokenGenerator::get()
in D8, in addition to where you've put it. Thanks!

I wouldn't say that anon users have a different session, they have *no* session.

And because they have no session, session_id() will generate a new session_id() for every request.

OK, maybe this:

Note that this function generates a token using the user session. Most anonymous users do not have a user session, so the session ID that is used in generating the token will be different on every page request, and the token will not be constant across page requests. This means that this function will not generally be useful for anonymous users.

@Berdir:

I wouldn't say that anon users have a different session, they have *no* session.

And because they have no session, session_id() will generate a new session_id() for every request.

I don't think that is correct? Anonymous users have a session as soon as you start a session. Any code can start a session at any time.

To do so, AFAIK, all you need to do is to put some value into $_SESSION and Drupal will automatically start a session prior to sending HTTP response headers.

Since this function may be called prior to having a session started, the code that intends to use a token is probably responsible for manually starting a session upfront.

Manually starting a session is as easy as calling drupal_session_start().

@sun: Yes, but jhodgdon's version is fine, as it says most anon users have no session.

The point is that the session is not persisted if there's nothing in $_SESSION by the end of the request, that's why session_id() always returns a different session id if you call this when no session exists.

Maybe this then:

Note that this function generates a token using the user session. Most anonymous users do not have a user session, so the session ID that is used in generating the token will be different on every page request, and the token will not be constant across page requests. This means that this function will not be useful for anonymous users unless you also make sure they have a non-empty session during the first request.

The generated token is based on the session ID of the current user. Normally,
anonymous users do not have a session, so the generated token will be
different on every page request. To generate a token for users without a
session, ensure to manually start a session prior to calling this function.

@see drupal_session_start()

Is just starting a session sufficient, or do you also have to set some kind of a variable in it?

Wording update, assuming this is sufficient: take out "ensure to" in #12.

(note: @see lines go at the bottom of the function docs).

@Jaypan -- too bad! I thought you were doing fine. :) All we need is:

a) Different wording for more accuracy -- see #12/13 and previous discussion. (this also needs to happen for 7)

b) An @see added at the end -- see #13 (this also needs to happen for 7)

c) The text also added to \Drupal\core\Access\CsrfTokenGenerator::get() -- which you can find in
core/lib/Drupal/Core/Access/CsrfTokenGenerator.php

in addition to the locations you had in the previous patch.

But if you'd prefer not to do the d8 patch, that's fine too. Thanks for your help so far!

RE #17, I've seen this on other issues but am not currently on this one. You can report a bug to the Drupal.org d7 QA project if you want. It may depend on your browser width?

RE #16, yes, the behavior is the same in Drupal 8. The CsrfTokenGenerator::get() method is exactly the same as drupal_get_token() in D7, and the behavior of "anonymous users do not generally have sessions" is also the same. Thanks for checking!

See wording comment in #13 -- please remove "ensure to" -- that isn't good English wording.

Other than that, the patch looks fine to me, thanks!

Thanks!

+++ b/core/lib/Drupal.php
@@ -548,8 +548,15 @@ public static function languageManager() {
+   * The generated token is based on the session ID of the current user. Normally,

+++ b/core/lib/Drupal/Core/Access/CsrfTokenGenerator.php
@@ -55,6 +55,11 @@ public function setCurrentUser(AccountInterface $current_user = NULL) {
+   * The generated token is based on the session ID of the current user. Normally,

More then 80 chars.

By my count, without the "+" that is the patch file but not the real file, the count is exactly 80. Should be OK.

And Hallelujah!!! We don't have to go to "Update this issue" in order to change issue status any more!!!!!!!!!!!!!!

No, just wait for it. What does help is not having it assigned to you.

@jhodgdon usually takes care of documentation issues within a few days, it is common to wait atleast day or two, in case someone has some feedback.

Yes, please be patient. I do not normally do Drupal contrib work on the weekends, and I was out yesterday, and I have a paying job in addition to the time I volunteer to the Drupal project. Sometimes it takes me up to a week to make a commit.

Also, as Berdir suggested, we usually try to wait a day or two after an issue is marked "RTBC" before it gets committed. There are a lot of people active in Drupal Core development who watch the "RTBC" queue for newly-marked issues and will review the patches, sometimes finding problems the original reviewers missed. So, the delay improves the quality of the Drupal Core code, and avoids a lot of problems.

Unfortunately I cannot commit this patch right now. There is an issue
#1996238: Replace hook_library_info() by *.libraries.yml file
which is marked "avoid commit conflicts" which touches some of the same files as this patch. Although they may not directly conflict, I am extra-careful and usually try to avoid committing patches that touch the same files as patches tagged "avoid commit conflicts". So we'll have to wait until that one is done. Sorry!

I'm the author of that patch and this patch here does not conflict with it — you can go ahead here, thanks! :)

sun: well I've lately been yelled at several times for people using sandboxes who don't even like to have non-conflicting patches touching the same files get committed. I don't like being yelled at (it wasn't you) for things I do during my volunteer time, so I'm being extra cautious. So... thanks but I think I'll still wait.

If one of the other committers wants to commit this they can. :)

If #1996238: Replace hook_library_info() by *.libraries.yml file is the only reason for waiting, then that entire issue/patch is 100% under my control, and this fine patch here does not conflict with it, because it does not touch a hook_library_info() implementation — resolving merge conflicts due to changes in those implementations is the only reason for why that issue is tagged with Avoid commits conflicts. A change like this here, even if it would cause a diff context conflict, is trivial for me to cope with over there.

In other words, the only remotely possible person that could "yell" at you would be me with regard to that particular issue, and I assure that it's going to be fine. :-)

Cool, here goes (and some other patch commits coming soon) -- committed to 8.x.

Needs a backport to 7.

Thanks! Looks good.

Thanks again all! Committed to 7.x.

The '@see drupal_session_start()' was missed off the backport...

drupal_token_anonymous.patch

This patch adds a parameter to drupal_get_token() and changes the last parameter in drupal_valid_token(), making it possible to get an IP-based token for anonymous users if they do not have a session. If they do have a session, that's always used, since it'll be more accurate than IP if it's available.