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

Simplelogin alias generation only generate random words instead the domain name #13024

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from 1 commit
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
11 changes: 11 additions & 0 deletions libs/common/src/vault/abstractions/cipher-form-uri.service.ts
Copy link
Member

@audreyality audreyality Jan 23, 2025

Choose a reason for hiding this comment

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

๐Ÿค” Was this change requested by the vault team? The solution analysis indicates the website should be passed using an attribute on the component, not a new service.

If the vault team did ask for this service to be created, then it should be integrated within their component, not the shared tools component.

Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
export abstract class CipherFormUriService {
/**
* The current URI value
*/
abstract uri: string;

/**
* Updates the current URI
*/
abstract setUri(uri: string): void;
}
30 changes: 30 additions & 0 deletions libs/common/src/vault/services/cipher-form-uri.service.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
// FIXME: Update this file to be type safe and remove this and next line
// @ts-strict-ignore
Copy link
Member

Choose a reason for hiding this comment

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

โš ๏ธ New services should follow strict type safety.

// eslint-disable-next-line import/no-restricted-paths
import { Injectable } from "@angular/core";
Copy link
Member

Choose a reason for hiding this comment

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

โŒ The lint is correct. @bitwarden/common may not depend upon angular dependencies. Services implemented in common should be manually configured using the JsLibServiceModule.


import { CipherFormUriService as CipherFormUriServiceAbstraction } from "../abstractions/cipher-form-uri.service";

@Injectable({
providedIn: "root",
})
export class CipherFormUriService implements CipherFormUriServiceAbstraction {
private _uri: string = "";

constructor() {}

/**
* Getter for the current URI
*/
get uri(): string {
return this._uri;
}

/**
* Setter for the URI
* @param uri - The new URI to set
*/
setUri(uri: string): void {
this._uri = uri;
}
}
Copy link
Member

Choose a reason for hiding this comment

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

๐Ÿ“ This change also needs to be applied to credential-generator.component.ts.

Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import { I18nService } from "@bitwarden/common/platform/abstractions/i18n.servic
import { LogService } from "@bitwarden/common/platform/abstractions/log.service";
import { IntegrationId } from "@bitwarden/common/tools/integration";
import { UserId } from "@bitwarden/common/types/guid";
import { CipherFormUriService } from "@bitwarden/common/vault/services/cipher-form-uri.service";
import { ToastService, Option } from "@bitwarden/components";
import {
AlgorithmInfo,
Expand Down Expand Up @@ -66,6 +67,7 @@ export class UsernameGeneratorComponent implements OnInit, OnDestroy {
private accountService: AccountService,
private zone: NgZone,
private formBuilder: FormBuilder,
private cipherFormUriService: CipherFormUriService,
Copy link
Member

Choose a reason for hiding this comment

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

โŒ This couples the username generator to the vault. At present, the vault is the only consumer, but it's not the only one. Rewrite this to use an attribute.

) {}

/** Binds the component to a specific user's settings. When this input is not provided,
Expand Down Expand Up @@ -336,7 +338,11 @@ export class UsernameGeneratorComponent implements OnInit, OnDestroy {

private typeToGenerator$(type: CredentialAlgorithm) {
const dependencies = {
on$: this.generate$,
on$: this.generate$.pipe(
map(() => ({
website: this.cipherFormUriService.uri,
})),
),
userId$: this.userId$,
};

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@
import {
BehaviorSubject,
combineLatest,
concat,
concatMap,
distinctUntilChanged,
endWith,
Expand All @@ -15,7 +14,6 @@
Observable,
ReplaySubject,
share,
skipUntil,
switchMap,
takeUntil,
withLatestFrom,
Expand Down Expand Up @@ -123,31 +121,22 @@
const engine = configuration.engine.create(this.getDependencyProvider());

// stream blocks until all of these values are received
const website$ = dependencies?.website$ ?? new BehaviorSubject<string>(null);
const request$ = website$.pipe(map((website) => ({ website })));
const settings$ = this.settings$(configuration, dependencies);

// if on$ triggers before settings are loaded, trigger as soon
// as they become available.
let readyOn$: Observable<any> = null;
if (dependencies?.on$) {
const NO_EMISSIONS = {};
const ready$ = combineLatest([settings$, request$]).pipe(
first(null, NO_EMISSIONS),
filter((value) => value !== NO_EMISSIONS),
share(),
);
readyOn$ = concat(
dependencies.on$?.pipe(switchMap(() => ready$)),
dependencies.on$.pipe(skipUntil(ready$)),
);
}

// generation proper
const generate$ = (readyOn$ ?? settings$).pipe(
withLatestFrom(request$, settings$),
concatMap(([, request, settings]) => engine.generate(request, settings)),
takeUntil(anyComplete([request$, settings$])),
// Handle the on$ emissions which now include the website data
const on$ =
dependencies?.on$ ?? new BehaviorSubject<{ website: string | null }>({ website: null });
// Ensure that settings are loaded before proceeding
const ready$ = combineLatest([settings$, on$]).pipe(first(), share());

// Generation logic
const generate$ = ready$.pipe(
withLatestFrom(on$, settings$),
concatMap(([, { website }, settings]) => {

Check failure on line 135 in libs/tools/generator/core/src/services/credential-generator.service.ts

View workflow job for this annotation

GitHub Actions / Test Results

CredentialGeneratorService โ–บ CredentialGeneratorService generate$ emits a generation only when `onCredentialGeneratorService generate$ emits โ–บ CredentialGeneratorService generate$ emits a generation only when `onCredentialGeneratorService generate$ e...

Failed test found in: junit.xml Error: TypeError: Cannot read properties of undefined (reading 'website')
Raw output
TypeError: Cannot read properties of undefined (reading 'website')
    at website (/home/runner/work/clients/clients/libs/tools/generator/core/src/services/credential-generator.service.ts:135:23)
    at doInnerSub (/home/runner/work/clients/clients/node_modules/rxjs/src/internal/operators/mergeInternals.ts:71:15)
    at outerNext (/home/runner/work/clients/clients/node_modules/rxjs/src/internal/operators/mergeInternals.ts:53:58)
    at OperatorSubscriber._this._next (/home/runner/work/clients/clients/node_modules/rxjs/src/internal/operators/OperatorSubscriber.ts:70:13)
    at OperatorSubscriber.Object.<anonymous>.Subscriber.next (/home/runner/work/clients/clients/node_modules/rxjs/src/internal/Subscriber.ts:75:12)
    at /home/runner/work/clients/clients/node_modules/rxjs/src/internal/operators/withLatestFrom.ts:105:22
    at OperatorSubscriber._this._next (/home/runner/work/clients/clients/node_modules/rxjs/src/internal/operators/OperatorSubscriber.ts:70:13)
    at OperatorSubscriber.Object.<anonymous>.Subscriber.next (/home/runner/work/clients/clients/node_modules/rxjs/src/internal/Subscriber.ts:75:12)
    at /home/runner/work/clients/clients/node_modules/rxjs/src/internal/Subject.ts:68:20
    at Object.errorContext (/home/runner/work/clients/clients/node_modules/rxjs/src/internal/util/errorContext.ts:29:5)
    at Subject.Object.<anonymous>.Subject.next (/home/runner/work/clients/clients/node_modules/rxjs/src/internal/Subject.ts:61:5)
    at Object.next (/home/runner/work/clients/clients/node_modules/rxjs/src/internal/operators/share.ts:225:33)
    at ConsumerObserver.Object.<anonymous>.ConsumerObserver.next (/home/runner/work/clients/clients/node_modules/rxjs/src/internal/Subscriber.ts:161:25)
    at SafeSubscriber.Object.<anonymous>.Subscriber._next (/home/runner/work/clients/clients/node_modules/rxjs/src/internal/Subscriber.ts:119:22)
    at SafeSubscriber.Object.<anonymous>.Subscriber.next (/home/runner/work/clients/clients/node_modules/rxjs/src/internal/Subscriber.ts:75:12)
    at /home/runner/work/clients/clients/node_modules/rxjs/src/internal/operators/throwIfEmpty.ts:50:22
    at OperatorSubscriber._this._next (/home/runner/work/clients/clients/node_modules/rxjs/src/internal/operators/OperatorSubscriber.ts:70:13)
    at OperatorSubscriber.Object.<anonymous>.Subscriber.next (/home/runner/work/clients/clients/node_modules/rxjs/src/internal/Subscriber.ts:75:12)
    at /home/runner/work/clients/clients/node_modules/rxjs/src/internal/operators/take.ts:60:26
    at OperatorSubscriber._this._next (/home/runner/work/clients/clients/node_modules/rxjs/src/internal/operators/OperatorSubscriber.ts:70:13)
    at OperatorSubscriber.Object.<anonymous>.Subscriber.next (/home/runner/work/clients/clients/node_modules/rxjs/src/internal/Subscriber.ts:75:12)
    at /home/runner/work/clients/clients/node_modules/rxjs/src/internal/observable/combineLatest.ts:272:34
    at OperatorSubscriber._this._next (/home/runner/work/clients/clients/node_modules/rxjs/src/internal/operators/OperatorSubscriber.ts:70:13)
    at OperatorSubscriber.Object.<anonymous>.Subscriber.next (/home/runner/work/clients/clients/node_modules/rxjs/src/internal/Subscriber.ts:75:12)
    at /home/runner/work/clients/clients/node_modules/rxjs/src/internal/Subject.ts:68:20
    at Object.errorContext (/home/runner/work/clients/clients/node_modules/rxjs/src/internal/util/errorContext.ts:29:5)
    at Subject.Object.<anonymous>.Subject.next (/home/runner/work/clients/clients/node_modules/rxjs/src/internal/Subject.ts:61:5)
    at /home/runner/work/clients/clients/libs/tools/generator/core/src/services/credential-generator.service.spec.ts:458:11
    at Generator.next (<anonymous>)
    at fulfilled (/home/runner/work/clients/clients/libs/tools/generator/core/src/services/credential-generator.service.spec.ts:5:58)
const request = { website };
return engine.generate(request, settings);
}),
takeUntil(anyComplete([on$, settings$])),
);

return generate$;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import {
UriMatchStrategySetting,
} from "@bitwarden/common/models/domain/domain-service";
import { I18nService } from "@bitwarden/common/platform/abstractions/i18n.service";
import { CipherFormUriService } from "@bitwarden/common/vault/services/cipher-form-uri.service";
import {
FormFieldModule,
IconButtonModule,
Expand Down Expand Up @@ -135,8 +136,10 @@ export class UriOptionComponent implements ControlValueAccessor {
constructor(
private formBuilder: FormBuilder,
private i18nService: I18nService,
private cipherFormUriService: CipherFormUriService,
) {
this.uriForm.valueChanges.pipe(takeUntilDestroyed()).subscribe((value) => {
this.cipherFormUriService.setUri(value.uri ?? "");
this.onChange(value);
});

Expand Down
Loading