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

Sac #25

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Sac #25

wants to merge 3 commits into from

Conversation

yamini27
Copy link

I created endpoint for Register and Login page. There is one issue that it uses 'register.html' for register() function which has not been created yet. And username is kept of length of 6 characters considering our Roll no. b1XXXX .Email can be any email for now. @Milind712000 please review this issue.

@yamini27
Copy link
Author

Sir, the Login and register new user pull request (8d885b6) was made by mistake. So, please ignore it and review only Register and Login new user.

Copy link
Member

@Milind712000 Milind712000 left a comment

Choose a reason for hiding this comment

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

  • Rebase and update your PR
  • Move RegisterForm to forms.py
  • Your code is not consistent with current db schema, update the schema or your code
  • store email and admin status in session

@yamini27
Copy link
Author

yamini27 commented Apr 3, 2020

Sir, for the issue #13 , I have made changes in app.py as was asked in Login and Regiter new user ( b3b89bf ). Login and Register ( 52c2819 ) includes session admin status and email which I forgot earlier. RegisterForm is included in Forms ( c0e6071 ). I have made a new template named dashboard such that the user will be directed to dashboard after logging-in. Since the route of Home is '/', it will be shown before login page.Hence, I have made changes accordingly. Sir, Please review these commits and suggest necessary changes.

app.py Outdated Show resolved Hide resolved
app.py Outdated Show resolved Hide resolved
app.py Outdated Show resolved Hide resolved
@yamini27
Copy link
Author

yamini27 commented Apr 9, 2020

Sir, I have made changes as asked. Please review the PR for the issue #13 and suggest further changes.

forms.py Outdated Show resolved Hide resolved
app.py Show resolved Hide resolved
app.py Outdated Show resolved Hide resolved
@karanankit01
Copy link
Collaborator

The current patch of this PR is working fine, you just need a few minor changes

app.py Outdated Show resolved Hide resolved
@yamini27
Copy link
Author

Sir, I have made changes as asked in commit 3365401. I haven't added any specific errors for the possible errors in RegisterForm and have relied on form.errors by default for all form errors. Please review this PR for #13 and suggest further changes.

app.py Outdated Show resolved Hide resolved
app.py Outdated Show resolved Hide resolved
@yamini27
Copy link
Author

Sir, I have made necessary changes in commit da1973a for the issue #13 . It works perfect now. Please review this commit and suggest any further changes.

app.py Outdated Show resolved Hide resolved
app.py Show resolved Hide resolved
@yamini27
Copy link
Author

Sir, made the changes for the issue #13 . Please review it and suggest any further changes.

@yamini27
Copy link
Author

Sir, I have made commit for the issue #13 in b5ea0ef. Now, it's working fine. Please review it and suggest further changes.

app.py Show resolved Hide resolved
@yamini27
Copy link
Author

Sir, I've made changes for the issue #13 . Please review the commit and suggest any further changes.

app.py Outdated Show resolved Hide resolved
app.py Outdated Show resolved Hide resolved
@yamini27
Copy link
Author

Sir, I have made changes and squashed all commits in 5dd97d8 for the issue #13 . Please review it and suggest any further changes.

.gitignore Outdated
@@ -0,0 +1,3 @@
__pycache__/
Make_Tables/__pycache__/
.sampleenv
Copy link
Member

Choose a reason for hiding this comment

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

  • gitignore .env instead of .sampleenv and add a newline at the end

app.py Outdated
else:

# Execute query
mycursor.execute("INSERT INTO Users(userID, email, password, name, admin) VALUES(%s, %s, %s, %s, %s)", (userID, email, password, name, admin))
Copy link
Member

Choose a reason for hiding this comment

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

  • check and make sure these variables have no unesscessary special characters, eg. name = "Milind; DROP TABLE USERS;"

Copy link
Member

Choose a reason for hiding this comment

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

Does this mean SQL Injections?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah , I guess this has to be done to handle SQL injections

app.py Outdated

if request.method == 'POST' and form.validate():

if (form.userID.data[0] == 'b' or form.userID.data[0] == 'd' or form.userID.data[0] == 't' or form.userID.data[0] == 's' or form.userID.data[0] == 'v' ) and form.userID.data[1:6].isdecimal :
Copy link
Member

Choose a reason for hiding this comment

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

  • Move this check to RegisterForm definition, use a custom validator

@yamini27
Copy link
Author

Sir, new commit bd363bb for the issue#13 has the asked changes. Please review it and suggest any further changes.

@@ -5,9 +5,9 @@
directory = os.getcwd()
envindex = directory.find('Make_Tables')
if envindex!=-1: #If current working directory is Make_Tables
load_dotenv(directory[:envindex]+"/.env")
load_dotenv(directory[:envindex]+"/.sampleenv")
Copy link
Member

Choose a reason for hiding this comment

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

  • read from .env only, .sampleenv is so that people know what their .env requires.
    (This is so that people don't have to worry about git tracking changes to .env also add a step to readme about creating .env using .sampleenv)

@yamini27
Copy link
Author

Sir, I made asked changes for the issue#13 in commit d7675e5. Please review it and suggest any further changes.

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.

5 participants