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

Store add-on passwords in the credentials file as well. #6543

Merged
merged 1 commit into from Mar 10, 2022

Conversation

Pentarctagon
Copy link
Member

This allows storing add-on passwords for uploading in the same credentials file as is currently used by the client when connecting to a multiplayer server. For add-on authors that don't keep their passwords as plaintext in their _server.pbl, this removes the need for them to manually enter their password each time they upload an add-on.

This allows storing add-on passwords for uploading in the same credentials file as is currently used by the client when connecting to a multiplayer server. For add-on authors that don't keep their passwords as plaintext in their _server.pbl, this removes the need for them to manually enter their password each time they upload an add-on.
@github-actions github-actions bot added Add-ons Issues with the add-ons client and/or server. UI User interface issues, including both back-end and front-end issues. labels Feb 26, 2022
@Wedge009
Copy link
Member

I was reminded of #6290, which resolved issues for storing multiple accounts. But this PR doesn't appear to change any of the credentials code anyway. I assume add-on accounts and MP/forum accounts can be different.

@Pentarctagon
Copy link
Member Author

It can - the password is retrieved by server+username, and nobody would be able to connect to add-ons.wesnoth.org for online multiplayer or vice versa.

@ProditorMagnus
Copy link
Contributor

Emails could be in scope too, so that _server.pbl could be published.

@stevecotton
Copy link
Contributor

Will add-on authors want to use the same passphrase for all of their add-ons? I was expecting one-per-add-on, so that ownership of individual add-ons could be passed around if another person becomes interested in maintaining them.

@stevecotton
Copy link
Contributor

We're inconsistent about whether to say "password" or "passphrase". At the moment it seems forum logins and private games use "password", but add-ons use "passphrase".

Some of the "passphrase" strings are server-side and aren't translated, so aren't in the .pot files.

@Pentarctagon
Copy link
Member Author

Will add-on authors want to use the same passphrase for all of their add-ons? I was expecting one-per-add-on, so that ownership of individual add-ons could be passed around if another person becomes interested in maintaining them.

There's nothing stopping it from being server+(author+addon_name), I suppose.

We're inconsistent about whether to say "password" or "passphrase". At the moment it seems forum logins and private games use "password", but add-ons use "passphrase".

campaignd has always called it "passphrase", and that's what it's in the _server.pbl.

@Pentarctagon
Copy link
Member Author

Emails could be in scope too, so that _server.pbl could be published.

Whenever #5866 could be merged, that could be the case, since then the email would be retrieved from the forum database instead of needing to be in the _server.pbl. There isn't any way currently to store it in the credentials though.

id = "remember_password"
definition = "default"

label = _ "Save password locally (encrypted)"
Copy link
Contributor

Choose a reason for hiding this comment

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

campaignd has always called it "passphrase", and that's what it's in the _server.pbl.

But the new UI string says otherwise, unless it's this way to reuse a string from elsewhere.

Copy link
Member Author

Choose a reason for hiding this comment

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

@stevecotton
Copy link
Contributor

Emails could be in scope too, so that _server.pbl could be published.

Whenever #5866 could be merged, that could be the case, since then the email would be retrieved from the forum database instead of needing to be in the _server.pbl. There isn't any way currently to store it in the credentials though.

If the point of having the email is to have a method of contacting the author, maybe the forum account name should be in the _server.pbl instead of the email address. I don't think it even needs #5866, because there's no email validation, so if someone wanted to they could already put another user's email in this field.

@Pentarctagon
Copy link
Member Author

Pentarctagon commented Feb 26, 2022

#5866 is specifically about using a user's forum account, so I don't really follow what you mean by using the forum account without that PR?

@stevecotton
Copy link
Contributor

IIUC, the only reason that campaignd currently needs an email address is so that an admin can contact the author, and the contents of _server.pbl's email field are only used if an admin manually looks at the data. Currently I think campaignd would accept email="octalot on the forums", that's untested but the validation code only seems to be checking that it's not empty. That wouldn't break the automated processes, and you'd still manage to contact the author via a manual process (and it would be exactly as vulnerable to spoofing as the email addresses currently are).

My point is that #5866 adds authentication of the upload, however the ability to use a forum account as a contact address doesn't need that, and it doesn't need the forum server to provide email addresses to campaignd.

@Pentarctagon
Copy link
Member Author

Pentarctagon commented Feb 26, 2022

What automated vs manual processes are you talking about? And how does any of it have anything to do with using the credentials file to store add-on passwords?

Letting people put a forum account as a point of contact without validating that its actually their forum account also makes little sense, and suffers from the same issue that you yourself pointed out with the current email contact field.

@Pentarctagon
Copy link
Member Author

Some additional information perhaps:

  • Originally the password was required to be stored plaintext in the _server.pbl.
  • I added functionality to allow entering the password via a prompt instead of storing it plaintext, but this was inconvenient since it meant re-entering the password every time you uploaded the add-on.
  • This PR resolves that inconvenience by saving that password to the encrypted credentials file.
  • The other PR makes providing an email in the _server.pbl unnecessary, since it can instead be queried from the forum database. It also provides official support for using your forum account for both uploading your add-ons an MP, and could additionally be used for other integrations later (ie: showing the Discord/IRC contact info from their forum profile, send players of a game a notice if the author of an add-on they're using joins their game, etc).

@stevecotton
Copy link
Contributor

stevecotton commented Feb 26, 2022

how does any of it have anything to do with using the credentials file to store add-on passwords?

My last 2 messages were replying to @ProditorMagnus 's suggestion that "Emails could be in scope too, so that _server.pbl could be published". If I've understood that right, it's a suggestion that the mechanism used in this PR for the passphrase could also be used for other fields that the author wants to keep out of a source-control system, even before #5866 is implemented.

This entire thread might just be unnecessary feature-creep.

@Pentarctagon
Copy link
Member Author

Ah, my bad then.

@Pentarctagon
Copy link
Member Author

@stevecotton any issue if I merge this?

@stevecotton
Copy link
Contributor

Will add-on authors want to use the same passphrase for all of their add-ons? I was expecting one-per-add-on, so that ownership of individual add-ons could be passed around if another person becomes interested in maintaining them.

There's nothing stopping it from being server+(author+addon_name), I suppose.

My only question is whether it should have add-on specific passphrases. I'm okay either way, just asking to check it wasn't forgotten.

@Pentarctagon
Copy link
Member Author

Pentarctagon commented Mar 10, 2022

My initial guess/assumption is that people generally use the same passphrase across multiple add-ons for the sake of convenience. At least from what I've seen, passing maintenance of an add-on to someone else usually either doesn't happen at all or happens with the release of a new stable series where there's a brand new add-ons server (and so knowing the password on the old add-ons server doesn't matter).

@Pentarctagon Pentarctagon merged commit 95aaac7 into wesnoth:master Mar 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Add-ons Issues with the add-ons client and/or server. UI User interface issues, including both back-end and front-end issues.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants