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

Register Page / Registration Flow #334

Closed
wants to merge 1,028 commits into from
Closed

Register Page / Registration Flow #334

wants to merge 1,028 commits into from

Conversation

toumorokoshi
Copy link
Contributor

Related to #322, this is a first draft at a registration page / registration flow. As of the initial pull request, it is the full workflow sans sending a confirmation e-mail, and and agreeing to a terms of use.

alex and others added 30 commits March 2, 2014 15:01
All we wanted were 4 random bytes in hex!
Simplify some code, use less confusing crypto nonsense
Fix #212 - Allow configuration of the search index
Document our current configuration options and format
Remove warehouse.download_statistics, it is now linehaul
Enable Warehouse to run on Python 3.3+, Python 2.7, and PyPy
Fix #190 - Document the requirement for uuid-ossp PostgreSQL extension
Enable the use of Camo to remove mixed content warnings
Implement the legacy XMLRPC search API
Convert new lines to line breaks when we can't render the description
Switch to using whitenoise instead of Werkzeug for static files
Document sentry's configuration
Use the new deterministic gzip support in whitenoie 0.12
@toumorokoshi
Copy link
Contributor Author

I was trying to ensure compatibility with Pypi (in terms of data). Here's some notes I jotted down along the way:

  • pypi had the following requirements:
    • username: begin and end with alphanumeric, allowing ._- in the middle
    • email must be alphanumeric or in ._+@- (this is more or less covered with wtforms' email validator
    • Email's are available until a user has successfully verified their account. Until then, the e-mail is not claimed.
  • the accounts_email table has a 'verified' column
    • As far as I could tell, this column wasn't ever set to anything besides False. I originally thought it was used in the user verification somewhere, but I couldn't find code to validate my theory.
  • I had to modify engine logic.
    • Using an 'engine' instance as the top level connection made it impossible to update the e-mail. It looks like sqlalchemy tries to guess what kind of executions need a commit at the end, and "WITH new_values..." (the query e-mail updating used) isn't one of them.
    • I solved this by making a contextmanager that generates a transaction on every request, which is committed it the very end, and having db's always source the engine from the app.
    • You can't rollback the top level transaction in the methods, but we couldn't do that before anyway. If we move to the pattern where the connection is passed along with the method, this'll give us the freedom of rolling back and transacting at will.
  • 'itsdangerous' was added to handle signing and unsigning of keys. I added a secret_key parameter in dev/config.yml which is the seed passed into itsdangerous.
  • as discussed in Warehouse needs a register page #322, I didn't implement openid support or gpg key info, as it sounds like the future of those are up in the air.

Next Steps

If this looks like a good direction, I'm going to add the agreeing to the terms page, and also add the e-mail sending part of it.

@@ -36,7 +36,7 @@ def test_basic_instantiation():
"access_token": "testing",
},
"database": {
"url": "postgres:///test_warehouse",
"url": "postgres:///warehouse",

Choose a reason for hiding this comment

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

Is this change (and the one in tests/test_config.yaml) intentional?

@r1chardj0n3s
Copy link

Apart from the bizarro documentation build error and my couple of notes this looks good (huge changes get fewer comments dontcha know ;)

Not only is it not a valid HTTP header but it also doesn't exist in the API.
@r1chardj0n3s
Copy link

I've fixed the documentation error from master; please rebase.

@toumorokoshi
Copy link
Contributor Author

@r1chardj0n3s thanks! I realize I should probably do smaller commits, but it's a bit hard to chunk full workflows like this...

I'll definitely make sure to keep as small as possible though :)

@dstufft
Copy link
Member

dstufft commented Feb 28, 2015

I'm not going to close this, but I've started porting Warehouse over to Pyramid to ease the concerns of the custom framework-ish thing I've got going on here. That work is on the pyramid branch inside this repository. I'm leaving this open because I'm going to need to port this PR over to that new branch, and I want to be able to re-use the work done here.

@toumorokoshi
Copy link
Contributor Author

@dstufft ok! Let me know if you want any help with that: I'm happy to spend some time to port it over.

@dstufft
Copy link
Member

dstufft commented Mar 17, 2015

If you want to do that, that'd be great :) I wasn't going to ask that since my decision to switch is what invalidated this PR but if you're willing to do that, it'd be great!

@toumorokoshi
Copy link
Contributor Author

Anything I can do to help get Warehouse out there :) Just letting you know I'm going to start the work now. I don't have an ETA, but i'll start a pull request as soon as I have something.

@dstufft
Copy link
Member

dstufft commented Mar 22, 2015

Awesome! If you need any help just let me know. Hopefully the new setup is easier to work on than before.

@dstufft
Copy link
Member

dstufft commented May 4, 2015

@toumorokoshi is it helpful to you to have this Pull Request remain open while you work on getting register support into the Pyramid based Warehouse? If not I'll close it now that you're working on it.

@toumorokoshi
Copy link
Contributor Author

Nope! Just wanted to let people know I'd like to tackle it, but I guess that's apparent now :)

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

Successfully merging this pull request may close these issues.

None yet