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

added CORS skip if session was created by AppAPI #40737

Merged
merged 3 commits into from
Oct 9, 2023
Merged

Conversation

bigcat88
Copy link
Member

@bigcat88 bigcat88 commented Oct 2, 2023

Related to: nextcloud/app_api/issues/80

This PR is about for the discussion, how to implement it in a right way.

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

bigcat88 commented Oct 2, 2023

Changes in AppAPI related to this: nextcloud/app_api/pull/81

After merge of this and backport to 27 we can publish AppAPI in the AppStore, and say all is working starting from 27.1.2

@solracsf solracsf added this to the Nextcloud 28 milestone Oct 2, 2023
@julien-nc
Copy link
Member

Maybe we don't need this. We don't have to support non-OCS endpoints in the app ecosystem.
WDYT @AndyScherzinger @nickvergessen ?

@bigcat88
Copy link
Member Author

bigcat88 commented Oct 3, 2023

Just to clarify why I propose to add the support:

  1. These all are not OCS, but RestAPI:
  1. We can easy add support for them, without breaking any existing stuff(if I didn't miss anything)

@julien-nc
Copy link
Member

@bigcat88 You're right, those APIs are not just internal but made to be used by clients. It would be nice to be able to use them in external apps.

@nickvergessen
Copy link
Member

Maybe we don't need this. We don't have to support non-OCS endpoints in the app ecosystem.

So yes, the apps should just have provide OCS APIs to begin with. But people follow old tutorials, so this is where we are.

I guess for now it's simpler to add this hack, but I also would prefer to simply require OCS APIs for support.
Maybe we can add a deprecation note into the app developer docs and then simply require OCS routes in a future version (1 year from now?) for app ecosystem support?
Would allow a quick transition for the moment and still ensure good future behaviour?

@blizzz blizzz mentioned this pull request Oct 5, 2023
1 task
@blizzz blizzz modified the milestones: Nextcloud 27.1.2, Nextcloud 28 Oct 5, 2023
@blizzz
Copy link
Member

blizzz commented Oct 5, 2023

master is 28

@bigcat88
Copy link
Member Author

bigcat88 commented Oct 5, 2023

master is 28

why this can not be backported to 27.1.2?

@nickvergessen
Copy link
Member

This PR targets master, so the milestone must be what master is which is 28.
Backporting can still be done, and then the backport PR will have a 27.x.y milestone assigned.

@DaphneMuller
Copy link

@AndyScherzinger could you perhaps help us with merging this in the right milestone? 27.1.2

@nickvergessen
Copy link
Member

could you perhaps help us with merging this in the right milestone? 27.1.2

Just needs the backport bot summoned. Then it will be backported to stable27 and be part of 27.1.3 (.1.2 is already being released at the moment).

But first CI needs fixing:
grafik

@nickvergessen
Copy link
Member

/backport to stable27

@AndyScherzinger
Copy link
Member

@DaphneMuller what @nickvergessen said, so when this is merged (for 28) the already requested backport bot (thanks to nick) will take care of the backport and will also assign the right v27.1.x version number. 👍

@bigcat88
Copy link
Member Author

bigcat88 commented Oct 5, 2023

@DaphneMuller what @nickvergessen said, so when this is merged (for 28) the already requested backport bot (thanks to nick) will take care of the backport and will also assign the right v27.1.x version number. 👍

I thought that this would be for release 27.1.2, because the date in the release calendar for 27.1.2 was in 10 days, not today. I see that the date in the calendar was wrong and has already been corrected.

thank you, we will fix the error that the tests found and it will be by 27.1.3

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

bigcat88 commented Oct 6, 2023

Done. Cypress errors looks unrelated to this, as the same Cypress errors are currently in other PRs.

@nickvergessen
Copy link
Member

Cypress errors looks unrelated to this

On this PR all cypress requests fail in "before", so it can't even log in.
Are we sure it's unrelated?

@bigcat88
Copy link
Member Author

bigcat88 commented Oct 9, 2023

Cypress errors looks unrelated to this

On this PR all cypress requests fail in "before", so it can't even log in. Are we sure it's unrelated?

it was unrelated, now CI is green

@bigcat88 bigcat88 merged commit 4e8ec0b into master Oct 9, 2023
41 checks passed
@bigcat88 bigcat88 deleted the cors-app_api branch October 9, 2023 10:40
@backportbot-nextcloud
Copy link

The backport to stable27 failed. Please do this backport manually.

# Switch to the target branch and update it
git checkout stable27
git pull origin stable27

# Create the new backport branch
git checkout -b fix/foo-stable27

# Cherry pick the change from the commit sha1 of the change against the default branch
# This might cause conflicts. Resolve them.
git cherry-pick abc123

# Push the cherry pick commit to the remote repository and open a pull request
git push origin fix/foo-stable27

More info at https://docs.nextcloud.com/server/latest/developer_manual/getting_started/development_process.html#manual-backport

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants