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

ExApp version check #29

Merged
merged 10 commits into from
Aug 9, 2023
Merged

ExApp version check #29

merged 10 commits into from
Aug 9, 2023

Conversation

andrey18106
Copy link
Collaborator

@andrey18106 andrey18106 commented Aug 2, 2023

  • Resolve conflicts
  • Merge into auth throttling PR first

@bigcat88
Copy link
Member

bigcat88 commented Aug 2, 2023

Thoughts:

  1. Admin do not stops ExApp and updates it manually(with new Docker image).
  2. New version of ExApp wants to register new stuff, it did not do that in this case.
  3. App already got displayed as updated in NC, it can not be updated automaticly, as we just imitate update here.

@bigcat88
Copy link
Member

bigcat88 commented Aug 2, 2023

First solution:

  1. Got another version then expected
  2. We disable app
  3. We enable app, accepting all API scopes automatically (as with flag --force-scopes)

Pros:

  • easy to manual update application

Cons:

  • app can imitate that it's version changed, and rerequest some API scopes that was not granted to it.

@bigcat88
Copy link
Member

bigcat88 commented Aug 2, 2023

Second solution:

  1. Got another version then expected
  2. We disable app immidiatly and notify admin about disabling(how can we know who shuold be notified?)

Pros:

  • admin in this case, just should enable app in the UI or console, accept or decline new API scopes requested from ExApp if any and that's all

Cons:

  • We do not know know whom can be notified about this. It is ugly to create such field only for the AppEcosystem. If we imagine such field will present in the Nextcloud ❤️

@julien-nc

@bigcat88 bigcat88 added help wanted Extra attention is needed enhancement New feature or request labels Aug 2, 2023
@julien-nc
Copy link
Member

how can we know who shuold be notified?

All members of the admin group should be fine.

It feels weird to automatically accept new scopes from an ExApp. It makes the scope system useless when an ExApp is updated manually outside of NC.
I would go for the second solution.

Also, we could recommend not to manually update ExApps as it can lead to longer app downtimes (until an admin validates the new scopes).

Also, it is very likely that the person who updates an ExApp manually is a NC admin and knows it's wise to check the "app settings v2" to validate new scopes. So even if the notification is not perfect and can be seen late, it's good enough for a use case that is not recommended. We can decide people updating ExApps manually should know what they are doing and it's fine if the workflow is not perfect.

Copy link
Member

@bigcat88 bigcat88 left a comment

Choose a reason for hiding this comment

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

Till this got rewritten to new algo

andrey18106 and others added 4 commits August 8, 2023 23:42
Disable ExApp and send notifications to admins

Added ExAppAdminNotifier

Minor fixes
Signed-off-by: Alexander Piskun <bigcat88@icloud.com>
Signed-off-by: Alexander Piskun <bigcat88@icloud.com>
Signed-off-by: Alexander Piskun <bigcat88@icloud.com>
@bigcat88
Copy link
Member

bigcat88 commented Aug 8, 2023

will fix tomorrow, need to extend nc_py_api by adding two new APIs for extra applications.

In wait for #35 to fix test

@andrey18106 andrey18106 changed the title ExApp version check draft ExApp version check Aug 9, 2023
andrey18106 and others added 4 commits August 9, 2023 17:33
Resolves: #35.

Return consistent ExApps list structure.

---------

Signed-off-by: Alexander Piskun <bigcat88@icloud.com>
Co-authored-by: Alexander Piskun <bigcat88@icloud.com>
Signed-off-by: Alexander Piskun <bigcat88@icloud.com>
@andrey18106 andrey18106 merged commit eabfb2d into main Aug 9, 2023
22 checks passed
@andrey18106 andrey18106 deleted the version-check-handling branch August 9, 2023 15:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants