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

Feature/login frontend integration #66

Merged
merged 7 commits into from
Nov 18, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions login-service/.env.dev
Original file line number Diff line number Diff line change
Expand Up @@ -132,9 +132,11 @@ GROPIUS_BCRYPT_HASH_ROUNDS=10 # default: 10

### Parameter for creating an auth client
### If this is set, an auth client with that name and with no requirement for secrets will be created.
### If you set the (optional) id, it will take precedence and a client will be created, if none with the given id AND name exist
### 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
Copy link
Member

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?

Copy link
Contributor Author

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

#GROPIUS_DEFAULT_AUTH_CLIENT_REDIRECT=http://localhost:1234/redirect

### Checking the consistency of the database entitities on startup
Expand Down
5 changes: 3 additions & 2 deletions login-service/src/api-login/auth-clients.controller.ts
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ export class AuthClientController {
private readonly backendUserSerice: BackendUserService,
private readonly loginDataSerive: UserLoginDataService,
private readonly authClientService: AuthClientService,
) {}
) { }

/**
* Gets all auth clients that exist in the system.
Expand Down Expand Up @@ -156,7 +156,8 @@ export class AuthClientController {
} else {
newClient.isValid = true;
}
if (input.requiresSecret != undefined) {
newClient.clientSecrets = []
Copy link
Member

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.

Copy link
Contributor Author

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

if (input.requiresSecret !== undefined) {
newClient.requiresSecret = input.requiresSecret;
} else {
newClient.requiresSecret = true;
Expand Down
10 changes: 6 additions & 4 deletions login-service/src/backend-services/token.service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ export class TokenService {
private readonly backendJwtService: JwtService,
private readonly activeLoginService: ActiveLoginService,
private readonly loginUserService: LoginUserService,
) {}
) { }

async signBackendAccessToken(user: LoginUser, expiresIn?: number): Promise<string> {
const expiryObject = !!expiresIn ? { expiresIn: expiresIn / 1000 } : {};
Expand Down Expand Up @@ -104,16 +104,18 @@ export class TokenService {
activeLoginId: string,
clientId: string,
uniqueId: string | number,
expiresIn?: number,
expiresInAt?: number | Date,
): Promise<string> {
const expiryObject = expiresIn !== undefined ? { expiresIn: expiresIn / 1000 } : {};
const expiresInObject = (typeof expiresInAt == "number") ? { expiresIn: expiresInAt / 1000 } : {};
const expiresAtObject = (typeof expiresInAt == "object" && expiresInAt instanceof Date) ? { exp: Math.floor(expiresInAt.getTime() / 1000) } : {};
return await this.backendJwtService.signAsync(
{
...expiresAtObject,
client_id: clientId,
},
{
subject: activeLoginId,
...expiryObject,
...expiresInObject,
jwtid: uniqueId.toString(),
secret: process.env.GROPIUS_LOGIN_SPECIFIC_JWT_SECRET,
audience: [TokenScope.REFRESH_TOKEN],
Expand Down
1 change: 1 addition & 0 deletions login-service/src/configuration-validator.ts
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ export const validationSchema = Joi.object({
GROPIUS_DEFAULT_USER_PASSWORD: Joi.string().optional(),
GROPIUS_DEFAULT_USER_STRATEGY_INSTANCE_NAME: Joi.string(),
GROPIUS_DEFAULT_AUTH_CLIENT_NAME: Joi.string().optional(),
GROPIUS_DEFAULT_AUTH_CLIENT_ID: Joi.string().regex(/^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$/i).optional(),
GROPIUS_DEFAULT_AUTH_CLIENT_REDIRECT: Joi.string().optional(),

GROPIUS_DEFAULT_CHECK_DATABASE_CONSISTENT: Joi.string().allow("none", "check", "fix").default("none"),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,16 +18,21 @@ export class CreateDefaultAuthClientService {
private readonly userLoginDataService: UserLoginDataService,
private readonly backendUserService: BackendUserService,
private readonly authClientService: AuthClientService,
) {}
) { }

async createDefaultAuthClient() {
const clientName = process.env.GROPIUS_DEFAULT_AUTH_CLIENT_NAME;
const clientId = process.env.GROPIUS_DEFAULT_AUTH_CLIENT_ID;
const redirectUri = process.env.GROPIUS_DEFAULT_AUTH_CLIENT_REDIRECT;

if (!clientName) {
if (!clientName && !clientId) {
return;
}
const nameObject = clientName ? { name: clientName } : {}
const idObject = clientId ? { id: clientId } : {}
let authClient = await this.authClientService.findOneBy({
name: clientName,
...nameObject,
...idObject,
requiresSecret: false,
isValid: true,
});
Expand All @@ -36,12 +41,18 @@ export class CreateDefaultAuthClientService {
`Valid auth client with name ${clientName} without secret already exists. Skipping creation. Id:`,
authClient.id,
);
if (!authClient.redirectUrls.includes(redirectUri)) {
this.logger.warn(`The existing auth client does not include the redirect url specified as config parameter!
If you require this, remove the existing client OR change the redirect URL via the API`)
}
return;
}

const redirectUri = process.env.GROPIUS_DEFAULT_AUTH_CLIENT_REDIRECT;

authClient = new AuthClient();
if (clientId) {
authClient.id = clientId;
}
authClient.isValid = true;
authClient.name = clientName;
authClient.requiresSecret = false;
Expand Down
1 change: 1 addition & 0 deletions login-service/src/model/postgres/LoginUser.entity.ts
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@ export class LoginUser {
toJSON() {
return {
id: this.id,
neo4jId: this.neo4jId,
username: this.username,
revokeTokensBefore: this.revokeTokensBefore,
};
Expand Down
6 changes: 4 additions & 2 deletions login-service/src/oauth-server/oauth-redirect.middleware.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ export class OauthRedirectMiddleware implements NestMiddleware {
private readonly tokenService: TokenService,
private readonly activeLoginService: ActiveLoginService,
private readonly authClientService: AuthClientService,
) {}
) { }

private handleErrorCases(state: (AuthStateData & OauthServerStateData) | undefined | null, url: URL): boolean {
const errorMessage = state?.authErrorMessage;
Expand Down Expand Up @@ -73,7 +73,9 @@ export class OauthRedirectMiddleware implements NestMiddleware {
throw new Error("Active login expired");
}
state.activeLogin.createdByClient = Promise.resolve(state.client);
state.activeLogin.expires = new Date(Date.now() + expiresIn);
if (state.activeLogin.expires == null) {
state.activeLogin.expires = new Date(Date.now() + expiresIn);
}
const codeJwtId = ++state.activeLogin.nextExpectedRefreshTokenNumber;

state.activeLogin = await this.activeLoginService.save(state.activeLogin);
Expand Down
4 changes: 2 additions & 2 deletions login-service/src/oauth-server/oauth-token.controller.ts
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ export class OauthTokenController {
private readonly authClientService: AuthClientService,
private readonly activeLoginService: ActiveLoginService,
private readonly tokenService: TokenService,
) {}
) { }

private async checkLoginDataIsVaild(loginData?: UserLoginData, activeLogin?: ActiveLogin) {
if (!loginData) {
Expand Down Expand Up @@ -122,7 +122,7 @@ export class OauthTokenController {
activeLogin.id,
currentClient.id,
activeLogin.nextExpectedRefreshTokenNumber,
undefined,
activeLogin.expires ?? undefined
);
return {
access_token: accessToken,
Expand Down
2 changes: 1 addition & 1 deletion login-service/src/strategies/Strategy.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ import { AuthResult, AuthStateData } from "./AuthResult";
export interface StrategyVariable {
name: string;
displayName?: string;
type: "boolean" | "number" | "object" | "string";
type: "boolean" | "number" | "object" | "string" | "password";
nullable?: boolean;
}

Expand Down
2 changes: 1 addition & 1 deletion login-service/src/strategies/StrategyUsingPassport.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

...this.getAdditionalPassportOptions(strategyInstance, authStateData),
},
(err, user: AuthResult | false, info) => {
Expand Down
14 changes: 9 additions & 5 deletions login-service/src/strategies/github/github.service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ export class GithubStrategyService extends StrategyUsingPassport {
* If imsTemplatedFieldsFilter not given, defaults to "https://api.github.com/graphql"
* - authorizationUrl: Oauth authorization URL. Optional, default: "https://github.com/login/oauth/authorize"
* - tokenUrl: Oauth token url. Optional, default: "https://github.com/login/oauth/access_token"
* - userProfileUrl: API URL to request user profile info from. Needs to be specified for GitHib Enterprise instances. Optional
* - clientId: Id of GitHub oauth app. Optional, default: GROPIUS_OAUTH_CLIENT_ID config variable
* - clientSecret: secret of GitHub oaut app. Optional, default: GROPIUS_OAUTH_CLIENT_SECRET config value
* - callbackUrl: Oauth callback url. Should be [URL]/authenticate/:id/callback. Optional, default empty
Expand All @@ -45,13 +46,13 @@ export class GithubStrategyService extends StrategyUsingPassport {
protected override checkAndExtendInstanceConfig(instanceConfig: object): object {
const resultingConfig = instanceConfig;

if (instanceConfig["imsTemplatedFieldsFilter"]) {
const githubUrl = instanceConfig["imsTemplatedFieldsFilter"]["graphql-url"];
if (resultingConfig["imsTemplatedFieldsFilter"]) {
const githubUrl = resultingConfig["imsTemplatedFieldsFilter"]["graphql-url"];
if (!githubUrl) {
throw new Error("At least GitHub URL must be given in imsTemplatedFieldsFilter");
}
} else {
instanceConfig["imsTemplatedFieldsFilter"] = {
resultingConfig["imsTemplatedFieldsFilter"] = {
"graphql-url": "https://api.github.com/graphql",
};
}
Expand All @@ -71,6 +72,7 @@ export class GithubStrategyService extends StrategyUsingPassport {
true,
"https://github.com/login/oauth/access_token",
);
resultingConfig["userProfileUrl"] = checkType(instanceConfig, "userProfileUrl", "string", true);
resultingConfig["clientId"] = checkType(
instanceConfig,
"clientId",
Expand Down Expand Up @@ -122,7 +124,7 @@ export class GithubStrategyService extends StrategyUsingPassport {
} {
return {
username: loginData.data?.username || undefined,
displayName: loginData.data?.username || undefined,
displayName: loginData.data?.displayName || undefined,
email: loginData.data?.email || undefined,
};
}
Expand Down Expand Up @@ -166,6 +168,7 @@ export class GithubStrategyService extends StrategyUsingPassport {
username,
github_id: profile.id,
email: profile.emails[0].value,
displayName: profile.displayName
};
const loginDataCandidates = await this.loginDataService.findForStrategyWithDataContaining(strategyInstance, {
github_id: profile.id,
Expand All @@ -183,9 +186,10 @@ export class GithubStrategyService extends StrategyUsingPassport {
return new passportGithub.Strategy(
{
authorizationURL: strategyInstance.instanceConfig["authorizationUrl"],
tokenURL: strategyInstance.instanceConfig["tokenUrl"],
userProfileURL: strategyInstance.instanceConfig["userProfileUrl"],
clientID: strategyInstance.instanceConfig["clientId"],
clientSecret: strategyInstance.instanceConfig["clientSecret"],
tokenURL: strategyInstance.instanceConfig["tokenUrl"],
callbackURL: strategyInstance.instanceConfig["callbackUrl"],
store: {
store: (req, state, meta, callback) => callback(null, state),
Expand Down
4 changes: 2 additions & 2 deletions login-service/src/strategies/perform-auth-function.service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ export class PerformAuthFunctionService {
private readonly activeLoginService: ActiveLoginService,
private readonly userLoginDataService: UserLoginDataService,
private readonly strategiesService: StrategiesService,
) {}
) { }

public checkFunctionIsAllowed(state: AuthStateData, instance: StrategyInstance, strategy: Strategy): string | null {
switch (state?.function) {
Expand Down Expand Up @@ -84,7 +84,7 @@ export class PerformAuthFunctionService {
let loginData = authResult.loginData;
loginData.data = authResult.dataUserLoginData;
const newExpiryDate = new Date(Date.now() + parseInt(process.env.GROPIUS_REGISTRATION_EXPIRATION_TIME_MS, 10));
if (loginData.expires != null && loginData.expires < newExpiryDate) {
if (loginData.expires != null) {
loginData.expires = newExpiryDate;
}
loginData = await this.userLoginDataService.save(loginData);
Expand Down
27 changes: 22 additions & 5 deletions login-service/src/strategies/userpass/userpass.service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import { AuthResult } from "../AuthResult";
import { StrategyUsingPassport } from "../StrategyUsingPassport";
import { JwtService } from "@nestjs/jwt";
import * as bcrypt from "bcrypt";
import { UserLoginData } from "src/model/postgres/UserLoginData.entity";

@Injectable()
export class UserpassStrategyService extends StrategyUsingPassport {
Expand All @@ -32,14 +33,14 @@ export class UserpassStrategyService extends StrategyUsingPassport {
} {
return {
username: {
name: "string",
name: "username",
displayName: "Username",
type: "string",
},
password: {
name: "string",
name: "password",
displayName: "Password",
type: "string",
type: "password",
},
};
}
Expand All @@ -48,8 +49,12 @@ export class UserpassStrategyService extends StrategyUsingPassport {
return {};
}

private async generateLoginDataData(password: string): Promise<{ password: string }> {
private async generateLoginDataData(
username: string,
password: string,
): Promise<{ username: string; password: string }> {
return {
username,
password: await bcrypt.hash(password, parseInt(process.env.GROPIUS_BCRYPT_HASH_ROUNDS, 10)),
};
}
Expand Down Expand Up @@ -81,7 +86,7 @@ export class UserpassStrategyService extends StrategyUsingPassport {
);

if (loginDataForCorrectUser.length == 0) {
const dataUserLoginData = await this.generateLoginDataData(password);
const dataUserLoginData = await this.generateLoginDataData(username, password);
return done(
null,
{ dataActiveLogin, dataUserLoginData, mayRegister: true },
Expand All @@ -108,4 +113,16 @@ export class UserpassStrategyService extends StrategyUsingPassport {
public override createPassportStrategyInstance(strategyInstance: StrategyInstance): passport.Strategy {
return new passportLocal.Strategy({}, this.passportUserCallback.bind(this, strategyInstance));
}

override getUserDataSuggestion(loginData: UserLoginData): {
username?: string;
displayName?: string;
email?: string;
} {
return {
username: loginData.data?.username || undefined,
displayName: loginData.data?.displayName || undefined,
email: loginData.data?.email || undefined,
};
}
}