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

[Key Vault Admin] Convenience layer - KeyVaultAccessControlClient #10815

Merged
Merged
Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
37 commits
Select commit Hold shift + click to select a range
7c3cfc5
constructor and some of the surrounding things
sadasant Aug 24, 2020
28b4eb6
createRoleAssignment
sadasant Aug 24, 2020
eb9cbe0
createRoleAssignment documentation
sadasant Aug 24, 2020
a5524f5
deleteRoleAssignment
sadasant Aug 24, 2020
167935d
getRoleAssignment
sadasant Aug 24, 2020
44e9eb3
listRoleAssignments
sadasant Aug 24, 2020
3790bd1
listRoleDefinitions
sadasant Aug 24, 2020
2188a87
api-extractor
sadasant Aug 24, 2020
60f4bd3
formatting
sadasant Aug 24, 2020
ac414c9
deleted the filter option on the list operations
sadasant Aug 24, 2020
9e5505a
fixing the build
sadasant Aug 24, 2020
57fd102
lint fixes
sadasant Aug 24, 2020
0ce383e
forgot to api extract the last commit
sadasant Aug 24, 2020
7f179ef
bad reference to a parameter
sadasant Aug 25, 2020
8919a93
Resolves https://github.com/Azure/azure-sdk-for-js/pull/10815#discuss…
sadasant Aug 25, 2020
ee1d71e
Addressing: https://github.com/Azure/azure-sdk-for-js/pull/10815#disc…
sadasant Aug 25, 2020
b7d3bf5
Feedback fixes
sadasant Aug 25, 2020
26e9583
role to roleScope, and RoleDefinitionPermission to KeyVaultPermission
sadasant Aug 26, 2020
a10a592
KeyVaultAccessControlClient documentation and removing lint from the …
sadasant Aug 26, 2020
09148fd
cleaned up bad reference to KeyClient
sadasant Aug 26, 2020
1b0f642
Addressing https://github.com/Azure/azure-sdk-for-js/pull/10815#discu…
sadasant Aug 27, 2020
4513d72
Merge remote-tracking branch 'Azure/master' into keyvault-admin/10799…
sadasant Aug 27, 2020
6ab4ae1
fixed bad references
sadasant Aug 27, 2020
7c845ca
Update sdk/keyvault/keyvault-admin/src/accessControlClient.ts
sadasant Sep 1, 2020
9030441
Apply suggestions from code review
sadasant Sep 2, 2020
18d7911
Fixes after updating from remote
sadasant Sep 2, 2020
74b7bfc
Addressing https://github.com/Azure/azure-sdk-for-js/pull/10815/files…
sadasant Sep 2, 2020
ca12be4
tsdoc cleanup
sadasant Sep 2, 2020
e306ff4
linting
sadasant Sep 2, 2020
d61f0b6
Merge remote-tracking branch 'Azure/master' into keyvault-admin/10799…
sadasant Sep 2, 2020
64a5d1d
fixes after updating the generated files
sadasant Sep 2, 2020
1386365
enforcing most of the properties, after discussing with .Net
sadasant Sep 2, 2020
590a835
Addressing https://github.com/Azure/azure-sdk-for-js/pull/10815#discu…
sadasant Sep 2, 2020
23431cc
no more operationOptionsToRequestOptionsBase
sadasant Sep 4, 2020
66bcdd4
Key Vault
sadasant Sep 4, 2020
8484177
Merge remote-tracking branch 'Azure/master' into keyvault-admin/10799…
sadasant Sep 4, 2020
24380a7
only 7.2-preview service version
sadasant Sep 4, 2020
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
31 changes: 31 additions & 0 deletions sdk/keyvault/keyvault-admin/api-extractor.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
{
"$schema": "https://developer.microsoft.com/json-schemas/api-extractor/v7/api-extractor.schema.json",
"mainEntryPointFilePath": "types/keyvault-admin/src/index.d.ts",
"docModel": {
"enabled": true
},
"apiReport": {
"enabled": true,
"reportFolder": "./review"
},
"dtsRollup": {
"enabled": true,
"untrimmedFilePath": "",
"publicTrimmedFilePath": "./types/keyvault-admin.d.ts"
},
"messages": {
"tsdocMessageReporting": {
"default": {
"logLevel": "none"
}
},
"extractorMessageReporting": {
"ae-missing-release-tag": {
"logLevel": "none"
},
"ae-unresolved-link": {
"logLevel": "none"
}
}
}
}
12 changes: 8 additions & 4 deletions sdk/keyvault/keyvault-admin/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@
"url": "https://github.com/Azure/azure-sdk-for-js/issues"
},
"main": "./dist/index.js",
"module": "dist-esm/src/index.js",
"module": "dist-esm/keyvault-admin/src/index.js",
"types": "./types/keyvault-admin.d.ts",
"engine": {
"node": ">=8.0.0"
Expand All @@ -29,10 +29,11 @@
"node": ">=8.0.0"
},
"files": [
"types/",
"types/keyvault-admin.d.ts",
"dist/",
"dist-browser/",
"dist-esm/src",
"dist-esm/keyvault-admin/src",
"dist-esm/keyvault-common/src",
"README.md",
"LICENSE"
],
Expand All @@ -52,7 +53,7 @@
"execute:js-samples": "echo skipped",
"execute:ts-samples": "echo skipped",
"execute:samples": "npm run build:samples && npm run execute:js-samples && npm run execute:ts-samples",
"extract-api": "echo skipped",
"extract-api": "tsc -p . && api-extractor run --local",
"format": "prettier --write --config ../../../.prettierrc.json --ignore-path ../../../.prettierignore \"src/**/*.ts\" \"test/**/*.ts\" \"*.{js,json}\"",
"integration-test:browser": "echo skipped",
"integration-test:node": "echo skipped",
Expand All @@ -73,12 +74,15 @@
"sideEffects": false,
"dependencies": {
"@azure/core-http": "^1.1.6",
"@azure/core-paging": "^1.1.1",
"@azure/core-tracing": "1.0.0-preview.9",
"@azure/logger": "^1.0.0",
"@opentelemetry/api": "^0.10.2",
"tslib": "^2.0.0"
},
"devDependencies": {
"@azure/eslint-plugin-azure-sdk": "^3.0.0",
"@microsoft/api-extractor": "7.7.11",
"@rollup/plugin-commonjs": "11.0.2",
"@rollup/plugin-json": "^4.0.0",
"@rollup/plugin-multi-entry": "^3.0.0",
Expand Down
98 changes: 98 additions & 0 deletions sdk/keyvault/keyvault-admin/review/keyvault-admin.api.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,98 @@
## API Report File for "@azure/keyvault-admin"

> Do not edit this file. It is a report generated by [API Extractor](https://api-extractor.com/).

```ts

import * as coreHttp from '@azure/core-http';
import { PagedAsyncIterableIterator } from '@azure/core-paging';
import { TokenCredential } from '@azure/core-http';

// @public (undocumented)
export class AccessControlClient {
constructor(vaultUrl: string, credential: TokenCredential, pipelineOptions?: AccessControlClientOptions);
createRoleAssignment(scope: RoleAssignmentScope, name: string, options?: CreateRoleAssignmentOptions): Promise<KeyVaultRoleAssignment>;
Copy link
Member

Choose a reason for hiding this comment

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

Per discussion below, the principalId and roleDefinitionId (?) should be parameters. Same goes for any required parameters in other methods that follow.

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 have updated this by this point, to be:

    createRoleAssignment(scope: RoleAssignmentScope, name: string, roleDefinitionId: string, principalId: string, options?: CreateRoleAssignmentOptions): Promise<KeyVaultRoleAssignment>;

Note that I used roleDefinitionId first, then principalId, since I believe that the former one has a smaller scope than the later one, so I'm trying to go from micro to macro, scope-wise. Does this make sense? I can change it, I won't push back, but this is my argument for my current approach.

Wait, what other methods have these as the required parameters?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The only methods that have these properties in .Net are: CreateRoleAssignment, and CreateRoleAssignmentAsync.

sadasant marked this conversation as resolved.
Show resolved Hide resolved
deleteRoleAssignment(scope: RoleAssignmentScope, name: string, options?: DeleteRoleAssignmentOptions): Promise<KeyVaultRoleAssignment>;
getRoleAssignment(scope: RoleAssignmentScope, name: string, options?: DeleteRoleAssignmentOptions): Promise<KeyVaultRoleAssignment>;
listRoleAssignments(scope: RoleAssignmentScope, options?: ListRoleAssignmentsOptions): PagedAsyncIterableIterator<KeyVaultRoleAssignment>;
listRoleDefinitions(scope: RoleAssignmentScope, options?: ListRoleDefinitionsOptions): PagedAsyncIterableIterator<KeyVaultRoleDefinition>;
readonly vaultUrl: string;
}

// @public
export interface AccessControlClientOptions extends coreHttp.PipelineOptions {
serviceVersion?: SUPPORTED_API_VERSIONS;
}

// @public
export interface CreateRoleAssignmentOptions extends coreHttp.OperationOptions {
properties?: RoleAssignmentProperties;
}

// @public
export interface DeleteRoleAssignmentOptions extends coreHttp.OperationOptions {
}

// @public
export interface KeyVaultRoleAssignment {
Copy link
Member

Choose a reason for hiding this comment

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

Just RoleAssignment

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Before I do this change, let's argue about prefixes here: #10815 (comment)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@heaths I'm assuming KeyVaultRoleAssignment is favored after that conversation I mentioned ^

Choose a reason for hiding this comment

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

python has KeyVaultRoleAssignment

readonly id?: string;
readonly name?: string;
properties?: RoleAssignmentPropertiesWithScope;
readonly type?: string;
}

// @public
export interface KeyVaultRoleDefinition {
Copy link
Member

Choose a reason for hiding this comment

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

Just RoleDefinition

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Before I do this change, let's argue about prefixes here: #10815 (comment)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@heaths I'm assuming KeyVaultRoleDefinition is favored after that conversation I mentioned ^

Choose a reason for hiding this comment

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

python has KeyVaultRoleDefinition

assignableScopes?: string[];
description?: string;
readonly id?: string;
readonly name?: string;
permissions?: RoleDefinitionPermission[];
Copy link
Member

Choose a reason for hiding this comment

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

KeyVaultPermission is what we have in .NET, but @christothes I'm seeing a lot of inconsistency in naming. We could probably ignore this for now but will have to figure it all out later. Maybe you and @sadasant can work it out now. The main thing is that models and clients should at least be non-generic terms (like "Definition"), and we probably want to consider we may have generic RBAC client in the future, so the "KeyVault" prefix might be a good idea universally. Curious what @KrzysztofCwalina thinks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good to me! I'll do the changes you suggest for now to align to .Net and then we'll follow up.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should the KeyVault prefix exist on the RoleDefinition type, for example? It seems generic, and since we're going to have other admin clients, it seemed appropriate.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same goes for RoleAssignment, should it be prefixed too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed the KeyVaultPermission part of this conversation here: 26e9583

Copy link
Member

Choose a reason for hiding this comment

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

One more consistency naming issue - java is using RoleScope rather than RoleAssignmentScope. I'm thinking we should snap to that or possibly even KeyVaultRoleScope. Thoughts?

//cc: @vcolin7

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 don't like the assignment part either! but otherwise I'll leave y'all to agree

Copy link
Member

Choose a reason for hiding this comment

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

Yeah I prefer to have the Assignment or Definition removed from the scope class name, since the scope can be used with both RoleAssignment and RoleDefinition. Aside from that I don't mind using either name, although I think using a "KeyVault" prefix everywhere seems a bit overkill if the classes that are in the Key Vault package and have names that are not that widely used (i.e. KeyVaultError instead of Error looks good to me but you could argue that KeyVaultRoleAssignment instead of RoleAssignment is unnecessary).

Copy link
Member

Choose a reason for hiding this comment

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

Reading a bit more of the conversation above I think it's a good idea to use the prefix to for the sake of consistency.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Python here: Azure/azure-sdk-for-python#13372 seems to be using these as well:

KeyVaultPermission
KeyVaultRoleAssignment
KeyVaultRoleAssignmentProperties
KeyVaultRoleAssignmentPropertiesWithScope
KeyVaultRoleDefinition

This should go to Archboard review eventually, but I think we could keep things consistent together first.

roleName?: string;
roleType?: string;
readonly type?: string;
sadasant marked this conversation as resolved.
Show resolved Hide resolved
}

// @public
export const LATEST_API_VERSION = "7.1";

// @public
export interface ListRoleAssignmentsOptions extends coreHttp.OperationOptions {
}

// @public
export interface ListRoleDefinitionsOptions extends coreHttp.OperationOptions {
}

// @public
export interface RoleAssignmentProperties {
principalId?: string;
roleDefinitionId?: string;
}

// @public
export interface RoleAssignmentPropertiesWithScope extends RoleAssignmentProperties {
scope?: RoleAssignmentScope;
}

// @public
export type RoleAssignmentScope = "/" | "/keys";
sadasant marked this conversation as resolved.
Show resolved Hide resolved

// @public
export interface RoleDefinitionPermission {
actions?: string[];
dataActions?: string[];
notActions?: string[];
notDataActions?: string[];
}

// @public
export const SDK_VERSION: string;

// @public
export type SUPPORTED_API_VERSIONS = "7.0" | "7.1" | "7.2-preview";
sadasant marked this conversation as resolved.
Show resolved Hide resolved


// (No @packageDocumentation comment for this package)

```
4 changes: 2 additions & 2 deletions sdk/keyvault/keyvault-admin/rollup.base.config.js
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ export function nodeConfig(test = false) {
const externalNodeBuiltins = [];
const additionalExternals = [];
const baseConfig = {
input: "dist-esm/src/index.js",
input: "dist-esm/keyvault-admin/src/index.js",
external: depNames.concat(externalNodeBuiltins, additionalExternals),
output: {
file: "dist/index.js",
Expand Down Expand Up @@ -83,7 +83,7 @@ export function nodeConfig(test = false) {

export function browserConfig(test = false) {
const baseConfig = {
input: "dist-esm/src/index.js",
input: "dist-esm/keyvault-admin/src/index.js",
output: {
file: "dist-browser/azure-keyvault-admin.js",
banner: banner,
Expand Down
Loading