Skip to content
This repository was archived by the owner on Mar 1, 2024. It is now read-only.
Closed
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
1 change: 1 addition & 0 deletions frontend/integration-tests/protractor.conf.ts
Original file line number Diff line number Diff line change
Expand Up @@ -195,6 +195,7 @@ export const config: Config = {
'tests/kubevirt/node.maintenance.scenario.ts',
'tests/kubevirt/vm.nic.binding.scenario.ts',
'tests/kubevirt/template.actions.scenario.ts',
'tests/kubevirt/non-admin.scenario.ts',
],
kubevirtWindows: [
'tests/kubevirt/kubevirt.login.scenario.ts',
Expand Down
Original file line number Diff line number Diff line change
@@ -1,14 +1,19 @@
import { browser } from 'protractor';
import { appHost } from '../../protractor.conf';
import { execSync } from 'child_process';
import { logIn } from './utils/utils';
import { KUBEADMIN_IDP, KUBEADMIN_USERNAME, BRIDGE_KUBEADMIN_PASSWORD } from './utils/consts';
import * as loginView from '../../views/login.view';


describe('Authentication', () => {
it('Web console logs in.', async() => {
beforeAll(async() => {
await browser.get(appHost);
if (process.env.BRIDGE_BASE_ADDRESS !== undefined) {
await logIn();
execSync(`oc login -u ${process.env.BRIDGE_AUTH_USERNAME} -p ${process.env.BRIDGE_AUTH_PASSWORD} --config=${process.env.KUBECONFIG}`);
}
await browser.sleep(3000); // Wait long enough for the login redirect to complete

Choose a reason for hiding this comment

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

Could we rather wait for some element to appear?

Copy link
Author

Choose a reason for hiding this comment

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

I copied this from login.scenario.ts. It seems no necessary to sleep here as there are await in loginView.login.

});

it('Web console logs in.', async() => {
await loginView.login(KUBEADMIN_IDP, KUBEADMIN_USERNAME, BRIDGE_KUBEADMIN_PASSWORD);
expect(loginView.userDropdown.getText()).toContain(KUBEADMIN_IDP);
execSync(`oc login -u ${KUBEADMIN_USERNAME} -p ${BRIDGE_KUBEADMIN_PASSWORD}`);
});
});
137 changes: 137 additions & 0 deletions frontend/integration-tests/tests/kubevirt/non-admin.scenario.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,137 @@
/* eslint-disable no-undef */
import { $, $$, browser, ExpectedConditions as until } from 'protractor';
import { appHost, testName } from '../../protractor.conf';
import { nonAdminSecret, nonAdminProvider } from './utils/mocks';
import { createResources } from './utils/utils';
import { KUBEADMIN_IDP, KUBEADMIN_USERNAME, BRIDGE_KUBEADMIN_PASSWORD, BRIDGE_HTPASSWD_IDP, BRIDGE_HTPASSWD_USERNAME, BRIDGE_HTPASSWD_PASSWORD } from './utils/consts';
import * as loginView from '../../views/login.view';
import * as crudView from '../../views/crud.view';
import Wizard from './models/wizard';
import Yaml from './models/yaml';
import { logout } from '../../views/kubevirt/login.view';
import { execSync } from 'child_process';

describe('Test nonadmin user behaviour', () => {
const nonAdminNS = `${testName}-nonadmin`;
const userRole = ['view', 'edit'];

const verifyPermissions = async function(ns: string, perm: string) {
const wizard = new Wizard();
const yaml = new Yaml();

await browser.get(`${appHost}/k8s/ns/${ns}/virtualmachines`);
if (perm === 'noAccess') {
await browser.wait(until.presenceOf(crudView.errorMessage));
expect(crudView.errorMessage.getText()).toContain('cannot list resource');

// TODO: check it can't use vm dialog once BZ1728523 is fixed.

Choose a reason for hiding this comment

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

Wouldn't it be better if different permissions were tested in separate specs?
This verifyPermissions method seems too big and has no versatility. If there is a bug in some of the permissions, there is no way to specifically skip it.
This way we have passing something that should fail. It's better to skip a failing test with xit, as it's visible in test report.

Copy link
Author

@gouyang gouyang Aug 7, 2019

Choose a reason for hiding this comment

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

Separate specs means different describe, right?
I can change it like:

describe('Test nonadmin user UI behavior', () => {
describe('Test nonadmin user with roleBinding view UI behavior', () => {
describe('Test nonadmin user with roleBinding edit UI behavior', () => {
describe('Test nonadmin user with clusterRoleBinding view UI behavior', () => {
describe('Test nonadmin user with clusterRoleBinding edit UI behavior', () => {

And it does not need to change verifyPermissions.

Copy link

Choose a reason for hiding this comment

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

No, spec is it block

Copy link
Author

Choose a reason for hiding this comment

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

Okay, I will make changes to the PR in console.

}
if (perm === 'view') {
await crudView.isLoaded(); // no error indicates it can view resource.
// TODO: check it can't use vm dialog once BZ1728523 is fixed.
}
if (perm === 'edit') {
await crudView.isLoaded();

Choose a reason for hiding this comment

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

Use Wizard instance for this:

const wizard = new Wizard();
await wizard.openWizard();
await wizard.closeWizard();

I believe there are similar methods in the Yaml class

Copy link
Author

Choose a reason for hiding this comment

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

Updated in openshift#2265

await wizard.openWizard();
await wizard.close();

await yaml.openYamlPage();
await yaml.cancelCreateVM();
}

await browser.get(`${appHost}/k8s/ns/${ns}/vmtemplates`);
if (perm === 'noAccess') {
await browser.wait(until.presenceOf(crudView.errorMessage));
expect(crudView.errorMessage.getText()).toContain('cannot list resource');
}
if (perm === 'view') {
await crudView.isLoaded(); // no error indicates it can view resource.
// TODO: check it can't use vm dialog once BZ1728523 is fixed.
}
if (perm === 'edit') {
await crudView.isLoaded();
await wizard.openWizard();
await wizard.close();
}

await browser.get(`${appHost}/overview/ns/${ns}/`);

Choose a reason for hiding this comment

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

this block would look better extracted into a function

if (perm === 'noAccess') {
await browser.wait(until.presenceOf(crudView.errorMessage));
expect(crudView.errorMessage.getText()).toContain('cannot list resource');
}
if (perm === 'view') {
await crudView.isLoaded(); // no error indicates it can view resource.
// TODO: check it can't use vm dialog once BZ1728523 is fixed.
}
if (perm === 'edit') {
await crudView.isLoaded();
}
};

const verifyPermissionWithRoleBinding = async function(ns: string, roleBinding: string, perm: string) {
execSync(`oc adm policy add-${roleBinding}-to-user ${perm} ${BRIDGE_HTPASSWD_USERNAME} -n ${ns}`);
await verifyPermissions(perm, ns);
execSync(`oc adm policy remove-${roleBinding}-from-user ${perm} ${BRIDGE_HTPASSWD_USERNAME} -n ${ns}`);
};

beforeAll(async() => {
try {
execSync('oc get -o yaml -n openshift-config secret test');
} catch (error) {
createResources([nonAdminSecret, nonAdminProvider]);
}
});

afterAll(async() => {
await logout();

Choose a reason for hiding this comment

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

When running with bridge, there is no logout button, it should be enough to just do oc login with different user and reload page.

Copy link
Author

Choose a reason for hiding this comment

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

oc login with different user and reload page does not work, it's not logging in a different user on UI.

await browser.wait(until.presenceOf($('.login-pf')));
await loginView.login(KUBEADMIN_IDP, KUBEADMIN_USERNAME, BRIDGE_KUBEADMIN_PASSWORD);
});

it('Login with nonadmin', async() => {
await logout();
// wait for logout complete
await browser.sleep(10000);

Choose a reason for hiding this comment

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

Here we should rather wait for login screen with kubeadmin/httpass provider selection

Copy link
Author

Choose a reason for hiding this comment

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

It can use that once 1721423 isn't blocking it.

// TODO: remove the refill appHost after bug 1721423 is fixed.
await browser.get(appHost);
await browser.wait(until.presenceOf($('.login-pf')));
await loginView.login(BRIDGE_HTPASSWD_IDP, BRIDGE_HTPASSWD_USERNAME, BRIDGE_HTPASSWD_PASSWORD);
//expect(loginView.userDropdown.getText()).toContain(BRIDGE_HTPASSWD_IDP);
});

it(`Creates test namespace ${nonAdminNS} for nonAdmin tests`, async() => {
const resource = browser.params.openshift === 'true' ? 'projects' : 'namespaces';
await browser.get(`${appHost}/k8s/cluster/${resource}`);
await crudView.isLoaded();
const exists = await crudView.rowForName(nonAdminNS).isPresent();
if (!exists) {
await crudView.createYAMLButton.click();
await browser.sleep(3000);
await browser.wait(until.presenceOf($('.modal-body__field')));
await $$('.modal-body__field').get(0).$('input').sendKeys(nonAdminNS);
await $$('.modal-body__field').get(1).$('input').sendKeys(`test-name=${nonAdminNS}`);
await $('.modal-content').$('#confirm-action').click();
}
});

it('Nonadmin can use vm dialog in its own namespace but not other namespace', async() => {
await verifyPermissions(nonAdminNS, 'edit');
await verifyPermissions(testName, 'noAccess');
});

userRole.forEach(role => {
it(`Nonadmin with RoleBinding ${role} can ${role} vm objetcs in the binding NS but not other NS`, async() => {
execSync(`oc adm policy add-role-to-user ${role} ${BRIDGE_HTPASSWD_USERNAME} -n ${testName}`);
await verifyPermissions(testName, role);
await verifyPermissions('default', 'noAccess');
execSync(`oc adm policy remove-role-from-user ${role} ${BRIDGE_HTPASSWD_USERNAME} -n ${testName}`);
});
});

userRole.forEach((role) => {
it(`Nonadmin with ClusterRoleBinding ${role} can ${role} vm objetcs in the cluster`, async() => {
await verifyPermissionWithRoleBinding(testName, 'cluster-role', role);
await verifyPermissionWithRoleBinding('default', 'cluster-role', role);
});
});
});
10 changes: 10 additions & 0 deletions frontend/integration-tests/tests/kubevirt/utils/consts.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,16 @@
export const DASHES = '---';
export const STORAGE_CLASS = process.env.STORAGE_CLASS;

export const KUBEADMIN_IDP = 'kube:admin';
export const KUBEADMIN_USERNAME = 'kubeadmin';
export const {
BRIDGE_HTPASSWD_IDP = 'test',
BRIDGE_HTPASSWD_USERNAME = 'test',
BRIDGE_HTPASSWD_PASSWORD = 'test',
BRIDGE_KUBEADMIN_PASSWORD,
KUBECONFIG,
} = process.env;

// TIMEOUTS
const SEC = 1000;
export const CLONE_VM_TIMEOUT = 300 * SEC;
Expand Down
35 changes: 35 additions & 0 deletions frontend/integration-tests/tests/kubevirt/utils/mocks.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,41 @@ import { STORAGE_CLASS } from './consts';
import { getRandomMacAddress } from './utils';


export const nonAdminSecret = {
apiVersion: 'v1',
kind: 'Secret',
metadata: {
name: 'test',
namespace: 'openshift-config',
},
type: 'Opaque',
data: {
htpasswd: 'dGVzdDokMnkkMDUkeENGNUZMTGEvamp5ZmJpLm1mVlBnLjFQVmNGMFRucG4vNmF0RzB1bExUOEN2TnhCZjE2MG0K',
},
};

export const nonAdminProvider = {
apiVersion: 'config.openshift.io/v1',
kind: 'OAuth',
metadata: {
name: 'cluster',
},
spec: {
identityProviders: [
{
name: 'test',
mappingMethod: 'claim',
type: 'HTPasswd',
htpasswd: {
fileData: {
name: 'test',
},
},
},
],
},
};

export const multusNad = {
apiVersion: 'k8s.cni.cncf.io/v1',
kind: 'NetworkAttachmentDefinition',
Expand Down
11 changes: 1 addition & 10 deletions frontend/integration-tests/tests/kubevirt/utils/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,6 @@ import { execSync } from 'child_process';
import { $, by, ElementFinder, browser, ExpectedConditions as until } from 'protractor';

import { config } from '../../../protractor.conf';
import { nameInput as loginNameInput, passwordInput as loginPasswordInput, submitButton as loginSubmitButton } from '../../../views/login.view';
import { PAGE_LOAD_TIMEOUT } from './consts';


export function removeLeakedResources(leakedResources: Set<string>) {
Expand Down Expand Up @@ -37,7 +35,7 @@ export function removeLeakableResource(leakedResources: Set<string>, resource) {
}

export function createResource(resource) {
execSync(`echo '${JSON.stringify(resource)}' | kubectl create -f -`);
execSync(`echo '${JSON.stringify(resource)}' | kubectl apply -f -`);
Copy link

Choose a reason for hiding this comment

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

Is this a good idea to mix declarative and imperative approach in creating resources? Wouldn't it be safer to have a new method, i.e applyResource?

Copy link
Author

Choose a reason for hiding this comment

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

Well, the purpose of this change is avoiding to create a new method. Of course it can has another method or maybe just use execSync.

And why do we need createResource for one line code?

}

export function createResources(resources) {
Expand Down Expand Up @@ -110,13 +108,6 @@ export async function getInputValue(elem: ElementFinder) {
return elem.getAttribute('value');
}

export async function logIn() {
await fillInput(loginNameInput, process.env.BRIDGE_AUTH_USERNAME);
await fillInput(loginPasswordInput, process.env.BRIDGE_AUTH_PASSWORD);
await click(loginSubmitButton);
await browser.wait(until.visibilityOf($('img.pf-c-brand')), PAGE_LOAD_TIMEOUT);
}

export function getRandStr(length: number) {
return Math.random().toString(36).replace(/[.]/g, '').substr(1, length); // First char is always 0
}
Expand Down
13 changes: 13 additions & 0 deletions frontend/integration-tests/views/kubevirt/login.view.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
/* eslint-disable no-undef, no-unused-vars */

import { $, browser, element, by, ExpectedConditions as until } from 'protractor';

export const logOutLink = element(by.linkText('Log out'));
export const userDropdown = $('[data-test=user-dropdown] .pf-c-dropdown__toggle');

export const logout = async() => {
await browser.wait(until.presenceOf(userDropdown));
await userDropdown.click();
await browser.wait(until.presenceOf(logOutLink));
await logOutLink.click();
};
7 changes: 1 addition & 6 deletions frontend/integration-tests/views/login.view.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
/* eslint-disable no-undef, no-unused-vars */

import { $, browser, ExpectedConditions as until, by, element } from 'protractor';
import { appHost } from '../protractor.conf';

export const nameInput = $('#inputUsername');
export const passwordInput = $('#inputPassword');
Expand All @@ -11,15 +10,11 @@ export const userDropdown = $('[data-test=user-dropdown] .pf-c-dropdown__toggle'

export const selectProvider = async(provider: string) => {
const idpLink = element(by.cssContainingText('.idp', provider));
while (!(await idpLink.isPresent())) {
await browser.get(appHost);
await browser.sleep(3000);
}
await idpLink.click();
};

export const login = async(providerName: string, username: string, password: string) => {
if (providerName) {
if (await $('a.idp').isPresent()) {
Copy link

Choose a reason for hiding this comment

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

Note that this change probably should not be necessary in openshift/console

Copy link
Author

Choose a reason for hiding this comment

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

Without this change, it can't run test with a cluster without idp.
It's not necessary if the test run with idp always, I know it's not perfect.

I have a PR in console about this: openshift#2016

await selectProvider(providerName);
}
await browser.wait(until.visibilityOf(nameInput));
Expand Down