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

Add a customisation point for widget permissions to preapprove identity request#9910

Closed
maheichyk wants to merge 5 commits into
matrix-org:developfrom
nordeck:widget_permissions_preapproved_identity_customization
Closed

Add a customisation point for widget permissions to preapprove identity request#9910
maheichyk wants to merge 5 commits into
matrix-org:developfrom
nordeck:widget_permissions_preapproved_identity_customization

Conversation

@maheichyk
Copy link
Copy Markdown
Contributor

@maheichyk maheichyk commented Jan 13, 2023

A new customization point for widget permissions to allow preapproval of identity requests for the widgets.

This will allow forks customization and consistent with #5439

Checklist

  • Tests written for new code (and old code if feasible)
  • Linter and other CI checks pass
  • Sign-off given on the changes (see CONTRIBUTING.md)

Here's what your changelog entry will look like:

✨ Features

  • Add a customisation point for widget permissions to preapprove identity request (#9910). Contributed by @maheichyk.

@maheichyk maheichyk requested a review from a team as a code owner January 13, 2023 13:32
@github-actions github-actions Bot added the Z-Community-PR Issue is solved by a community member's PR label Jan 13, 2023
…ustomization

# Conflicts:
#	src/stores/widgets/StopGapWidgetDriver.ts
@maheichyk
Copy link
Copy Markdown
Contributor Author

Signed-off-by: Mikhail Aheichyk mikhail.aheichyk@nordeck.net

Copy link
Copy Markdown
Member

@richvdh richvdh left a comment

Choose a reason for hiding this comment

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

As someone totally unfamiliar with the widgets interface: isn't there some documentation that needs updating?

Comment on lines +46 to +48
async function isIdentityRequestPreapproved(widget: Widget): Promise<boolean> {
return false;
}
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.

as I understand it, this function is never called, and only exists to provide the type of IWidgetPermissionCustomisations.isIdentityRequestPreapproved ? It seems a very round-the-houses way of defining a type.

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.

You are correct, this function is not called and just an example for how the customization can be implemented. During build time it is replaced with another function if desired, so isIdentityRequestPreapproved would really never be used.

As all other customizations are implement this way, e.g. the preapproveCapabilities function above, so I think we should keep the pattern?

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.

are there any other examples of this than preapproveCapabilities?

At the very least, please add a comment to the function saying it is never called and is only an example.

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.

Function is updated with the comment that it is used to provide the type only.

src/customisations contains more similar use cases, for example: 'src/customisations/Security.ts'

Comment thread src/customisations/WidgetPermissions.ts Outdated
}

/**
* Approves the widget for identity token request, if it can be approved.
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.

  • Approves the widget for identity token request, if it can be approved.

I'm afraid I don't understand what this sentence means. It seems very circular?

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.

yes, it was simplified.

Comment thread src/customisations/WidgetPermissions.ts Outdated
@richvdh richvdh added the T-Enhancement New features, changes in functionality, performance boosts, user-facing improvements label Jan 13, 2023
@richvdh
Copy link
Copy Markdown
Member

richvdh commented Jan 13, 2023

Also: note that this is failing the CI quality gate. It will need some tests, please.

@robintown robintown removed their request for review January 16, 2023 03:14
maheichyk and others added 2 commits January 16, 2023 13:08
Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com>
@maheichyk
Copy link
Copy Markdown
Contributor Author

Also: note that this is failing the CI quality gate. It will need some tests, please.

I think it is a bit tricky to test a new code added to StopGapWidgetDriver.askOpenID because this method is not tested currently.

@maheichyk
Copy link
Copy Markdown
Contributor Author

Closing this PR in favor of #10121

@maheichyk maheichyk closed this Mar 7, 2023
@benparsons benparsons added the A-Timesheet-1 Log any time spent on this into the A-Timesheet-1 project label Mar 20, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

A-Timesheet-1 Log any time spent on this into the A-Timesheet-1 project T-Enhancement New features, changes in functionality, performance boosts, user-facing improvements Z-Community-PR Issue is solved by a community member's PR

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants