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

Extract api for UI from api v1 routes and remove session dependent for API/v1 #16052

Closed
wants to merge 26 commits into from

Conversation

lunny
Copy link
Member

@lunny lunny commented Jun 2, 2021

Currently, API/v1 depends on Session auth check but it should be removed. Because mixed UI API requests and API v1 will make something complicated.

There are many differences between the two APIs.

  • UI API in fact should be consumed only by Gitea UI and could not be stable, it could be changed according to UI need and don't care about the break.
    But API v1 will be exposed to external applications and should be stable and less break.

  • UI API should read cookie and session like other routes but API v1 should only read HTTP header for the token when authentication, cookie and session is unnecessary.

  • UI API should have csrf check but API v1 should not have csrf check.

  • UI API should not support basic auth but API v1 should support basic auth and token auth

Currently, I copied the routes UI needed from v1 to UI but still use original functions. This is only a start and I think we can do more.

After this merged, API could be disabled or UI could be disabled if someone need that.

depends on #16455

@lunny lunny added the type/enhancement An improvement of existing functionality label Jun 2, 2021
@lunny lunny changed the title Split api for UI from api v1 routes Extract api for UI from api v1 routes Jun 2, 2021
@lunny lunny force-pushed the lunny/api_ui branch 3 times, most recently from 654d114 to d09fa02 Compare June 5, 2021 13:13
@lunny lunny added this to the 1.15.0 milestone Jun 5, 2021
@lunny lunny force-pushed the lunny/api_ui branch 2 times, most recently from 9c8a3d6 to d4ae10e Compare June 9, 2021 15:26
@lunny
Copy link
Member Author

lunny commented Jun 9, 2021

depends on #16086

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Jun 9, 2021
@6543
Copy link
Member

6543 commented Jun 9, 2021

@lunny please resolve conficts ;)

@lunny lunny added the pr/wip This PR is not ready for review label Jun 10, 2021
@techknowlogick techknowlogick modified the milestones: 1.15.0, 1.16.0 Jun 23, 2021
@lunny lunny mentioned this pull request Jul 14, 2021
@lunny lunny force-pushed the lunny/api_ui branch 2 times, most recently from d2bc268 to 46060fb Compare July 30, 2021 17:09
@lunny lunny force-pushed the lunny/api_ui branch 2 times, most recently from 40e09b2 to b2a42da Compare August 21, 2021 03:45
@codecov-commenter
Copy link

Codecov Report

Merging #16052 (b2a42da) into main (0bd58d6) will decrease coverage by 0.10%.
The diff coverage is 37.36%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main   #16052      +/-   ##
==========================================
- Coverage   45.38%   45.27%   -0.11%     
==========================================
  Files         760      764       +4     
  Lines       85495    85736     +241     
==========================================
+ Hits        38803    38820      +17     
- Misses      40410    40614     +204     
- Partials     6282     6302      +20     
Impacted Files Coverage Δ
modules/session/store.go 0.00% <0.00%> (ø)
routers/api/ui/api.go 25.11% <25.11%> (ø)
modules/context/api_ui.go 84.61% <84.61%> (ø)
modules/context/api.go 70.87% <100.00%> (-4.01%) ⬇️
routers/api/v1/api.go 77.58% <100.00%> (-0.90%) ⬇️
routers/common/security.go 100.00% <100.00%> (ø)
routers/init.go 63.41% <100.00%> (+6.27%) ⬆️
routers/web/web.go 90.40% <100.00%> (-0.13%) ⬇️
services/auth/auth.go 32.14% <100.00%> (+2.51%) ⬆️
models/repo_mirror.go 58.69% <0.00%> (-10.87%) ⬇️
... and 25 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0bd58d6...b2a42da. Read the comment docs.

@lunny lunny removed the pr/wip This PR is not ready for review label Aug 21, 2021
@lunny lunny force-pushed the lunny/api_ui branch 3 times, most recently from 7e75bc1 to 2d4dbf8 Compare October 15, 2021 08:14
@lafriks
Copy link
Member

lafriks commented Oct 15, 2021

tests are failing

modules/session/store.go Outdated Show resolved Hide resolved
@lunny lunny changed the title Extract api for UI from api v1 routes Extract api for UI from api v1 routes and remove session dependent for API/v1 Mar 25, 2022
@lunny lunny closed this Apr 4, 2022
@lunny lunny deleted the lunny/api_ui branch April 4, 2022 09:16
@lunny lunny removed this from the 1.17.0 milestone Apr 4, 2022
6543 pushed a commit that referenced this pull request Apr 7, 2022
Reusing `/api/v1` from Gitea UI Pages have pros and cons.
Pros:
1) Less code copy

Cons:
1) API/v1 have to support shared session with page requests.
2) You need to consider for each other when you want to change something about api/v1 or page.

This PR moves all dependencies to API/v1 from UI Pages.

Partially replace #16052
AbdulrhmnGhanem pushed a commit to kitspace/gitea that referenced this pull request Aug 24, 2022
Reusing `/api/v1` from Gitea UI Pages have pros and cons.
Pros:
1) Less code copy

Cons:
1) API/v1 have to support shared session with page requests.
2) You need to consider for each other when you want to change something about api/v1 or page.

This PR moves all dependencies to API/v1 from UI Pages.

Partially replace go-gitea#16052
@go-gitea go-gitea locked and limited conversation to collaborators May 3, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. type/enhancement An improvement of existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants