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

authentication #594

Open
wants to merge 74 commits into
base: trunk
Choose a base branch
from
Open

authentication #594

wants to merge 74 commits into from

Conversation

glyph
Copy link
Member

@glyph glyph commented May 18, 2023

The sessions layer is missing some stuff. This is an omnibus PR that adds it, and will add docs for it.

I would recommend starting the review by looking through the new documentation at https://klein--594.org.readthedocs.build/en/594/ — specifically https://klein--594.org.readthedocs.build/en/594/introduction/3-forms.html and https://klein--594.org.readthedocs.build/en/594/introduction/4-auth.html .

It's probably too big to land as-is. In fact, some parts of this are probably entirely new, separate projects that should be a dependency. Some of this stuff might be worth spinning out, but looking at the documentation, it would be difficult to land it piecemeal and get a cohesive story about what it's all for.

@codecov
Copy link

codecov bot commented May 18, 2023

Codecov Report

Attention: 4 lines in your changes are missing coverage. Please review.

Comparison is base (6b569bf) 99.07% compared to head (cc87fda) 99.33%.

❗ Current head cc87fda differs from pull request most recent head c243f63. Consider uploading reports for the commit c243f63 to get more accurate results

Files Patch % Lines
src/klein/storage/passwords/_scrypt.py 94.02% 4 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##            trunk     #594      +/-   ##
==========================================
+ Coverage   99.07%   99.33%   +0.26%     
==========================================
  Files          46       71      +25     
  Lines        3986     5591    +1605     
  Branches      531      770     +239     
==========================================
+ Hits         3949     5554    +1605     
- Misses         23       26       +3     
+ Partials       14       11       -3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

glyph added 29 commits May 30, 2023 23:28
- move memory session store to its own dedicated package
- implement MemoryAccountStore (i.e. ISimpleAccount/ISimpleAccountBinding authorizers)
at least on pypy3.9, typing.Protocol appears to runtime-check its args to make
sure they are all TypeVars (i.e. not ParamSpecs)
@glyph glyph marked this pull request as ready for review June 6, 2023 10:12
@glyph glyph requested a review from a team as a code owner June 6, 2023 10:12
@glyph
Copy link
Member Author

glyph commented Jun 6, 2023

There's a lot here, but I think now that there is documentation it's possible to get a holistic view of the whole shebang.

@glyph glyph requested a review from wsanchez June 6, 2023 10:13
@adiroiban
Copy link
Member

Hi @glyph

I don't know what to say / do about this PR.

I have never used klein and I don't have experience in terms of API design for micro web frameworks.

Also, for templating I am just using Jinja2... and I am perfectly fine with IResource.putChild

This is one reason why I think that is best for klein to be a separate repo/project, and not merged into twisted/twisted.

but this PR is in review for a long time, and it looks like nobody else cared about this API :(

So if you are using this, I think is best to merge this as it is, at least you don't have to use a fork of klein.

at a quick look, the PR looks good.

So as long as the tests pass, I think is best to merge it.

thanks

@glyph
Copy link
Member Author

glyph commented Sep 26, 2023

This is one reason why I think that is best for klein to be a separate repo/project, and not merged into twisted/twisted.

Yeah, I think Klein has a sufficiently distinct idiom that it should remain separate. That said some things probably ought to be merged back…

@glyph
Copy link
Member Author

glyph commented Sep 26, 2023

So if you are using this, I think is best to merge this as it is, at least you don't have to use a fork of klein.

Thanks!

Right now my plan is to put the large-new-library bit of this — https://github.com/glyph/dbxs — into separate package so it can have its own documentation and not have a big digression in the middle of the Klein docs about a whole new database thing just so we can avoid a big sqlalchemy dependency. But once I've released that, I'll land this on the strength of this review.

@adiroiban
Copy link
Member

Looking forward to dbxs.

For my project, which only simple sqlite access, I ended up with a custom layer to prevent sqlinjection, construct simple select queries, and handle migration between schema changes, and wrapping the thread based API for sqlite to a deferred API.

have you checked peewee ?
I remember checking it and it was looking ok.
I don't remember why I decided not to use it

@adiroiban
Copy link
Member

As it looks like others don't care that much about klein, I think that as long as all tests/checks pass on this PR, it can be merged.

Otherwise, I don't know when it will be reviewed.

If this is merged, maybe people will start using it and send feedback later.

Cheers

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

2 participants