Skip to content
This repository was archived by the owner on Feb 8, 2024. It is now read-only.

[Connect] Conditionally render Access Request navigation menu #1273

Merged
avatus merged 8 commits into from
Oct 20, 2022
Merged

[Connect] Conditionally render Access Request navigation menu #1273

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
d194789
Conditionally render Access Request navigation item.
avatus Oct 17, 2022
043d4e6
Add protogen files
avatus Oct 17, 2022
f61054a
Remove syncClusterInfo from logout
avatus Oct 17, 2022
5bc9fc0
Remove outdated comment
avatus Oct 18, 2022
3a1d27a
Swap Identity and NavigationMenu positions in TopBar
avatus Oct 19, 2022
d73d80d
Update access request navigation menu conditional statement
avatus Oct 19, 2022
3a878c8
protofiles
avatus Oct 19, 2022
641a0ce
Merge branch 'master' into michaelmyers/connect/add_cluster_feature_d…
avatus Oct 20, 2022
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
28 changes: 28 additions & 0 deletions packages/teleterm/src/services/tshd/v1/cluster_pb.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,12 @@ export class Cluster extends jspb.Message {
setLoggedInUser(value?: LoggedInUser): Cluster;


hasFeatures(): boolean;
clearFeatures(): void;
getFeatures(): Features | undefined;
setFeatures(value?: Features): Cluster;


serializeBinary(): Uint8Array;
toObject(includeInstance?: boolean): Cluster.AsObject;
static toObject(includeInstance: boolean, msg: Cluster): Cluster.AsObject;
Expand All @@ -47,6 +53,7 @@ export namespace Cluster {
connected: boolean,
leaf: boolean,
loggedInUser?: LoggedInUser.AsObject,
features?: Features.AsObject,
}
}

Expand Down Expand Up @@ -233,3 +240,24 @@ export namespace ResourceAccess {
pb_delete: boolean,
}
}

export class Features extends jspb.Message {
getAdvancedAccessWorkflows(): boolean;
setAdvancedAccessWorkflows(value: boolean): Features;


serializeBinary(): Uint8Array;
toObject(includeInstance?: boolean): Features.AsObject;
static toObject(includeInstance: boolean, msg: Features): Features.AsObject;
static extensions: {[key: number]: jspb.ExtensionFieldInfo<jspb.Message>};
static extensionsBinary: {[key: number]: jspb.ExtensionFieldBinaryInfo<jspb.Message>};
static serializeBinaryToWriter(message: Features, writer: jspb.BinaryWriter): void;
static deserializeBinary(bytes: Uint8Array): Features;
static deserializeBinaryFromReader(message: Features, reader: jspb.BinaryReader): Features;
}

export namespace Features {
export type AsObject = {
advancedAccessWorkflows: boolean,
}
}
205 changes: 204 additions & 1 deletion packages/teleterm/src/services/tshd/v1/cluster_pb.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ var global = (function() { return this || window || global || self || Function('

goog.exportSymbol('proto.teleport.terminal.v1.ACL', null, global);
goog.exportSymbol('proto.teleport.terminal.v1.Cluster', null, global);
goog.exportSymbol('proto.teleport.terminal.v1.Features', null, global);
goog.exportSymbol('proto.teleport.terminal.v1.LoggedInUser', null, global);
goog.exportSymbol('proto.teleport.terminal.v1.ResourceAccess', null, global);
/**
Expand Down Expand Up @@ -103,6 +104,27 @@ if (goog.DEBUG && !COMPILED) {
*/
proto.teleport.terminal.v1.ResourceAccess.displayName = 'proto.teleport.terminal.v1.ResourceAccess';
}
/**
* Generated by JsPbCodeGenerator.
* @param {Array=} opt_data Optional initial data array, typically from a
* server response, or constructed directly in Javascript. The array is used
* in place and becomes part of the constructed object. It is not cloned.
* If no data is provided, the constructed object will be empty, but still
* valid.
* @extends {jspb.Message}
* @constructor
*/
proto.teleport.terminal.v1.Features = function(opt_data) {
jspb.Message.initialize(this, opt_data, 0, -1, null, null);
};
goog.inherits(proto.teleport.terminal.v1.Features, jspb.Message);
if (goog.DEBUG && !COMPILED) {
/**
* @public
* @override
*/
proto.teleport.terminal.v1.Features.displayName = 'proto.teleport.terminal.v1.Features';
}



Expand Down Expand Up @@ -140,7 +162,8 @@ proto.teleport.terminal.v1.Cluster.toObject = function(includeInstance, msg) {
proxyHost: jspb.Message.getFieldWithDefault(msg, 3, ""),
connected: jspb.Message.getBooleanFieldWithDefault(msg, 4, false),
leaf: jspb.Message.getBooleanFieldWithDefault(msg, 5, false),
loggedInUser: (f = msg.getLoggedInUser()) && proto.teleport.terminal.v1.LoggedInUser.toObject(includeInstance, f)
loggedInUser: (f = msg.getLoggedInUser()) && proto.teleport.terminal.v1.LoggedInUser.toObject(includeInstance, f),
features: (f = msg.getFeatures()) && proto.teleport.terminal.v1.Features.toObject(includeInstance, f)
};

if (includeInstance) {
Expand Down Expand Up @@ -202,6 +225,11 @@ proto.teleport.terminal.v1.Cluster.deserializeBinaryFromReader = function(msg, r
reader.readMessage(value,proto.teleport.terminal.v1.LoggedInUser.deserializeBinaryFromReader);
msg.setLoggedInUser(value);
break;
case 8:
var value = new proto.teleport.terminal.v1.Features;
reader.readMessage(value,proto.teleport.terminal.v1.Features.deserializeBinaryFromReader);
msg.setFeatures(value);
break;
default:
reader.skipField();
break;
Expand Down Expand Up @@ -274,6 +302,14 @@ proto.teleport.terminal.v1.Cluster.serializeBinaryToWriter = function(message, w
proto.teleport.terminal.v1.LoggedInUser.serializeBinaryToWriter
);
}
f = message.getFeatures();
if (f != null) {
writer.writeMessage(
8,
f,
proto.teleport.terminal.v1.Features.serializeBinaryToWriter
);
}
};


Expand Down Expand Up @@ -404,6 +440,43 @@ proto.teleport.terminal.v1.Cluster.prototype.hasLoggedInUser = function() {
};


/**
* optional Features features = 8;
* @return {?proto.teleport.terminal.v1.Features}
*/
proto.teleport.terminal.v1.Cluster.prototype.getFeatures = function() {
return /** @type{?proto.teleport.terminal.v1.Features} */ (
jspb.Message.getWrapperField(this, proto.teleport.terminal.v1.Features, 8));
};


/**
* @param {?proto.teleport.terminal.v1.Features|undefined} value
* @return {!proto.teleport.terminal.v1.Cluster} returns this
*/
proto.teleport.terminal.v1.Cluster.prototype.setFeatures = function(value) {
return jspb.Message.setWrapperField(this, 8, value);
};


/**
* Clears the message field making it undefined.
* @return {!proto.teleport.terminal.v1.Cluster} returns this
*/
proto.teleport.terminal.v1.Cluster.prototype.clearFeatures = function() {
return this.setFeatures(undefined);
};


/**
* Returns whether this field is set.
* @return {boolean}
*/
proto.teleport.terminal.v1.Cluster.prototype.hasFeatures = function() {
return jspb.Message.getField(this, 8) != null;
};



/**
* List of repeated fields within this message type.
Expand Down Expand Up @@ -1701,4 +1774,134 @@ proto.teleport.terminal.v1.ResourceAccess.prototype.setDelete = function(value)
};





if (jspb.Message.GENERATE_TO_OBJECT) {
/**
* Creates an object representation of this proto.
* Field names that are reserved in JavaScript and will be renamed to pb_name.
* Optional fields that are not set will be set to undefined.
* To access a reserved field use, foo.pb_<name>, eg, foo.pb_default.
* For the list of reserved names please see:
* net/proto2/compiler/js/internal/generator.cc#kKeyword.
* @param {boolean=} opt_includeInstance Deprecated. whether to include the
* JSPB instance for transitional soy proto support:
* http://goto/soy-param-migration
* @return {!Object}
*/
proto.teleport.terminal.v1.Features.prototype.toObject = function(opt_includeInstance) {
return proto.teleport.terminal.v1.Features.toObject(opt_includeInstance, this);
};


/**
* Static version of the {@see toObject} method.
* @param {boolean|undefined} includeInstance Deprecated. Whether to include
* the JSPB instance for transitional soy proto support:
* http://goto/soy-param-migration
* @param {!proto.teleport.terminal.v1.Features} msg The msg instance to transform.
* @return {!Object}
* @suppress {unusedLocalVariables} f is only used for nested messages
*/
proto.teleport.terminal.v1.Features.toObject = function(includeInstance, msg) {
var f, obj = {
advancedAccessWorkflows: jspb.Message.getBooleanFieldWithDefault(msg, 1, false)
};

if (includeInstance) {
obj.$jspbMessageInstance = msg;
}
return obj;
};
}


/**
* Deserializes binary data (in protobuf wire format).
* @param {jspb.ByteSource} bytes The bytes to deserialize.
* @return {!proto.teleport.terminal.v1.Features}
*/
proto.teleport.terminal.v1.Features.deserializeBinary = function(bytes) {
var reader = new jspb.BinaryReader(bytes);
var msg = new proto.teleport.terminal.v1.Features;
return proto.teleport.terminal.v1.Features.deserializeBinaryFromReader(msg, reader);
};


/**
* Deserializes binary data (in protobuf wire format) from the
* given reader into the given message object.
* @param {!proto.teleport.terminal.v1.Features} msg The message object to deserialize into.
* @param {!jspb.BinaryReader} reader The BinaryReader to use.
* @return {!proto.teleport.terminal.v1.Features}
*/
proto.teleport.terminal.v1.Features.deserializeBinaryFromReader = function(msg, reader) {
while (reader.nextField()) {
if (reader.isEndGroup()) {
break;
}
var field = reader.getFieldNumber();
switch (field) {
case 1:
var value = /** @type {boolean} */ (reader.readBool());
msg.setAdvancedAccessWorkflows(value);
break;
default:
reader.skipField();
break;
}
}
return msg;
};


/**
* Serializes the message to binary data (in protobuf wire format).
* @return {!Uint8Array}
*/
proto.teleport.terminal.v1.Features.prototype.serializeBinary = function() {
var writer = new jspb.BinaryWriter();
proto.teleport.terminal.v1.Features.serializeBinaryToWriter(this, writer);
return writer.getResultBuffer();
};


/**
* Serializes the given message to binary data (in protobuf wire
* format), writing to the given BinaryWriter.
* @param {!proto.teleport.terminal.v1.Features} message
* @param {!jspb.BinaryWriter} writer
* @suppress {unusedLocalVariables} f is only used for nested messages
*/
proto.teleport.terminal.v1.Features.serializeBinaryToWriter = function(message, writer) {
var f = undefined;
f = message.getAdvancedAccessWorkflows();
if (f) {
writer.writeBool(
1,
f
);
}
};


/**
* optional bool advanced_access_workflows = 1;
* @return {boolean}
*/
proto.teleport.terminal.v1.Features.prototype.getAdvancedAccessWorkflows = function() {
return /** @type {boolean} */ (jspb.Message.getBooleanFieldWithDefault(this, 1, false));
};


/**
* @param {boolean} value
* @return {!proto.teleport.terminal.v1.Features} returns this
*/
proto.teleport.terminal.v1.Features.prototype.setAdvancedAccessWorkflows = function(value) {
return jspb.Message.setProto3BooleanField(this, 1, value);
};


goog.object.extend(exports, proto.teleport.terminal.v1);
5 changes: 4 additions & 1 deletion packages/teleterm/src/ui/TopBar/NavigationMenu/NavigationMenu.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,10 @@ export function NavigationMenu() {
const [isPopoverOpened, setIsPopoverOpened] = useState(false);
const selectorRef = useRef<HTMLButtonElement>();

if (!activeRootCluster) {
if (

@gzdunek gzdunek Oct 19, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Maybe something like this? It seems easier to understand

const shouldShowMenu = !!activeRootCluster?.features.advancedAccessWorkflows;
if (!shouldShowMenu) {
  return null;
}

@ravicious ravicious Oct 19, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Would it make sense to put the access requests menu to the left of profile selector? Otherwise the profile selector switches position when switching between a cluster that has access requests and one that doesn't.

!activeRootCluster ||
!activeRootCluster.features.advancedAccessWorkflows

@ravicious ravicious Oct 19, 2022 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'd assume that when we get a response from endpoint that doesn't actually populate features, such as ListRootClusters, then features is going to be undefined. But I'm surprised to see it's actually the whole struct with fields simply set to false. Interesting, I didn't know that.

@avatus avatus Oct 19, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I think this is bad design. I've updated the backend to only return the features on the relevant rpcs (such as GetCluster). Although I think in the future we may want to rename ListRootClusters to something like ListRootClustersInProfile since that more clearly describes what it does

@ravicious ravicious Oct 20, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Although I think in the future we may want to rename ListRootClusters to something like ListRootClustersInProfile since that more clearly describes what it does

Another idea for the future would be to utilize the render view pattern that Alan mentioned. But yeah, we're free to completely change the RPCs around getting root cluster details, there's nothing holding us back. They work the way they work right now because we've never had to do more than just read stuff from the disk.

) {
return <></>;
}

Expand Down
3 changes: 0 additions & 3 deletions packages/teleterm/src/ui/services/clusters/clustersService.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,6 @@ export class ClustersService extends ImmutableStore<ClustersServiceState> {
async logout(clusterUri: string) {
// TODO(gzdunek): logout and removeCluster should be combined into a single acton in tshd
await this.client.logout(clusterUri);
await this.syncClusterInfo(clusterUri);

@avatus avatus Oct 17, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Why would we need to try and syncClusterInfo after logging out? I removed it because now that GetCluster has a addMetadataToRetryableError, logging out would fail here during the sync. "no SSH auth methods found. are you logged in?". we just logged out the line above, of course we aren't logged in! I think this line might have just been an error from the start (git blame says back in febuary) and it wasn't never noticed since it was essentially ignoring the error.

@ravicious ravicious Oct 19, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Just for posterity, I'm linking my comment from another PR gravitational/teleport#17497 (comment) which explains that this is probably a remnant of times when logging out and removing a cluster from the list were two separate actions.

this.removeResources(clusterUri);
await this.removeCluster(clusterUri);
await this.removeClusterKubeConfigs(clusterUri);
Expand Down Expand Up @@ -705,8 +704,6 @@ export class ClustersService extends ImmutableStore<ClustersServiceState> {
return useStore(this).state;
}

// Note: client.getCluster ultimately reads data from the disk, so syncClusterInfo will not fail
// with a retryable error in case the certs have expired.
private async syncClusterInfo(clusterUri: string) {
const cluster = await this.client.getCluster(clusterUri);
const assumedRequests = cluster.loggedInUser
Expand Down