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

Add API with browser session #20613

Closed

Conversation

tyroneyeh
Copy link
Contributor

Closes #20585

@Gusted
Copy link
Contributor

Gusted commented Aug 2, 2022

I don't think this is correct, the API shouldn't use a sessioner, The API shouldn't be accessible via some sort of CSRF token. Just create a user token for this purpose.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Aug 2, 2022
@wxiaoguang
Copy link
Contributor

wxiaoguang commented Aug 2, 2022

Some background (quoted from another comment)

I think it's worth to write some design document to clarify the architecture, to make the roadmap clear. Maybe one day in the future enough people want to make the API use session again, no matter what happens, it really needs to be discussed and make a consensus and have a document.

For me, I also think it's not fine to make API use session again at the moment, no benefit.


Usually, the web backend and api backend is different. And there were some PRs to decouple the API endpoints from the web. Some backgrounds:

@tyroneyeh
Copy link
Contributor Author

tyroneyeh commented Aug 2, 2022

My current use is to add javascript to the Gitea interface, get some information through the API, and display it in the original interface, such as report information
Not other systems, what should I do?

@Gusted
Copy link
Contributor

Gusted commented Aug 2, 2022

My current use is to add javascript to the Gitea interface, get some information through the API, and display it in the original interface, such as report information
Other systems cannot be accessed, what should I do?

No idea what info you're accessing, but it might be possible that you could access it via the current web API's.

@wxiaoguang
Copy link
Contributor

wxiaoguang commented Aug 2, 2022

My current use is to add javascript to the Gitea interface, get some information through the API, and display it in the original interface, such as report information Not other systems, what should I do?

TBH, no idea from my side. Making the API use session (correctly) might be a solution. That's what I meant Maybe one day in the future enough people want to make the API use session again and it really needs to be discussed and make a consensus and have a document.


I think you can describe your requirements with more details and examples, and ping @go-gitea/maintainers

@tyroneyeh
Copy link
Contributor Author

The data I need to organize, the format output by the original UI cannot be used directly. For example, I want to organize the issue I reported in the past week output my format. My browser has already been authenticated, and I have to re-authenticate it when I use AJAX to read it. which doesn't make sense

@tyroneyeh
Copy link
Contributor Author

To give another example, for example, I have added a desktop notification program information to the footer.tmpl through the API, if there is no browser session, it will not be able to get

'Notification' in window && Notification.requestPermission().then(function(permission) {
    if ('granted' == permission) {
        const channel = new BroadcastChannel('logout-notify'), notificationfunc = function() {
            $.getJSON('/api/v1/notifications?limit=10', {_csrf: config.csrfToken}).then(function(response) {
                if (response.length) {
                    $('.notification_count').text(response.length);
                    $('.notification_count.hidden').removeClass('hidden');
                    const notificationed = localStorage.getItem('notificationed');
                    response.reverse();
                    $(response).each(function(_, data) {
                        if (!notificationed || notificationed && data.updated_at > notificationed) {
                            localStorage.setItem('notificationed', data.updated_at);
                            new Notification(data.repository.full_name +'#'+ data.subject.html_url.slice(data.subject.html_url.lastIndexOf('/') + 1) +' '+ data.subject.state, {
                                icon: window.config.appUrl + 'assets/img/favicon.png',
                                body: data.subject.title,
                                tag: 'Gitea'+ data.id
                            }).onclick = function(e) {
                                e.preventDefault();
                                window.open(data.subject.latest_comment_html_url || data.subject.html_url);
                            };
                        }
                    });
                }
            });
        }, startnotification = function() {
            if (!notificationtimer)
                notificationtimer = setInterval(notificationfunc, 60000);
        };
        channel.onmessage = function() {
            clearTimeout(notificationtimer);
            notificationtimer = 0;
        };
        channel.postMessage('');
        startnotification();
        // notificationfunc();
        window.onfocus = function() {
            startnotification();
            channel.postMessage('');
        };
    }
});

@lunny
Copy link
Member

lunny commented Aug 4, 2022

To give another example, for example, I have added a desktop notification program information to the footer.tmpl through the API, if there is no browser session, it will not be able to get

'Notification' in window && Notification.requestPermission().then(function(permission) {
    if ('granted' == permission) {
        const channel = new BroadcastChannel('logout-notify'), notificationfunc = function() {
            $.getJSON('/api/v1/notifications?limit=10', {_csrf: config.csrfToken}).then(function(response) {
                if (response.length) {
                    $('.notification_count').text(response.length);
                    $('.notification_count.hidden').removeClass('hidden');
                    const notificationed = localStorage.getItem('notificationed');
                    response.reverse();
                    $(response).each(function(_, data) {
                        if (!notificationed || notificationed && data.updated_at > notificationed) {
                            localStorage.setItem('notificationed', data.updated_at);
                            new Notification(data.repository.full_name +'#'+ data.subject.html_url.slice(data.subject.html_url.lastIndexOf('/') + 1) +' '+ data.subject.state, {
                                icon: window.config.appUrl + 'assets/img/favicon.png',
                                body: data.subject.title,
                                tag: 'Gitea'+ data.id
                            }).onclick = function(e) {
                                e.preventDefault();
                                window.open(data.subject.latest_comment_html_url || data.subject.html_url);
                            };
                        }
                    });
                }
            });
        }, startnotification = function() {
            if (!notificationtimer)
                notificationtimer = setInterval(notificationfunc, 60000);
        };
        channel.onmessage = function() {
            clearTimeout(notificationtimer);
            notificationtimer = 0;
        };
        channel.postMessage('');
        startnotification();
        // notificationfunc();
        window.onfocus = function() {
            startnotification();
            channel.postMessage('');
        };
    }
});

Create a new router for web routers.

@tyroneyeh
Copy link
Contributor Author

Porting the same code from API to Web Router? Or from web router call API functions directly?

@lunny
Copy link
Member

lunny commented Aug 4, 2022

Porting the same code from API to Web Router? Or from web router call API functions directly?

The function maybe reused.

@tyroneyeh
Copy link
Contributor Author

Adding m.Get("/list", notify.ListNotifications) in router web.go shows error

2022/08/04 21:16:12 [62ebc69c] router: failed    GET /notifications/list?limit=10 for [::1]:54238, panic in 6.2ms @ notify/user.go:18(notify.ListNotifications), err=interface conversion: interface {} is nil, not *context.APIContext

@tyroneyeh
Copy link
Contributor Author

Error details:

An error occurred:

PANIC: interface conversion: interface {} is nil, not *context.APIContext
/usr/local/Cellar/go/1.18.3/libexec/src/runtime/iface.go:262 (0x400d369)
panicdottypeE: panic(&TypeAssertionError{iface, have, want, ""})
/Users/tyroneyeh/workspace/github/gitea/modules/context/api.go:140 (0x5b5520c)
GetAPIContext: return req.Context().Value(apiContextKey).(*APIContext)
/Users/tyroneyeh/workspace/github/gitea/modules/web/wrap_convert.go:62 (0x5b5517a)
convertHandler.func5: ctx := context.GetAPIContext(req)
/Users/tyroneyeh/workspace/github/gitea/modules/web/wrap.go:41 (0x5b53689)
wrapInternal.func1: done, deferrable := handler(resp, req, others...)
/usr/local/Cellar/go/1.18.3/libexec/src/net/http/server.go:2084 (0x452fe8e)
HandlerFunc.ServeHTTP: f(w, r)
/Users/tyroneyeh/go/pkg/mod/github.com/go-chi/chi/v5@v5.0.7/mux.go:442 (0x57636d5)
(*Mux).routeHTTP: h.ServeHTTP(w, r)
/usr/local/Cellar/go/1.18.3/libexec/src/net/http/server.go:2084 (0x452fe8e)
HandlerFunc.ServeHTTP: f(w, r)
/Users/tyroneyeh/workspace/github/gitea/modules/web/wrap.go:63 (0x5b53b0f)
Middle.func1.1: next.ServeHTTP(ctx.Resp, ctx.Req)
/usr/local/Cellar/go/1.18.3/libexec/src/net/http/server.go:2084 (0x452fe8e)
HandlerFunc.ServeHTTP: f(w, r)
/Users/tyroneyeh/workspace/github/gitea/modules/web/wrap.go:63 (0x5b53b0f)
Middle.func1.1: next.ServeHTTP(ctx.Resp, ctx.Req)
/usr/local/Cellar/go/1.18.3/libexec/src/net/http/server.go:2084 (0x452fe8e)
HandlerFunc.ServeHTTP: f(w, r)
/Users/tyroneyeh/workspace/github/gitea/modules/web/wrap.go:63 (0x5b53b0f)
Middle.func1.1: next.ServeHTTP(ctx.Resp, ctx.Req)
/usr/local/Cellar/go/1.18.3/libexec/src/net/http/server.go:2084 (0x452fe8e)
HandlerFunc.ServeHTTP: f(w, r)
/Users/tyroneyeh/go/pkg/mod/github.com/go-chi/chi/v5@v5.0.7/middleware/get_head.go:37 (0x5c0ac44)
GetHead.func1: next.ServeHTTP(w, r)
/usr/local/Cellar/go/1.18.3/libexec/src/net/http/server.go:2084 (0x452fe8e)
HandlerFunc.ServeHTTP: f(w, r)
/Users/tyroneyeh/workspace/github/gitea/modules/web/wrap.go:63 (0x5b53b0f)
Middle.func1.1: next.ServeHTTP(ctx.Resp, ctx.Req)
/usr/local/Cellar/go/1.18.3/libexec/src/net/http/server.go:2084 (0x452fe8e)
HandlerFunc.ServeHTTP: f(w, r)
/Users/tyroneyeh/workspace/github/gitea/modules/context/context.go:802 (0x57744ba)
Contexter.func1.1: next.ServeHTTP(ctx.Resp, ctx.Req)
/usr/local/Cellar/go/1.18.3/libexec/src/net/http/server.go:2084 (0x452fe8e)
HandlerFunc.ServeHTTP: f(w, r)
/Users/tyroneyeh/go/pkg/mod/github.com/go-chi/chi/v5@v5.0.7/mux.go:71 (0x576150c)
(*Mux).ServeHTTP: mx.handler.ServeHTTP(w, r)
/Users/tyroneyeh/go/pkg/mod/github.com/go-chi/chi/v5@v5.0.7/mux.go:314 (0x5762ebb)
(*Mux).Mount.func1: handler.ServeHTTP(w, r)
/usr/local/Cellar/go/1.18.3/libexec/src/net/http/server.go:2084 (0x452fe8e)
HandlerFunc.ServeHTTP: f(w, r)
/Users/tyroneyeh/go/pkg/mod/github.com/go-chi/chi/v5@v5.0.7/mux.go:442 (0x57636d5)
(*Mux).routeHTTP: h.ServeHTTP(w, r)
/usr/local/Cellar/go/1.18.3/libexec/src/net/http/server.go:2084 (0x452fe8e)
HandlerFunc.ServeHTTP: f(w, r)
/Users/tyroneyeh/workspace/github/gitea/routers/web/base.go:174 (0x5ddf011)
Recovery.func1.1: next.ServeHTTP(w, req)
/usr/local/Cellar/go/1.18.3/libexec/src/net/http/server.go:2084 (0x452fe8e)
HandlerFunc.ServeHTTP: f(w, r)
/Users/tyroneyeh/go/pkg/mod/gitea.com/go-chi/session@v0.0.0-20211218221615-e3605d8b28b8/session.go:257 (0x513b6dd)
Sessioner.func1.1: next.ServeHTTP(w, req)
/usr/local/Cellar/go/1.18.3/libexec/src/net/http/server.go:2084 (0x452fe8e)
HandlerFunc.ServeHTTP: f(w, r)
/Users/tyroneyeh/workspace/github/gitea/modules/web/wrap.go:110 (0x5b545a8)
WrapWithPrefix.func1.1: next.ServeHTTP(resp, req)
/usr/local/Cellar/go/1.18.3/libexec/src/net/http/server.go:2084 (0x452fe8e)
HandlerFunc.ServeHTTP: f(w, r)
/Users/tyroneyeh/go/pkg/mod/github.com/go-chi/chi/v5@v5.0.7/mux.go:71 (0x576150c)
(*Mux).ServeHTTP: mx.handler.ServeHTTP(w, r)
/Users/tyroneyeh/go/pkg/mod/github.com/go-chi/chi/v5@v5.0.7/mux.go:314 (0x5762ebb)
(*Mux).Mount.func1: handler.ServeHTTP(w, r)
/usr/local/Cellar/go/1.18.3/libexec/src/net/http/server.go:2084 (0x452fe8e)
HandlerFunc.ServeHTTP: f(w, r)
/Users/tyroneyeh/go/pkg/mod/github.com/go-chi/chi/v5@v5.0.7/mux.go:442 (0x57636d5)
(*Mux).routeHTTP: h.ServeHTTP(w, r)
/usr/local/Cellar/go/1.18.3/libexec/src/net/http/server.go:2084 (0x452fe8e)
HandlerFunc.ServeHTTP: f(w, r)
/Users/tyroneyeh/workspace/github/gitea/routers/common/middleware.go:79 (0x5c10242)
Middlewares.func2.1: next.ServeHTTP(resp, req)
/usr/local/Cellar/go/1.18.3/libexec/src/net/http/server.go:2084 (0x452fe8e)
HandlerFunc.ServeHTTP: f(w, r)
/Users/tyroneyeh/workspace/github/gitea/modules/web/routing/logger_manager.go:123 (0x5b4f3af)
(*requestRecordsManager).handler.func1: next.ServeHTTP(w, req)
/usr/local/Cellar/go/1.18.3/libexec/src/net/http/server.go:2084 (0x452fe8e)
HandlerFunc.ServeHTTP: f(w, r)
/Users/tyroneyeh/go/pkg/mod/github.com/go-chi/chi/v5@v5.0.7/middleware/strip.go:30 (0x5c0d9b8)
StripSlashes.func1: next.ServeHTTP(w, r)
/usr/local/Cellar/go/1.18.3/libexec/src/net/http/server.go:2084 (0x452fe8e)
HandlerFunc.ServeHTTP: f(w, r)
/Users/tyroneyeh/go/pkg/mod/github.com/chi-middleware/proxy@v1.1.1/middleware.go:37 (0x5c0a2b6)
ForwardedHeaders.func1.1: h.ServeHTTP(w, r)
/usr/local/Cellar/go/1.18.3/libexec/src/net/http/server.go:2084 (0x452fe8e)
HandlerFunc.ServeHTTP: f(w, r)
/Users/tyroneyeh/workspace/github/gitea/routers/common/middleware.go:32 (0x5c10092)
Middlewares.func1.1: next.ServeHTTP(context.NewResponse(resp), req.WithContext(ctx))
/usr/local/Cellar/go/1.18.3/libexec/src/net/http/server.go:2084 (0x452fe8e)
HandlerFunc.ServeHTTP: f(w, r)
/Users/tyroneyeh/go/pkg/mod/github.com/go-chi/chi/v5@v5.0.7/mux.go:88 (0x57614c1)
(*Mux).ServeHTTP: mx.handler.ServeHTTP(w, r)
/Users/tyroneyeh/workspace/github/gitea/modules/web/route.go:200 (0x5b52acd)
(*Route).ServeHTTP: r.R.ServeHTTP(w, req)
/usr/local/Cellar/go/1.18.3/libexec/src/net/http/server.go:2916 (0x453347a)
serverHandler.ServeHTTP: handler.ServeHTTP(rw, req)
/usr/local/Cellar/go/1.18.3/libexec/src/net/http/server.go:1966 (0x452e936)
(*conn).serve: serverHandler{c.server}.ServeHTTP(w, w.req)
/usr/local/Cellar/go/1.18.3/libexec/src/runtime/asm_amd64.s:1571 (0x406dae0)
goexit: BYTE $0x90 // NOP
Gitea Version: 1.18.0+dev-215-g7cc7c3e44

@lunny
Copy link
Member

lunny commented Aug 4, 2022

Yes, the contexts are different from Web and API routers.

@tyroneyeh
Copy link
Contributor Author

Why can't we use the existing API and need to write another web router? what else can this API do?

@lunny
Copy link
Member

lunny commented Aug 5, 2022

Why can't we use the existing API and need to write another web router? what else can this API do?

The design to split UI rourters from API make things more flexible. Here is the pros,

  • You can change API and don't worry whether it will break UI.
  • It becomes possible to deploy an API-only Gitea instance.
  • It's easier to request API from applications not only web browsers.

@wxiaoguang
Copy link
Contributor

wxiaoguang commented Aug 5, 2022

Why can't we use the existing API and need to write another web router? what else can this API do?

The design to split UI rourters from API make things more flexible. Here is the pros,
* You can change API and don't worry whether it will break UI.
* It becomes possible to deploy an API-only Gitea instance.
* It's easier to request API from applications not only web browsers.

It only means that "do not put WebUI-only routers to API routers", it doesn't answer why "the API shouldn't be accessed by session".

"making the API can be accessed by session" and "keeping WebUI-only routers in web routers" can also have these Pros.

@lafriks lafriks added the status/blocked This PR cannot be merged yet, i.e. because it depends on another unmerged PR label Aug 6, 2022
@lafriks
Copy link
Member

lafriks commented Aug 6, 2022

It may have security implications if API can be called directly using user session and without CSRF token

@tyroneyeh
Copy link
Contributor Author

It may have security implications if API can be called directly using user session and without CSRF token

Added

@lunny
Copy link
Member

lunny commented Aug 7, 2022

Why can't we use the existing API and need to write another web router? what else can this API do?

The design to split UI rourters from API make things more flexible. Here is the pros,

  • You can change API and don't worry whether it will break UI.
  • It becomes possible to deploy an API-only Gitea instance.
  • It's easier to request API from applications not only web browsers.

It only means that "do not put WebUI-only routers to API routers", it doesn't answer why "the API shouldn't be accessed by session".

"making the API can be accessed by session" and "keeping WebUI-only routers in web routers" can also have these Pros.

Once API supports session, they could be used from UI with no error. Then you have to write some documents to prevent users to send PRs to do that. So I think making it impossible technically is reasonable.

@tyroneyeh tyroneyeh closed this Aug 18, 2022
@tyroneyeh tyroneyeh deleted the Add-API-with-browser-session branch August 18, 2022 06:26
@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. status/blocked This PR cannot be merged yet, i.e. because it depends on another unmerged PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[v1.17.0] API cannot use _csrf token get information
6 participants