CRISTAL-381: Make the github backend supported #687

pjeanjean · 2025-02-20T13:28:09Z

Jira URL

https://jira.xwiki.org/browse/CRISTAL-381

Changes

Description

Working authentication
Content display
Content edition
Content deletion
Updated navigation tree to use public API
Updated history to use public API

Clarifications

Login operations for the GitHub API have to be done in the backend. As such, a new backend has been implemented in ./github-authentication. It requires to set a Client ID for a GitHub app that has repo:read and repo:write permissions for the repository the user wants to use. This can be set in ./github-authentication/src/config.ts.

While working on this issue, I noticed that the transformImage was not working properly (i.e., mutation listeners were in fact not triggered on document change). I don't know since when this happens, but I added a simpler transformImagesInElements() function that should have, as an added benefit, less performance impact for this purpose.

Screenshots & Video

Executed Tests

Each new and changed feature was tested manually, and the current test suite was run manually.

Expected merging strategy

Prefers squash: Yes
Backport on branches:
- N/A

* Working authentication * Content display * Content edition * Content deletion * Updated navigation tree to use public API * Updated history to use public API

manuelleduc · 2025-02-20T15:49:59Z

core/authentication/authentication-github/src/GitHubAuthenticationManager.ts

+  private readonly accessTokenCookieKeyPrefix = "accessToken";
+
+  async start(): Promise<void> {
+    const authorizationUrl = new URL("http://localhost:15682/device-login");


Shouldn't this URL be defined in the configuration?

manuelleduc · 2025-02-20T15:50:39Z

core/authentication/authentication-github/src/GitHubAuthenticationManager.ts

+    } = await response.json();
+
+    window.localStorage.setItem(
+      "currentConfigName",


Why are all the string constants defined in fields but not this one?

manuelleduc · 2025-02-21T08:41:52Z

core/authentication/authentication-api/src/authenticationManager.ts

@@ -29,7 +29,7 @@ interface AuthenticationManager {
  /**
   *  Starts the authentication process.
   */
-  start(): void;
+  start(): Promise<void>;


the signature changes, we need to update the since for this method

manuelleduc · 2025-02-21T08:44:39Z

core/authentication/authentication-github/src/GitHubAuthenticationManager.ts

+    let expiresIn: number =
+      parseInt(window.localStorage.getItem(this.localStorageExpiresIn)!) * 1000;
+
+    const intervalId = setInterval(async () => {


This block deserves an explanation, and possible to be isolated in a private method, feels hard to understand without carefully reading the code.

manuelleduc · 2025-02-21T08:45:12Z

core/authentication/authentication-github/src/GitHubAuthenticationManager.ts

+      parseInt(window.localStorage.getItem(this.localStorageExpiresIn)!) * 1000;
+
+    const intervalId = setInterval(async () => {
+      fetch(verificationUrl).then(async (response) => {


await is preferable over then in async methods

manuelleduc · 2025-02-21T08:45:34Z

core/authentication/authentication-github/src/GitHubAuthenticationManager.ts

+            jsonResponse.token_type!,
+            cookiesOptions,
+          );
+          window.location.reload();


is this working on electron?

manuelleduc · 2025-02-21T08:48:51Z

core/authentication/authentication-ui/src/vue/GitHubLoginMenu.vue

+  let expirationSeconds: number = parseInt(
+    window.localStorage.getItem(localStorageExpiresIn)!,
+  );
+  const intervalId = setInterval(() => {


I think this deserves a comment

manuelleduc · 2025-02-21T08:49:13Z

core/authentication/authentication-ui/src/vue/GitHubLoginMenu.vue

+      </x-btn>
+    </template>
+    <template #default>
+      Step 1: Copy the following code


Missing translations

manuelleduc · 2025-02-21T08:52:25Z

core/authentication/authentication-ui/src/vue/UserDetails.vue

@@ -52,8 +52,8 @@ function logout() {
  <template v-if="!error">
    <i18n-t keypath="userDescription" tag="span">
      <a :href="profile">
-        <x-avatar v-if="avatar" :image="avatar" :name="name"></x-avatar>
-        {{ name }}
+        <x-avatar v-if="avatar" :image="avatar" :name="name"></x-avatar


unwanted change?

This was actually wanted, I forgot to mention it. With the previous version, the underline of the link was applied across the avatar image as well, which was quite disturbing.

manuelleduc · 2025-02-21T08:55:41Z

core/history/history-github/src/components/componentsInit.ts

-                    },
-                  }).href,
-                };
+        jsonResponse.forEach(


while not "officially" voted, it seems like using for ... of is preferable over forEach, see https://github.com/github/eslint-plugin-github/blob/main/docs/rules/array-foreach.md

manuelleduc · 2025-02-21T08:56:48Z

core/navigation-tree/navigation-tree-github/src/components/componentsInit.ts

      });
      const jsonResponse = await response.json();
-      jsonResponse.payload.tree.items.forEach(
-        (treeNode: { name: string; path: string; contentType: string }) => {
+      console.log(jsonResponse);


leftover debug log?

manuelleduc · 2025-02-21T08:58:39Z

github-authentication/src/config.ts

+const config = {
+  GITHUB_DEVICE_LOGIN_URL: "https://github.com/login/device/code",
+  GITHUB_DEVICE_VERIFY_URL: "https://github.com/login/oauth/access_token",
+  GITHUB_APP_CLIENT_ID: "",


is it required to update this value and recompile to have a working instance with a Github backend?

ok, got it, this is specifically for the third party github auth server, ok for now

manuelleduc · 2025-02-21T09:02:15Z

skin/src/vue/c-content.vue

  if (cristal && contentRoot.value) {
    ContentTools.listenToClicks(contentRoot.value, cristal);
    ContentTools.transformMacros(contentRoot.value, cristal);
+    ContentTools.transformImagesInElement(contentRoot.value, cristal);


Does not seems related to the PR?

It was, actually, kind of. We did not catch that image transformation stopped working with the current backends because images are supposed to be handled as attachments, and the GitHub backend is the one right now that actually uses it (to display images stored in the repository).

manuelleduc · 2025-02-21T09:08:58Z

web/public/config.json

+    "configType": "GitHub",
+    "serverRendering": false,
+    "offline": false,
+    "baseURL": "https://raw.githubusercontent.com/ldubost/test/master/",


Just for confirmation, this means that a config entry is currently required for each repo we want to give access to in a given Cristal instance?

At this point, yes.

ok, I suggest creating a Jira issue for improving that (and other limitations you know of) so that we have a clear vision of where we are at.

manuelleduc · 2025-02-21T10:43:02Z

core/history/history-github/src/components/componentsInit.ts

+        .get()
+        ?.getAuthorizationHeader();
+      const headers: { Accept: string; Authorization?: string } = {
+        Accept: "application/json",


Missing authentication headers

manuelleduc · 2025-02-21T10:49:11Z

edit is displayed even when not logged in
missing header on history query
for now, we'll keep using the same json based storage, we'll switch for all backends at once (makes it easier to support internal links, new pages creation and attachments)
document all the setup in the wiki

manuelleduc · 2025-02-21T11:06:59Z

introduce authentication-github-ui
load it from github.ts in lib
move GitHubLoginMenu.vue in authentication-github-ui and define an extension UI component for it, with a sidebar.actions UIXP
update GitHubLoginMenu.vue to have an empty activator (see AttachmentPreview.vue for inspiration)
on GitHubAuthenticationManager.start change the vmodel of GitHubLoginMenu.vue to make the modal visible

CRISTAL-381: Make the github backend supported

c1eb529

* Working authentication * Content display * Content edition * Content deletion * Updated navigation tree to use public API * Updated history to use public API

pjeanjean assigned manuelleduc Feb 20, 2025

manuelleduc reviewed Feb 20, 2025

View reviewed changes

Merge branch 'main' into CRISTAL-381

c7648c8

manuelleduc reviewed Feb 21, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CRISTAL-381: Make the github backend supported #687

CRISTAL-381: Make the github backend supported #687

pjeanjean commented Feb 20, 2025

manuelleduc Feb 20, 2025

manuelleduc Feb 20, 2025

manuelleduc Feb 21, 2025

manuelleduc Feb 21, 2025

manuelleduc Feb 21, 2025

manuelleduc Feb 21, 2025

manuelleduc Feb 21, 2025

manuelleduc Feb 21, 2025

manuelleduc Feb 21, 2025

pjeanjean Feb 21, 2025

manuelleduc Feb 21, 2025

manuelleduc Feb 21, 2025

manuelleduc Feb 21, 2025

manuelleduc Feb 21, 2025

manuelleduc Feb 21, 2025

pjeanjean Feb 21, 2025

manuelleduc Feb 21, 2025

pjeanjean Feb 21, 2025

manuelleduc Feb 21, 2025

manuelleduc Feb 21, 2025

manuelleduc commented Feb 21, 2025

manuelleduc commented Feb 21, 2025

CRISTAL-381: Make the github backend supported #687

Are you sure you want to change the base?

CRISTAL-381: Make the github backend supported #687

Conversation

pjeanjean commented Feb 20, 2025

Jira URL

Changes

Description

Clarifications

Screenshots & Video

Executed Tests

Expected merging strategy

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

manuelleduc commented Feb 21, 2025

manuelleduc commented Feb 21, 2025