Arc 1456 auto app creation #1401

Harminder84 · 2022-07-22T02:47:55Z

What's in this PR?
Automatic app creation has been implemented using App manifest flow.

Sequence diagram

Video demonstrating the feature

Screen.Recording.2022-07-25.at.4.07.55.pm.mov

Why
This is one of the options to create GitHub app for enterpise

Added feature flags
Existing flag GHE_SERVER

Affected issues
ARC-1456

How has this been tested?
Unit test cases added for new routes

rachellerathbone · 2022-07-22T03:25:56Z

ghe-app-manifest.template.json

@@ -0,0 +1,34 @@
+{
+  "name": "ghe-app-for-jira",


We probably want this to be an empty string right? Otherwise this will auto-fill the input for every customer which they will then have to delete.

@rachellerathbone, don't we want to give any relevant default name?

it's not a bad idea, however, something to consider is that the name has to be unique per server...

Caught up with Jade on this one and she agrees it should be blank. From a UX standpoint, we'd only pre-populate an input field if it wasn't clear what a user is meant to enter. However, is this case, app name is pretty straightforward. Plus, to @mboudreau's point: GH app names need to be unique per server so they second a customer tries to create a second app, this would fail.

Just out of curiosity, what happens when you try and add an existing app name in the input? Guessing GH renders some kind of error but want to be sure.

GH renders error if you try to add an exisiting app name -

Interestingly you can proceed without giving app name and will eventually fails when trying to complete app manifest flow. Not sure issue with specific version of GH Enterprise.

static/js/jira-server-url.js

rachellerathbone · 2022-07-22T03:31:41Z

Can you add a video recording of the implementation now that you've included the whole flow (from clicking 'create' on auto/manual through to the redirect screen).

Harminder84 · 2022-07-22T03:41:55Z

views/jira-redirect.hbs

@@ -0,0 +1,15 @@
+<!doctype html>


This temporary page is created to test the flow. Actual page will be developed as part of another ticket.

krazziekay

I'd suggest to add test cases for the new routes.

Harminder84 · 2022-07-25T01:03:43Z

src/routes/github/manifest/github-manifest-complete-get.ts

+		installationId: installation.id
+	});
+	req.session.temp = undefined;
+	res.json({ success:true });


Need to render app installation page which is part of another ticket and is in progress.

mboudreau · 2022-07-27T00:51:43Z

src/routes/github/manifest/github-manifest-complete-get.ts

+import { Installation } from "~/src/models/installation";
+
+export const GithubManifestCompleteGet = async (req: Request, res: Response) => {
+	const uuid = req.params.uuid;


where is this coming from?

This is coming from redirect_url where UUID is part of path parameter.
"redirect_url": "<<APP_HOST>>/github/manifest/<<UUID>>/complete"

maybe we can put both uuid and gheHost in the session cookie, as sessio cookie is signed cannot be tempered.

ghe-app-manifest.template.json

rachellerathbone · 2022-07-27T01:03:38Z

static/js/jira-redirect.js

@@ -0,0 +1,22 @@
+const params = new URLSearchParams(window.location.search.substring(1));
+const gheHost = params.get("serverUrl");


Where are we passing this as a param? In jira-server-url.js it looks like we're setting customData to an empty object if no app exists const customData = data.appExists ? { serverUrl: gheServerURL } : {}; so from empty state we're never passing this value to the URL. And then in github-app-creation.js the only thing we're pulling from params is the jiraHost. So my guess here is that gheHost will be null.

We need to fix this and also add error handling in the event there isn't a gheHost.

You are correct @rachellerathbone. I discussed with @krazziekay, he already worked on it and serverUrl will be available in URL once his PR gets merged.

mboudreau · 2022-07-27T01:12:17Z

ghe-app-manifest.template.json

+  "redirect_url": "<<APP_HOST>>/github/manifest/<<UUID>>/complete",
+  "hook_attributes": {
+    "url": "<<APP_HOST>>/github/<<UUID>>/events"
+  },
+  "setup_url": "<<APP_HOST>>/github/<<UUID>>/setup",


Can you explain the flow here? UUID is setup beforehand but the app is not installed yet, so what's the point of having the UUID there?

Also, keep in mind that we might use this app manifest for our own dev flow. As might want to keep it a bit more abstracted?

The flow is. : -

Construct JSON for GitHub app

Submit JSON data to GitHub Enterprise

GitHub Enterpise redirected back to redirect_url with code which is supplied as JSON data

Exchage code with GitHub Enterprise

Save data to GitHubServerApps table

When we're submitting data to GH Enterprise in step 2, we need to construct setup_url, webhook_url and others with UUID.

@mboudreau do you have thoughts on how we should handle the url value here? This is required and is meant to link to the apps homepage. However, we don't have the app name at this point (even if we prepopulate, people can edit it). The best I can see us doing is entering <<APP_HOST>>/apps/ and adding messaging somewhere in the auto flow to let customers know they will need to update this in their settings manually?

Maybe we should get together on this, but my understanding is that we shouldn't create any apps on our end until the github flow is done, which will provide us with everything we need to create the app in our DB. Creating the app beforehand means that the process might not finish and we'll have apps that were never created...

static/js/github-app-creation.js

mboudreau · 2022-07-27T01:13:16Z

src/interfaces/common.ts

+				temp?:  {
+					[key: string]: any;
+				}


Can you explain this addition and why it's needed?

I need to store GHE Host in session that can be fetched when user redirect back to redirect_url from GitHub Enterprise.

mboudreau

a few issues that needs addressing

Harminder84 · 2022-07-25T06:45:56Z

ghe-app-manifest.template.json

@@ -0,0 +1,34 @@
+{
+  "name": "ghe-app-for-jira",
+  "url": "https://github.com/marketplace/jira-software-github",


@rachellerathbone, what should be the URL for GHE app? Right now, I'm using the cloud app URL here.

Harminder84 · 2022-07-27T01:57:04Z

src/routes/github/manifest/github-manifest-complete-get.ts

+import { Installation } from "~/src/models/installation";
+
+export const GithubManifestCompleteGet = async (req: Request, res: Response) => {
+	const uuid = req.params.uuid;


This is coming from redirect_url where UUID is part of path parameter.
"redirect_url": "<<APP_HOST>>/github/manifest/<<UUID>>/complete"

Harminder84 · 2022-07-27T02:26:14Z

src/interfaces/common.ts

+				temp?:  {
+					[key: string]: any;
+				}


I need to store GHE Host in session that can be fetched when user redirect back to redirect_url from GitHub Enterprise.

Harminder84 · 2022-07-27T02:35:15Z

ghe-app-manifest.template.json

+  "redirect_url": "<<APP_HOST>>/github/manifest/<<UUID>>/complete",
+  "hook_attributes": {
+    "url": "<<APP_HOST>>/github/<<UUID>>/events"
+  },
+  "setup_url": "<<APP_HOST>>/github/<<UUID>>/setup",


The flow is. : -

Construct JSON for GitHub app

Submit JSON data to GitHub Enterprise

GitHub Enterpise redirected back to redirect_url with code which is supplied as JSON data

Exchage code with GitHub Enterprise

Save data to GitHubServerApps table

When we're submitting data to GH Enterprise in step 2, we need to construct setup_url, webhook_url and others with UUID.

Harminder84 · 2022-07-27T02:54:11Z

static/js/jira-redirect.js

@@ -0,0 +1,22 @@
+const params = new URLSearchParams(window.location.search.substring(1));
+const gheHost = params.get("serverUrl");


You are correct @rachellerathbone. I discussed with @krazziekay, he already worked on it and serverUrl will be available in URL once his PR gets merged.

Harminder84 · 2022-07-27T05:30:29Z

ghe-app-manifest.template.json

@@ -0,0 +1,34 @@
+{
+  "name": "ghe-app-for-jira",


GH renders error if you try to add an exisiting app name -

Harminder84 · 2022-07-27T05:32:28Z

ghe-app-manifest.template.json

@@ -0,0 +1,34 @@
+{
+  "name": "ghe-app-for-jira",


Interestingly you can proceed without giving app name and will eventually fails when trying to complete app manifest flow. Not sure issue with specific version of GH Enterprise.

static/js/jira-app-creation.js

static/js/jira-redirect.js

src/routes/github/manifest/github-manifest-complete-get.ts

gxueatlassian · 2022-07-28T02:43:41Z

src/routes/github/manifest/github-manifest-complete-get.ts

+import { Installation } from "~/src/models/installation";
+
+export const GithubManifestCompleteGet = async (req: Request, res: Response) => {
+	const uuid = req.params.uuid;


maybe we can put both uuid and gheHost in the session cookie, as sessio cookie is signed cannot be tempered.

src/routes/github/manifest/github-manifest-complete-get.test.ts

static/js/jira-app-creation.js

Harminder84 added 3 commits July 20, 2022 16:37

WIP

786e417

wip

8abfe9f

wip

874c2bf

rachellerathbone reviewed Jul 22, 2022

View reviewed changes

merge with main

6faf369

rachellerathbone reviewed Jul 22, 2022

View reviewed changes

static/js/jira-server-url.js Outdated Show resolved Hide resolved

Harminder84 commented Jul 22, 2022

View reviewed changes

Harminder84 added 2 commits July 22, 2022 13:46

Merge branch 'main' into ARC-1456-auto-app-creation

c3a23b1

Merge branch 'main' into ARC-1456-auto-app-creation

d60f1ad

krazziekay reviewed Jul 22, 2022

View reviewed changes

wip

025d3b0

Harminder84 commented Jul 25, 2022

View reviewed changes

Harminder84 and others added 3 commits July 25, 2022 13:41

add test

0ec19a0

Merge branch 'main' into ARC-1456-auto-app-creation

27fb7eb

Merge branch 'main' into ARC-1456-auto-app-creation

1afd6d9

Harminder84 marked this pull request as ready for review July 25, 2022 06:26

Harminder84 requested a review from a team as a code owner July 25, 2022 06:26

mboudreau reviewed Jul 27, 2022

View reviewed changes

ghe-app-manifest.template.json Outdated Show resolved Hide resolved

rachellerathbone requested changes Jul 27, 2022

View reviewed changes

mboudreau reviewed Jul 27, 2022

View reviewed changes

rachellerathbone reviewed Jul 27, 2022

View reviewed changes

static/js/github-app-creation.js Outdated Show resolved Hide resolved

mboudreau reviewed Jul 27, 2022

View reviewed changes

mboudreau suggested changes Jul 27, 2022

View reviewed changes

Harminder84 added 2 commits July 27, 2022 12:41

file renamed

451ed64

Merged with main

dd303d2

Harminder84 commented Jul 28, 2022

View reviewed changes

gxueatlassian reviewed Jul 28, 2022

View reviewed changes

krazziekay reviewed Jul 28, 2022

View reviewed changes

static/js/jira-app-creation.js Outdated Show resolved Hide resolved

Harminder84 added 3 commits July 28, 2022 15:12

wip

eb25779

wip

c118039

code refactor

3c00fb0

mboudreau approved these changes Jul 31, 2022

View reviewed changes

Merge branch 'main' into ARC-1456-auto-app-creation

73d53af

rachellerathbone approved these changes Aug 1, 2022

View reviewed changes

Merge branch 'main' into ARC-1456-auto-app-creation

abbd947

Harminder84 merged commit 2a7a6ff into main Aug 2, 2022

Harminder84 deleted the ARC-1456-auto-app-creation branch August 2, 2022 01:59

krazziekay mentioned this pull request Aug 2, 2022

Passing the GHE baseUrl for auto app creation #1442

Merged

rachellerathbone mentioned this pull request Dec 22, 2022

[Snyk] Security upgrade probot from 9.11.3 to 11.0.0 atlassian-forks/github-for-jira#21

Open

snyk-bot mentioned this pull request Dec 22, 2022

[Snyk] Security upgrade probot from 9.11.3 to 11.0.0 Mement-Mori/github-for-jira#20

Open

Manny27nyc mentioned this pull request Dec 22, 2022

[Snyk] Security upgrade probot from 9.11.3 to 11.0.0 Manny27nyc/github-for-jira#33

Open

MaxMood96 mentioned this pull request Dec 23, 2022

[Snyk] Security upgrade probot from 9.11.3 to 11.0.0 MaxMood96/github-for-jira#17

Open

nickfnblum mentioned this pull request Dec 23, 2022

[Snyk] Security upgrade probot from 9.11.3 to 11.0.0 nickfnblum/jira#19

Open

rachellerathbone mentioned this pull request Oct 13, 2024

[Snyk] Fix for 1 vulnerabilities atlassian-forks/github-for-jira#43

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Arc 1456 auto app creation #1401

Arc 1456 auto app creation #1401

Harminder84 commented Jul 22, 2022 •

edited

Loading

rachellerathbone Jul 22, 2022

Harminder84 Jul 22, 2022 •

edited

Loading

mboudreau Jul 25, 2022

rachellerathbone Jul 27, 2022

Harminder84 Jul 27, 2022

Harminder84 Jul 27, 2022

rachellerathbone commented Jul 22, 2022

Harminder84 Jul 22, 2022 •

edited

Loading

krazziekay left a comment

Harminder84 Jul 25, 2022

mboudreau Jul 27, 2022

Harminder84 Jul 27, 2022

gxueatlassian Jul 28, 2022

rachellerathbone Jul 27, 2022

Harminder84 Jul 27, 2022

mboudreau Jul 27, 2022

mboudreau Jul 27, 2022

Harminder84 Jul 27, 2022

rachellerathbone Jul 28, 2022

mboudreau Jul 28, 2022

mboudreau Jul 27, 2022

Harminder84 Jul 27, 2022

mboudreau left a comment

Harminder84 Jul 25, 2022

Harminder84 Jul 27, 2022

Harminder84 Jul 27, 2022

Harminder84 Jul 27, 2022

Harminder84 Jul 27, 2022

Harminder84 Jul 27, 2022

Harminder84 Jul 27, 2022

gxueatlassian Jul 28, 2022

		@@ -0,0 +1,22 @@
		const params = new URLSearchParams(window.location.search.substring(1));
		const gheHost = params.get("serverUrl");

Arc 1456 auto app creation #1401

Arc 1456 auto app creation #1401

Conversation

Harminder84 commented Jul 22, 2022 • edited Loading

Choose a reason for hiding this comment

Harminder84 Jul 22, 2022 • edited Loading

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

rachellerathbone commented Jul 22, 2022

Harminder84 Jul 22, 2022 • edited Loading

Choose a reason for hiding this comment

krazziekay left a 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

mboudreau left a 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

Harminder84 commented Jul 22, 2022 •

edited

Loading

Harminder84 Jul 22, 2022 •

edited

Loading

Harminder84 Jul 22, 2022 •

edited

Loading