-
Notifications
You must be signed in to change notification settings - Fork 24
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
Auth #18
Auth #18
Conversation
Fixed Spacing in Navbars
JWT_REFRESH_SECRET= | ||
JWT_SECRET= | ||
GOOGLE_CLIENT_ID= | ||
GOOGLE_CLIENT_SECRET= |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Aksh-Bansal-dev add a URL to the page where users need to register for google client id and secret.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it will be better to add a URL to the official docs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Aksh-Bansal-dev no URL added here.
@@ -36,6 +40,7 @@ mongoose.connect( | |||
// ROUTES |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Aksh-Bansal-dev I want to propose one thing......we should create tables(collection) in the database if a developer/user is setting up the database for the first time, just check and create empty collections directly.
or am I forgetting something and it automatically creates one when it is referenced?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We just have to create a database in MongoDB, and the rest is handled by the mongoose. We don't need to create collections manually.
frontend/src/App.tsx
Outdated
React.useEffect(() => { | ||
updateAccessToken(setLoading); | ||
}); | ||
// if (loading) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should remove the comments or explain why they are present there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I forgot to remove unnecessary comments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just remove these comments and squash so that I can merge this PR thx
@@ -0,0 +1 @@ | |||
_ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is there actually a file named _ ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, it's for husky
|
||
const RegistrationForm: React.FC = () => { | ||
const classes = useStyles(); | ||
// const [login, setLogin] = useState(true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Aksh-Bansal-dev more comments
<Typography className={classes.or} align="center"> | ||
OR | ||
</Typography> | ||
{/* {login ? <Login setLogin={setLogin} /> : <Signup setLogin={setLogin} />} */} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here
@@ -1,7 +1,7 @@ | |||
import React from "react"; | |||
import NavBar from "../../components/Navs/Navbar"; | |||
import { createStyles, makeStyles } from "@material-ui/core/styles"; | |||
import { Container } from "@material-ui/core"; | |||
// import { Container } from "@material-ui/core"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove these comment lines
@@ -1,10 +1,3 @@ | |||
.fullpage { | |||
/* display: grid; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here
server/src/model/User.ts
Outdated
clubs?: string[]; | ||
authority: "member" | "admin"; | ||
clubs?: clubObj[]; | ||
// authority: "member" | "admin"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here
server/src/model/User.ts
Outdated
}, | ||
}, | ||
], | ||
// authority: { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here
passport.authenticate("google", { | ||
scope: ["profile", "email"], | ||
session: false, | ||
// scope: ["https://www.googleapis.com/auth/plus.login"], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
here as well
@@ -4,44 +4,39 @@ import { getAccessToken, sendRefreshToken } from "../utils/tokenstuff"; | |||
import { verify } from "jsonwebtoken"; | |||
const Router = express.Router(); | |||
|
|||
Router.post("/refresh_token", async (req, res) => { | |||
// console.log(req); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
here
server/src/utils/tokenstuff.ts
Outdated
@@ -24,6 +24,7 @@ export const sendRefreshToken = (res: Response, user?: User): void => { | |||
), | |||
{ | |||
httpOnly: true, | |||
// path: "/", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
here
Issue: #17