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

Revert "Fix case where sqlachemy db driver raises exception" #70

Merged
merged 1 commit into from
May 9, 2019

Conversation

jwag956
Copy link
Member

@jwag956 jwag956 commented May 9, 2019

Reverts #69

@fmorato
Copy link

fmorato commented May 9, 2019

What happened?

@jwag956 jwag956 merged commit d331660 into master May 9, 2019
@jwag956
Copy link
Member Author

jwag956 commented May 9, 2019

@fmorato In my application I am using postgres and psycopg2. It turns out that psycopg2 is very picky about mixing formats (like trying to query an integer field with text string; or trying to use the 'lower' function on an integer. sqlite doesn't seem to mind this.
Secondly, when psycopg2 throws such an error (DataError or others) it aborts the transaction, and wont execute any other queries until that transaction is closed.
I am not even sure who started a transaction - and certainly don't want that code to be messing with sessions and things like that.
This whole notion of taking random input and trying to match various columns seems slightly suspect to me - in fact there are only 3 places in the forms that actually use get_user() (instead of find_user which takes explicit {attr:value} parameter.

For example - with login - it calls get_user with the email form input - why doesn't it call find_user(email: form_email_input). If you want to login with some other form variable sure seems like you already have to subclass LoginForm - so could just change the validator as well (sort of annoying since that would require copying a bunch of checks) - but certainly doable.
Since this topic has gotten so much attention - I am probably missing something - which is why I am interested in your (and others) use case and what you have done/are running into.

@jwag956
Copy link
Member Author

jwag956 commented May 9, 2019

I should add - working on a better fix - by using available metadata from sqlalchemy to actually check the types of the column prior to issuing the query. This I think can make all this backwards compatible.

@fmorato
Copy link

fmorato commented May 11, 2019

^^ thanks for explaining.
I'm looking into how to set up authentication and authorization for an API. flask-security has most of the functionality I need, including user management and sending emails. It feels quite appealing but indeed because it was originally made to work with forms and now it's being adapted to work also without forms the package's api is not so clear to me.

About the numeric fields for identifiers, even if the content is numeric, I'd just create a text column (VARCHAR). Handles more use cases and is less picky, as you say. I think it's better for this kind of library to have a more generalist solution with easier to maintain code than trying to solve "everyone's" use case and adding too much complexity.

I get it that maintaining legacy code can be challenging. My 2 cents is that you can take the liberty ie. of being more strict about the code and removing features that may not make sense and add those that do. Documenting those would be good. I'd also aim for python3.5+ only. Easier to maintain with smaller set of supported versions.

This might be going too much out of scope of this PR but I write it here anyway.
Another issue with this library is that it depends on unmaintained libraries: Flask-Principal, Flask-Mail. Some of the issues you are finding might come from those but I haven't checked them extensively.

@jwag956
Copy link
Member Author

jwag956 commented May 12, 2019

thanks for the comments and thoughts - helps me refine mine.
As I mentioned in my README - I plan on focusing on non-form/API/SPA support - it is actually very close - really just needing support for reset and forgot password (which I have working code for but need to make it more general). As part of that having explicit docs (maybe swagger?) for that is critical.

As for identifiers - I realized that for other ORMs, in fact the code only supported 'id' as the primary key, and then offers to match on other fields - the sqlalchemy code is more complex and I don't know why. As I figured out - I believe they want to support the ability to have a single login form that can say 'enter email or goverment id' and have it match. That seems reasonable.

As for versions - my thought right now is to follow flask's lead - the pallets folks are actively working on those packages and since flask_security is part of that eco-system - seems like a good approach.

I need to look into Flask-Principle/Mail - I know that the Flask-Mail could easily be abstracted out and I agree that seems like a good idea...

I am also looking into basic best practice security - things like signing algorithms, token contents etc since that's what all this is about.

thanks again for your thoughts.

@jwag956 jwag956 deleted the revert-69-fix633 branch May 16, 2019 14:20
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