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

Fix request namespace list permissions check if a list rule is not the first match #7282

Merged

Conversation

jakolehm
Copy link
Contributor

@jakolehm jakolehm commented Mar 6, 2023

Smaller version of #7280 .

Signed-off-by: Jari Kolehmainen <jari.kolehmainen@gmail.com>
@jakolehm jakolehm added bug Something isn't working blocker area/rbac Something related to Kube RBAC permissions labels Mar 6, 2023
@jakolehm jakolehm added this to the 6.4.2 milestone Mar 6, 2023
Signed-off-by: Jari Kolehmainen <jari.kolehmainen@gmail.com>
@jakolehm jakolehm marked this pull request as ready for review March 6, 2023 07:01
@jakolehm jakolehm requested a review from a team as a code owner March 6, 2023 07:01
@jakolehm jakolehm requested review from Iku-turso, jim-docker and jansav and removed request for a team March 6, 2023 07:01
Comment on lines +30 to +38
new Promise((resolve) => resolve({
body: {
status: {
incomplete: true,
resourceRules: [],
nonResourceRules: [],
},
},
})),
Copy link
Contributor

Choose a reason for hiding this comment

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

Using asyncFn for stuff like this would remove a bit of duplication :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe give an example and I'll refactor? :)

Copy link
Contributor

Choose a reason for hiding this comment

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

An example on adding asyncFn to existing code structure. Note that the different parts of code here can be spread in different beforeEach to remove said duplication.

const createSelfSubjectRulesReviewMock = asyncFn();

const proxyConfigStub = createProxyConfigStub(createSelfSubjectRulesReviewMock);

const requestPermissions = requestNamespaceListPermissions(proxyConfigStub);

const permissionCheckPromise = requestPermissions("some-namespace");

// Note: in different test, this could be `.reject(...)`.
createSelfSubjectRulesReviewMock.resolve({
  body: {
    status: {
      incomplete: true,
      resourceRules: [],
      nonResourceRules: [],
    },
  },
});

const permissionCheck = await permissionCheckPromise;

expect(permissionCheck({
  apiName: "pods",
  group: "",
  kind: "Pod",
  namespaced: true,
})).toBeTruthy();

Signed-off-by: Jari Kolehmainen <jari.kolehmainen@gmail.com>
import type { RequestNamespaceListPermissionsFor } from "./request-namespace-list-permissions.injectable";
import requestNamespaceListPermissionsForInjectable from "./request-namespace-list-permissions.injectable";

const createFakeProxyConfig = (statusResponse: Promise<{ body: { status: V1SubjectRulesReviewStatus }}>) => ({
Copy link
Contributor

Choose a reason for hiding this comment

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

To consolidate communication in future, this is a stub instead of fake :)

A fake is a trusted simulation of something with non-trivial implementation. A stub is a trivial substitution of value.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed.

Signed-off-by: Jari Kolehmainen <jari.kolehmainen@gmail.com>
@jakolehm jakolehm changed the title Fix request namespace list permissions Fix request namespace list permissions check if a list rule is not the first match Mar 6, 2023
})),
) as any);

const permissionCheck = await requestPermissions("fake-namespace");
Copy link
Contributor

Choose a reason for hiding this comment

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

A string value that does not affect test passing or failing should be "irrelevant" or "irrelevant-namespace".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed.


describe("when api returns incomplete data", () => {
it("returns truthy function", async () => {
const requestPermissions = requestNamespaceListPermissions(createFakeProxyConfig(
Copy link
Contributor

Choose a reason for hiding this comment

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

If injection of dependencies were done using injectables instead of configuring curried functions manually, we could get rid of many steps of test initialisation here, namely everything but override of createSelfSubjectRulesReviewInjectable, (for which that would asyncFn fit nicely).

Copy link
Contributor

@Iku-turso Iku-turso Mar 6, 2023

Choose a reason for hiding this comment

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

Example:

const createSelfSubjectRulesReviewMock = asyncFn();

di.override(createSelfSubjectRulesReviewInjectable, () => createSelfSubjectRulesReviewMock);

const requestNamespaceListPermissions = di.inject(requestNamespaceListPermissionsInjectable);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree that the implementation could be simplified a lot but I'm not comfortable doing that in this PR.

Signed-off-by: Jari Kolehmainen <jari.kolehmainen@gmail.com>
@jakolehm jakolehm merged commit 6789643 into release/v6.4 Mar 6, 2023
@jakolehm jakolehm deleted the fix-requestNamespaceListPermissionsForInjectable branch March 6, 2023 08:46
@jweak jweak mentioned this pull request Mar 6, 2023
jweak added a commit that referenced this pull request Mar 6, 2023
* Fix request namespace list permissions check if a list rule is not the first match (#7282)

Signed-off-by: Jari Kolehmainen <jari.kolehmainen@gmail.com>

* Fix cluster metadata detectors (#7255)

Signed-off-by: Juho Heikka <juho.heikka@gmail.com>

* Fix extension API not having all the correct types (#7263)

Signed-off-by: Sebastian Malton <sebastian@malton.name>

* Update release guide and fix release script (#7276)

Signed-off-by: Sebastian Malton <sebastian@malton.name>

* Release 6.4.2

Signed-off-by: Juho Heikka <juho.heikka@gmail.com>

---------

Signed-off-by: Jari Kolehmainen <jari.kolehmainen@gmail.com>
Signed-off-by: Juho Heikka <juho.heikka@gmail.com>
Signed-off-by: Sebastian Malton <sebastian@malton.name>
Co-authored-by: Sebastian Malton <sebastian@malton.name>
jakolehm added a commit that referenced this pull request Mar 6, 2023
…e first match (#7282)

* fix requestNamespaceListPermissionsForInjectable

Signed-off-by: Jari Kolehmainen <jari.kolehmainen@gmail.com>

* more tests

Signed-off-by: Jari Kolehmainen <jari.kolehmainen@gmail.com>

* fix test descriptions

Signed-off-by: Jari Kolehmainen <jari.kolehmainen@gmail.com>

* fake -> stub

Signed-off-by: Jari Kolehmainen <jari.kolehmainen@gmail.com>

* fake-namespace -> irrelevant-namespace

Signed-off-by: Jari Kolehmainen <jari.kolehmainen@gmail.com>

---------

Signed-off-by: Jari Kolehmainen <jari.kolehmainen@gmail.com>
@jakolehm jakolehm mentioned this pull request Mar 6, 2023
Nokel81 pushed a commit that referenced this pull request Mar 6, 2023
* fix requestNamespaceListPermissionsForInjectable



* more tests



* fix test descriptions



* fake -> stub



* fake-namespace -> irrelevant-namespace



---------

Signed-off-by: Jari Kolehmainen <jari.kolehmainen@gmail.com>
@Nokel81 Nokel81 mentioned this pull request Mar 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/rbac Something related to Kube RBAC permissions blocker bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants