-
Notifications
You must be signed in to change notification settings - Fork 1
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
Feature/login frontend integration #66
Conversation
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.
Please check the comments.
### To use it for oauth, set a redirect url | ||
### The clientId of the created/found auth client will be printed to the console on startup | ||
#GROPIUS_DEFAULT_AUTH_CLIENT_NAME=initial-client | ||
#GROPIUS_DEFAULT_AUTH_CLIENT_ID=01234567-89ab-cdef-fedc-ba9876543210 |
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 such an ID be public visible in the repository?
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.
this one is just for development
it's just a random UUID, imho it does not really matter
of course you should never ever deploy it in production using dev mode
@@ -156,7 +156,8 @@ export class AuthClientController { | |||
} else { | |||
newClient.isValid = true; | |||
} | |||
if (input.requiresSecret != undefined) { | |||
newClient.clientSecrets = [] |
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 this list nonempty or null before, or why do you assign an empty list? Especially, since it is not filled in the remaining method.
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.
afaik it's undefined before
@@ -71,7 +71,7 @@ export abstract class StrategyUsingPassport extends Strategy { | |||
passportStrategy, | |||
{ | |||
session: false, | |||
state: jwtService.sign(authStateData), | |||
state: jwtService.sign(authStateData), // TODO: check if an expiration and/or an additional random value are needed |
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 assume the TODO comment is on intention. Perhaps also create an issue for it so we do not miss the TODO?
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.
done
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.
Open questions were answered. PR is ready to merge.
probably more work to do, but I want to make a realease...