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

WebAPI: Allow to specify session cookie name #18384

Merged
merged 1 commit into from
Jan 17, 2023

Conversation

glassez
Copy link
Member

@glassez glassez commented Jan 10, 2023

Closes #18329.

@glassez glassez added the WebAPI WebAPI-related issues/changes label Jan 10, 2023
@glassez glassez added this to the 4.6.0 milestone Jan 10, 2023
@glassez glassez marked this pull request as ready for review January 14, 2023 09:32
@glassez glassez requested review from sledgehammer999 and a team January 14, 2023 09:33
@brvphoenix
Copy link
Contributor

It may be useful to add a command line parameter (and/or an environment variable) to define the cookie name.

@glassez
Copy link
Member Author

glassez commented Jan 15, 2023

It may be useful to add a command line parameter (and/or an environment variable) to define the cookie name.

Environment variable is unlikely to be useful since it will affect all the instances but this option is mostly to use different cookie name per instance.

@brvphoenix
Copy link
Contributor

brvphoenix commented Jan 15, 2023

It may be useful to add a command line parameter (and/or an environment variable) to define the cookie name.

Environment variable is unlikely to be useful since it will affect all the instances but this option is mostly to use different cookie name per instance.

Prepending a single command with environment variable will take effect for that command only, for example, FOO=bar qbittorrent-nox in linux terminal. The windows CMD can also set local variable by SETLOCAL. I'm not familiar with WSL. If the cookie name can be set by a command line parameter, it should also be OK.

@glassez
Copy link
Member Author

glassez commented Jan 15, 2023

It may be useful to add a command line parameter (and/or an environment variable) to define the cookie name.

Environment variable is unlikely to be useful since it will affect all the instances but this option is mostly to use different cookie > Prepending a single command with environment variable will take effect for that command only, for example, FOO=bar qbittorrent-nox in linux terminal. The windows CMD can also set local variable by SETLOCAL. I'm not familiar with WSL.

Ok. Thank you for explanation.

If the cookie name can be set by a command line parameter, it should also be OK.

In any case, let's leave all this for future improvements, if there really is a need for them.

Copy link
Member

@sledgehammer999 sledgehammer999 left a comment

Choose a reason for hiding this comment

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

Token approval. I haven't tested this.

@sledgehammer999
Copy link
Member

sledgehammer999 commented Jan 16, 2023

It may be useful to add a command line parameter (and/or an environment variable) to define the cookie name.

FYI, and since last time I checked, our "cmd options code" automatically creates an ENV lookup for each cmd option. I don't know if this has changed over the last year.
EDIT: To clarify, each option can be both passed as an ENV or cmd option. IIRC ENV takes precedence.

@sledgehammer999
Copy link
Member

IIRC ENV takes precedence.

It's the other way around. See qbittorrent --help for details.

if (cookieName.isEmpty() || (cookieName.size() > 128))
return false;

const QRegularExpression invalidNameRegex {u"[^a-zA-Z0-9_\\-]"_qs};
Copy link
Member

Choose a reason for hiding this comment

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

This is a very general remark and I don't know if this has been discussed already.
For regex we should consider switching to raw string literals. It will improve regex readability by dropping the need for backslash escapes.
However, I don't know if raw string literals can mix with user-defined literals (_qs).

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't know if this has been discussed already.
For regex we should consider switching to raw string literals.

We didn't discuss such a requirement earlier.

It will improve regex readability by dropping the need for backslash escapes.

Personally, I don't care about it in trivial cases like this. In complex regexps, it really makes sense.
However, if you see the point of making this a requirement for regular expressions, I don't mind. But in this case, it would be good to start by converting all existing instances, so that later some newly-minted contributors do not poke their finger there when they are pointed out to such a requirement.

Copy link
Member

Choose a reason for hiding this comment

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

I am not keen into make it a hard requirement. You should treat it more like a suggestion IMO.
Do you think it should be a requirement?

Copy link
Member Author

Choose a reason for hiding this comment

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

Do you think it should be a requirement?

No.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
WebAPI WebAPI-related issues/changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add ability to specify session cookie name in webui settings
3 participants