Skip to content
This repository has been archived by the owner on Feb 8, 2018. It is now read-only.

Fix usernames restriction #4100

Merged
merged 3 commits into from
Jul 28, 2016
Merged

Fix usernames restriction #4100

merged 3 commits into from
Jul 28, 2016

Conversation

ghost
Copy link

@ghost ghost commented Jul 23, 2016

This aims to fix H1-128121.

We're currently listing all the existing files inside www_root and comparing the username to each file/folder to ensure an user won't try to use it. However, there is a mapping between the files and the website route (lookup.json.spt is served aslookup.json), so the comparison is little bit more complicated.

I'm just adding a list of forbidden extensions, and it will to to match the list of files with username, username.txt, ... It won't really affect performances since it's only called when creating an account or changing the username. I would really know if somebody has a more elegant way to do it 🐱

@ghost ghost added the Review label Jul 23, 2016
@ghost
Copy link
Author

ghost commented Jul 28, 2016

@whit537 : do we need something else to be added to this PR?

@chadwhitacre
Copy link
Contributor

Let's see ...

@chadwhitacre
Copy link
Contributor

Rebased on master, previous head was 6f054cc.

@chadwhitacre
Copy link
Contributor

I want to go over this to make sure our logic precisely matches what Aspen does ...

@ghost
Copy link
Author

ghost commented Jul 28, 2016

Sure!

# file existing on the web_root folder.
for ext in ['', '.spt', '.ico', '.txt', '.osdd']:
if (lowercased + ext) in gratipay.RESTRICTED_USERNAMES:
raise UsernameIsRestricted(suggested)
Copy link
Contributor

Choose a reason for hiding this comment

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

What if we took a different approach entirely, and actually ran /newusername through the Aspen request processor? That would match the condition more exactly, and would be more future proof.

Copy link
Contributor

Choose a reason for hiding this comment

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

A redirect (to either /username/ or /~username) would indicate that the username is available. A 200 would indicate that it is not.

Copy link
Contributor

@chadwhitacre chadwhitacre Jul 28, 2016

Choose a reason for hiding this comment

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

That's basically an internal redirect, though, which we don't have good facilities for in Aspen. The test harness is the closest we have, and we don't want to import all that machinery into production. Hmm ...

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually, all we care about is dispatch. Maybe we can isolate that?

Copy link
Contributor

Choose a reason for hiding this comment

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

Hrm, not easily. Dispatch is embedded in Gratipay's algorithm, and it seems that we'd want to test the real algorithm.

@chadwhitacre
Copy link
Contributor

I'm trying out an approach based on an internal redirect ...

@chadwhitacre
Copy link
Contributor

Now I'm giving up on that approach and suggesting 5592272.

Why do we need to check .ico, .txt, and .osdd? Yes, those file extensions appear in the directory listing, but a username of robots won't conflict with robots.txt, will it?

@ghost
Copy link
Author

ghost commented Jul 28, 2016

I completely agree with all of your remarks, my change was somehow clumsy. I need to try to improve my PRs in the future!

(wow, great reactivity of Github concerning the link issue!)

@chadwhitacre
Copy link
Contributor

Rebased on master. Previous head was d2a25a3.

@chadwhitacre
Copy link
Contributor

@Nashe Cool, no worries. PRs are better than no PRs! :)

I'm ready to merge when Travis is green ...

@chadwhitacre chadwhitacre merged commit 82ddbed into master Jul 28, 2016
@chadwhitacre chadwhitacre deleted the fix-user-restriction branch July 28, 2016 20:22
@chadwhitacre
Copy link
Contributor

"PR early, PR often!" 💃

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant