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

Signed install #41

Merged
merged 2 commits into from
Aug 27, 2021
Merged

Signed install #41

merged 2 commits into from
Aug 27, 2021

Conversation

skv-headless
Copy link
Contributor

@skv-headless skv-headless commented Jul 28, 2021

@@ -49,9 +49,20 @@ def verify
return false
end

if encoding_data['alg'] == 'RS256'
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 think that condition is not enough. We also need to check descriptor. Not sure if we can do that

ACE reference https://bitbucket.org/atlassian/atlassian-connect-express/src/4bb8249ec63241ba6b22bdfcf28759362c54dcc9/lib/middleware/verify-installation.js#lines-88

Copy link
Collaborator

Choose a reason for hiding this comment

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

You mean to know if we are using signed installs or not? I think we can add flag to the configuration that would solve that, WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah. I think adding flag is the only option

@skv-headless skv-headless changed the title [WIP] Signed install Signed install Jul 28, 2021
@stefan-hc
Copy link
Contributor

Looks good to me. You've asked on slack about possible things to break/test, rough list:

  • test with two dev Jira instances having app already installed: let one (J1) have installed 'previous' version of app without these changes, second one (J2) already with these changes, then run the backend for both instances:
  • both should be able to uninstall -> install with old descriptor -> uninstall
  • J2 should be able to disable -> enable (using the same app version) - I don't expect this to be affected, but better check if it still works
  • on J1: test that app upgrade (installation of the new version) does work
  • test unavailability of 'connect-install-keys.atlassian.com' host (will the app+jira provide meaningful error to the customer? will the app logs contain useful message for the app supporters?)
  • anything else? 🤔

@skv-headless
Copy link
Contributor Author

@stefan-hc

  • J2 should be able to disable -> enable (using the same app version) - I don't expect this to be affected, but better check if it still works

didn't understand 😞 what should I disable -> enable?

  • test unavailability of 'connect-install-keys.atlassian.com' host (will the app+jira provide meaningful error to the customer? will the app logs contain useful message for the app supporters?)

Added log. The error message looks like this

Screenshot 2021-07-29 at 11 50 56

@stefan-hc
Copy link
Contributor

what should I disable -> enable?

Good question. I believe (but my memory might fail me) that at some point in the past there was more buttons in "Manage your apps" page in Jira: not only "uninstall" "install" "subscribe" etc, but also "disable" "enable". If you check Atlassian documentation there are even some mentions about app disabling/enabling: https://support.atlassian.com/jira-cloud-administration/docs/integrate-apps/

Might be it is copy-paste from Jira Server/DataCenter (where you can disable arbitrary apps, heck, you can disable it's modules one by one), might be that Atlassian has removed Enabling/Disabling function from Jira Cloud...

Perhaps "subscribe / unsubscribe" is current name of "enable / disable". What I suggest:

  • play a bit with subscribe / unsubscribe - I believe these might call backend on "/enabled" "/disabled" endpoints (instead of /installed /uninstalled). I suppose nothing has changed from the Atlassian side, but it nonetheless might be good idea to check happy path
  • if subscribe/unsubscribe is doing something completely different - ignore my "J2 should be able to disable -> enable" request: it has totally no sense.

Ok for you?

@skv-headless
Copy link
Contributor Author

skv-headless commented Jul 30, 2021

what should I disable -> enable?

Good question. I believe (but my memory might fail me) that at some point in the past there was more buttons in "Manage your apps" page in Jira: not only "uninstall" "install" "subscribe" etc, but also "disable" "enable". If you check Atlassian documentation there are even some mentions about app disabling/enabling: https://support.atlassian.com/jira-cloud-administration/docs/integrate-apps/

Might be it is copy-paste from Jira Server/DataCenter (where you can disable arbitrary apps, heck, you can disable it's modules one by one), might be that Atlassian has removed Enabling/Disabling function from Jira Cloud...

Perhaps "subscribe / unsubscribe" is current name of "enable / disable". What I suggest:

  • play a bit with subscribe / unsubscribe - I believe these might call backend on "/enabled" "/disabled" endpoints (instead of /installed /uninstalled). I suppose nothing has changed from the Atlassian side, but it nonetheless might be good idea to check happy path
  • if subscribe/unsubscribe is doing something completely different - ignore my "J2 should be able to disable -> enable" request: it has totally no sense.

Ok for you?

Tested on staging. Everything works fine for me

@stefan-hc
Copy link
Contributor

Awesome.

Copy link
Contributor

@bulinutza bulinutza left a comment

Choose a reason for hiding this comment

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

Happy to see more contributors to this gem!

@bulinutza bulinutza merged commit 3fc48fe into MeisterLabs:master Aug 27, 2021
@pawelniewie pawelniewie deleted the signed-install branch August 31, 2021 08:32
@pawelniewie pawelniewie restored the signed-install branch August 31, 2021 08:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants