Summary

Fix issues with authenticating to private-API servers.

Review Request #14751 — Created Jan. 7, 2026 and submitted Jan. 14, 2026, 6:45 p.m.

Information

Owner

maubin

Repository

RBTools

Branch

master

Bugs

Depends On

Reviewers

Groups

rbtools

People

Description

RBTools assumes that the public API resources would always be public. We
weren't properly handling communication with private-API servers. This
has revealed itself in a few issues:

Web-based login caused a regression. When a command that requires the
API first initializes itself, it creates an API client and fetches the
root resource. That fetch triggers our auth handlers to prompt for
authentication, but the web login handler would break because its
callback expects the root resource and API client to already be set.
This regression was never made public, it comes from recent
work that hasn't been released yet.
If a user is logged out and does rbt logout, they'll be prompted to
login again.
If a user is logged out and does rbt login, they'll successfully
login but we'll print the "You're already logged in" message.

This change addresses those issues and makes sure that we properly
authenticate commands when interacting with private-API servers. We add
a has_session_cookie() method to the API client (and all the way down
to the server class) which returns whether we have a local session
cookie for the server. This information helps us deal with issues 2 and
3. We also update the login/logout commands to needs_api=False to
address issues 2 and 3.

The web login handler no longer assumes that a root resource is set. We
need the root to get the server info, in order to see whether web login
is supported on the server or not. In the case where we're working with a
private-API server while logged out, we'll just assume that the server
supports web login. Even if it doesn't, the auth will fail and we'll
move on to the username/password prompt handler.

Testing Done


Ran unit tests.
Tested login/logout on both private-API servers and public ones.
Tested login with api tokens, username/password, and web login.

Commits

Summary	ID
Fix issues with authenticating to private-API servers. Review Board's API has certain resources that require authentication. But some resources like its root resource, the session resource, and the server resource don't require any. However, there's a server setting that lets you disable anonymous read access to the API. When that is set, every request to the API must be authenticated. RBTools assumes that the public API resources would always be public. We weren't properly handling communication with private-API servers. This has revealed itself in a few issues: 1. Web-based login caused a regression. When a command that requires the API first initializes itself, it creates an API client and fetches the root resource. That fetch triggers our auth handlers to prompt for authentication, but the web login handler would break because its callback expects the root resource and API client to already be set. 2. If a user is logged out and does `rbt logout`, they'll be prompted to login again. 3. If a user is logged out and does `rbt login`, they'll successfully login but we'll print the "You're already logged in" message. This change addresses those issues and makes sure that we properly authenticate commands when interacting with private-API servers. We add a `has_session_cookie()` method to the API client (and all the way down to the server class) which returns whether we have a local session cookie for the server. This information helps us deal with issues 2 and 3. We also update the login/logout commands to needs_api=False to address issues 2 and 3. The web login handler no longer assumes that a root resource is set. We need the root to get the server info, in order to see whether web login is supported on the server or not. In the case where we're working with a private-API server while logged out, we'll just assume that the server supports web login. Even if it doesn't, the auth will fail and we'll move on to the username/password prompt handler.	747f1a1e7c397562e9bd2173c6ef17343cae70a2

Summary

Fix issues with authenticating to private-API servers.

Review Board's API has certain resources that require authentication. But some resources like its root resource, the session resource, and the server resource don't require any. However, there's a server setting that lets you disable anonymous read access to the API. When that is set, every request to the API must be authenticated. RBTools assumes that the public API resources would always be public. We weren't properly handling communication with private-API servers. This has revealed itself in a few issues: 1. Web-based login caused a regression. When a command that requires the API first initializes itself, it creates an API client and fetches the root resource. That fetch triggers our auth handlers to prompt for authentication, but the web login handler would break because its callback expects the root resource and API client to already be set. 2. If a user is logged out and does `rbt logout`, they'll be prompted to login again. 3. If a user is logged out and does `rbt login`, they'll successfully login but we'll print the "You're already logged in" message. This change addresses those issues and makes sure that we properly authenticate commands when interacting with private-API servers. We add a `has_session_cookie()` method to the API client (and all the way down to the server class) which returns whether we have a local session cookie for the server. This information helps us deal with issues 2 and 3. We also update the login/logout commands to needs_api=False to address issues 2 and 3. The web login handler no longer assumes that a root resource is set. We need the root to get the server info, in order to see whether web login is supported on the server or not. In the case where we're working with a private-API server while logged out, we'll just assume that the server supports web login. Even if it doesn't, the auth will fail and we'll move on to the username/password prompt handler.

747f1a1e7c397562e9bd2173c6ef17343cae70a2

Issues

Description	From	Last Updated
Why do we need three different ways to fetch this? Can you add a comment explaining?	david	Jan. 7, 2026, 10:27 p.m.
Typo: instantianting -> instantiating	david	Jan. 7, 2026, 10:28 p.m.
Instead of this, can we update the type hint of get_authenticated_session to not be \| None? According to my reading …	david	Jan. 7, 2026, 10:28 p.m.
This could produce a false-positive if a cookie's value has this set. Shouldn't happen, but I wonder if it'd make …	chipx86	Jan. 8, 2026, 9:37 p.m.
Can you update this for the full module path?	chipx86	Jan. 8, 2026, 8:41 p.m.
Looks like we only use this in the session.authenticated case. Should we fetch it inline there?	chipx86	Jan. 8, 2026, 9:36 p.m.
We can probably inline this here.	chipx86	Jan. 8, 2026, 9:27 p.m.
This can be updated to use the local api_client.	chipx86	Jan. 8, 2026, 9:27 p.m.
So this info doesn't get lost later, can we document why we're checking against these classes explicitly?	chipx86	Jan. 8, 2026, 9:30 p.m.

flake8 passed.

JSHint passed.

Change Summary:: Fixed a formatting issue in the description.
Description:: Review Board's API has certain resources that require authentication.
But some resources like its root resource, the session resource, and
the server resource don't require any. However, there's a server
setting that lets you disable anonymous read access to the API. When
that is set, every request to the API must be authenticated.

RBTools assumes that the public API resources would always be public. We
weren't properly handling communication with private-API servers. This
has revealed itself in a few issues:

~

Web-based login caused a regression. When a command that requires the
API first initializes itself, it creates an API client and fetches the
root resource. That fetch triggers our auth handlers to prompt for
authentication, but the web login handler would break because its callback expects the root resource and API client to already be set.

~

Web-based login caused a regression. When a command that requires the
API first initializes itself, it creates an API client and fetches the
root resource. That fetch triggers our auth handlers to prompt for
authentication, but the web login handler would break because its
callback expects the root resource and API client to already be set.

If a user is logged out and does rbt logout, they'll be prompted to
login again.

If a user is logged out and does rbt login, they'll successfully
login but we'll print the "You're already logged in" message.

This change addresses those issues and makes sure that we properly
authenticate commands when interacting with private-API servers. We add
a has_session_cookie() method to the API client (and all the way down
to the server class) which returns whether we have a local session
cookie for the server. This information helps us deal with issues 2 and
3. We also update the login/logout commands to needs_api=False to
address issues 2 and 3.

The web login handler no longer assumes that a root resource is set. We
need the root to get the server info, in order to see whether web login
is supported on the server or not. In the case where we're working with a
private-API server while logged out, we'll just assume that the server
supports web login. Even if it doesn't, the auth will fail and we'll
move on to the username/password prompt handler.

Change Summary:: Added a note that the web login regression isn't public.
Description:: Review Board's API has certain resources that require authentication.
But some resources like its root resource, the session resource, and
the server resource don't require any. However, there's a server
setting that lets you disable anonymous read access to the API. When
that is set, every request to the API must be authenticated.

RBTools assumes that the public API resources would always be public. We
weren't properly handling communication with private-API servers. This
has revealed itself in a few issues:

~

Web-based login caused a regression. When a command that requires the
API first initializes itself, it creates an API client and fetches the
root resource. That fetch triggers our auth handlers to prompt for
authentication, but the web login handler would break because its
callback expects the root resource and API client to already be set.

~

Web-based login caused a regression. When a command that requires the
API first initializes itself, it creates an API client and fetches the
root resource. That fetch triggers our auth handlers to prompt for
authentication, but the web login handler would break because its
callback expects the root resource and API client to already be set.
This regression was never made public, it comes from recent
work that hasn't been released yet.

If a user is logged out and does rbt logout, they'll be prompted to
login again.

If a user is logged out and does rbt login, they'll successfully
login but we'll print the "You're already logged in" message.

This change addresses those issues and makes sure that we properly
authenticate commands when interacting with private-API servers. We add
a has_session_cookie() method to the API client (and all the way down
to the server class) which returns whether we have a local session
cookie for the server. This information helps us deal with issues 2 and
3. We also update the login/logout commands to needs_api=False to
address issues 2 and 3.

The web login handler no longer assumes that a root resource is set. We
need the root to get the server info, in order to see whether web login
is supported on the server or not. In the case where we're working with a
private-API server while logged out, we'll just assume that the server
supports web login. Even if it doesn't, the auth will fail and we'll
move on to the username/password prompt handler.

rbtools/api/request.py (Diff revision 1)

The issue has been resolved. Show all issues

Why do we need three different ways to fetch this? Can you add a comment explaining?

maubin Jan. 7, 2026, 10:30 p.m.

Oops, we don't need all three actually. Originally I was just doing req.headers.get('Cookie') which failed because urllib stores cookies in unredirected_hdrs so that they don't get leaked in redirects. I also thought req.get_header('Cookie') was failing to get the cookie but I was mistaken, its source code shows that it looks in both headers and unredirected_hdrs so I can just use that.

rbtools/commands/base/commands.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Typo: instantianting -> instantiating
```

rbtools/commands/login.py (Diff revision 1)

The issue has been dropped. Show all issues

Instead of this, can we update the type hint of get_authenticated_session to not be | None? According to my reading of the code, it always returns a value.

maubin Jan. 7, 2026, 8:19 p.m.

It can be None if auth_required=False and the session isn't authenticated. I'll add some type overloads to deal with that. I'll do it in an upcoming change where I fix up various typing things that got messed up, I think while merging in release-5.x.

Commits:

	Summary	ID
	Fix issues with authenticating to private-API servers. Review Board's API has certain resources that require authentication. But some resources like its root resource, the session resource, and the server resource don't require any. However, there's a server setting that lets you disable anonymous read access to the API. When that is set, every request to the API must be authenticated. RBTools assumes that the public API resources would always be public. We weren't properly handling communication with private-API servers. This has revealed itself in a few issues: 1. Web-based login caused a regression. When a command that requires the API first initializes itself, it creates an API client and fetches the root resource. That fetch triggers our auth handlers to prompt for authentication, but the web login handler would break because its callback expects the root resource and API client to already be set. 2. If a user is logged out and does `rbt logout`, they'll be prompted to login again. 3. If a user is logged out and does `rbt login`, they'll successfully login but we'll print the "You're already logged in" message. This change addresses those issues and makes sure that we properly authenticate commands when interacting with private-API servers. We add a `has_session_cookie()` method to the API client (and all the way down to the server class) which returns whether we have a local session cookie for the server. This information helps us deal with issues 2 and 3. We also update the login/logout commands to needs_api=False to address issues 2 and 3. The web login handler no longer assumes that a root resource is set. We need the root to get the server info, in order to see whether web login is supported on the server or not. In the case where we're working with a private-API server while logged out, we'll just assume that the server supports web login. Even if it doesn't, the auth will fail and we'll move on to the username/password prompt handler.	a5258e8021da6a0797d62f7148fce9d898422dce
	Fix issues with authenticating to private-API servers. Review Board's API has certain resources that require authentication. But some resources like its root resource, the session resource, and the server resource don't require any. However, there's a server setting that lets you disable anonymous read access to the API. When that is set, every request to the API must be authenticated. RBTools assumes that the public API resources would always be public. We weren't properly handling communication with private-API servers. This has revealed itself in a few issues: 1. Web-based login caused a regression. When a command that requires the API first initializes itself, it creates an API client and fetches the root resource. That fetch triggers our auth handlers to prompt for authentication, but the web login handler would break because its callback expects the root resource and API client to already be set. 2. If a user is logged out and does `rbt logout`, they'll be prompted to login again. 3. If a user is logged out and does `rbt login`, they'll successfully login but we'll print the "You're already logged in" message. This change addresses those issues and makes sure that we properly authenticate commands when interacting with private-API servers. We add a `has_session_cookie()` method to the API client (and all the way down to the server class) which returns whether we have a local session cookie for the server. This information helps us deal with issues 2 and 3. We also update the login/logout commands to needs_api=False to address issues 2 and 3. The web login handler no longer assumes that a root resource is set. We need the root to get the server info, in order to see whether web login is supported on the server or not. In the case where we're working with a private-API server while logged out, we'll just assume that the server supports web login. Even if it doesn't, the auth will fail and we'll move on to the username/password prompt handler.	547598e1918613117d0013ed4c2d51af05eb8aef

Diff:

Revision 2 (+958 -72)

Show changes

	rbtools/api/client.py
	rbtools/api/request.py
	rbtools/api/transport/__init__.py
	rbtools/api/transport/sync.py
	rbtools/commands/login.py
	rbtools/commands/logout.py
	rbtools/commands/base/commands.py
	rbtools/commands/tests/test_login.py
	4 more

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

rbtools/commands/login.py (Diff revision 2)

This will go away in a later change.

Ship it!

```
Ship It!
```

rbtools/api/request.py (Diff revision 2)

The issue has been resolved. Show all issues

This could produce a false-positive if a cookie's value has this set. Shouldn't happen, but I wonder if it'd make more sense to check the cookie jar?

maubin Jan. 8, 2026, 9:36 p.m.

I considered iterating over the cookies in the cookie jar, but then I'd have to deal with domain matching (like the logic that we have in CookiePolicy.domain_return_ok()) to make sure that the cookie applies to the given server. cookie_jar.add_cookie_header(req) will deal with that for me.

To deal with the false-positive, I could load the cookie header into a cookie so that it parses the header into the individual cookies that exist in it, and then see if there's an rbsessionid cookie.

rbtools/commands/base/commands.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Can you update this for the full module path?
```

rbtools/commands/login.py (Diff revision 2)

The issue has been dropped. Show all issues

Looks like we only use this in the session.authenticated case. Should we fetch it inline there?

maubin Jan. 8, 2026, 9:36 p.m.

We have to fetch it here before our first API call because it tells us whether we already have a session cookie before hitting the API. That'll tell us whether the user was already logged in or not, and then we use it to choose the right success message to print: "Successfully logged in ..." or "You are already logged in ...". I'll add a comment with explanation.

I know that the existence of the session cookie doesn't exactly mean the user is authenticated, because the cookie could be stale, but its what works best for the purposes of choosing a message. Its what deals with issue #3 in the description.

One downfall though is that in the case of a stale cookie the user will be prompted to login, and the message will print "You are already logged in". I think its a rare enough occurence to just leave that though. We could track the cookie value before the log in and then compare it to the cookie value after to see if its changed, but I don't think it’s worth the complexity.

rbtools/commands/logout.py (Diff revision 2)
The issue has been resolved. Show all issues
```
We can probably inline this here.
```
rbtools/commands/logout.py (Diff revision 2)
The issue has been resolved. Show all issues
```
This can be updated to use the local api_client.
```

rbtools/testing/commands.py (Diff revision 2)

The issue has been resolved. Show all issues

So this info doesn't get lost later, can we document why we're checking against these classes explicitly?

Commits:

	Summary	ID
	Fix issues with authenticating to private-API servers. Review Board's API has certain resources that require authentication. But some resources like its root resource, the session resource, and the server resource don't require any. However, there's a server setting that lets you disable anonymous read access to the API. When that is set, every request to the API must be authenticated. RBTools assumes that the public API resources would always be public. We weren't properly handling communication with private-API servers. This has revealed itself in a few issues: 1. Web-based login caused a regression. When a command that requires the API first initializes itself, it creates an API client and fetches the root resource. That fetch triggers our auth handlers to prompt for authentication, but the web login handler would break because its callback expects the root resource and API client to already be set. 2. If a user is logged out and does `rbt logout`, they'll be prompted to login again. 3. If a user is logged out and does `rbt login`, they'll successfully login but we'll print the "You're already logged in" message. This change addresses those issues and makes sure that we properly authenticate commands when interacting with private-API servers. We add a `has_session_cookie()` method to the API client (and all the way down to the server class) which returns whether we have a local session cookie for the server. This information helps us deal with issues 2 and 3. We also update the login/logout commands to needs_api=False to address issues 2 and 3. The web login handler no longer assumes that a root resource is set. We need the root to get the server info, in order to see whether web login is supported on the server or not. In the case where we're working with a private-API server while logged out, we'll just assume that the server supports web login. Even if it doesn't, the auth will fail and we'll move on to the username/password prompt handler.	547598e1918613117d0013ed4c2d51af05eb8aef
	Fix issues with authenticating to private-API servers. Review Board's API has certain resources that require authentication. But some resources like its root resource, the session resource, and the server resource don't require any. However, there's a server setting that lets you disable anonymous read access to the API. When that is set, every request to the API must be authenticated. RBTools assumes that the public API resources would always be public. We weren't properly handling communication with private-API servers. This has revealed itself in a few issues: 1. Web-based login caused a regression. When a command that requires the API first initializes itself, it creates an API client and fetches the root resource. That fetch triggers our auth handlers to prompt for authentication, but the web login handler would break because its callback expects the root resource and API client to already be set. 2. If a user is logged out and does `rbt logout`, they'll be prompted to login again. 3. If a user is logged out and does `rbt login`, they'll successfully login but we'll print the "You're already logged in" message. This change addresses those issues and makes sure that we properly authenticate commands when interacting with private-API servers. We add a `has_session_cookie()` method to the API client (and all the way down to the server class) which returns whether we have a local session cookie for the server. This information helps us deal with issues 2 and 3. We also update the login/logout commands to needs_api=False to address issues 2 and 3. The web login handler no longer assumes that a root resource is set. We need the root to get the server info, in order to see whether web login is supported on the server or not. In the case where we're working with a private-API server while logged out, we'll just assume that the server supports web login. Even if it doesn't, the auth will fail and we'll move on to the username/password prompt handler.	747f1a1e7c397562e9bd2173c6ef17343cae70a2

Diff:

Revision 3 (+986 -76)

Show changes

	rbtools/api/client.py
	rbtools/api/request.py
	rbtools/api/transport/__init__.py
	rbtools/api/transport/sync.py
	rbtools/commands/login.py
	rbtools/commands/logout.py
	rbtools/commands/base/commands.py
	rbtools/commands/tests/test_login.py
	4 more

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Ship it!

```
Ship It!
```

Status:: Completed
Change Summary:: Pushed to master (e29fb76)

		Review Board's API has certain resources that require authentication.
		But some resources like its root resource, the session resource, and
		the server resource don't require any. However, there's a server
		setting that lets you disable anonymous read access to the API. When
		that is set, every request to the API must be authenticated.

		RBTools assumes that the public API resources would always be public. We
		weren't properly handling communication with private-API servers. This
		has revealed itself in a few issues:

~		Web-based login caused a regression. When a command that requires the API first initializes itself, it creates an API client and fetches the root resource. That fetch triggers our auth handlers to prompt for authentication, but the web login handler would break because its callback expects the root resource and API client to already be set.
	~	Web-based login caused a regression. When a command that requires the API first initializes itself, it creates an API client and fetches the root resource. That fetch triggers our auth handlers to prompt for authentication, but the web login handler would break because its callback expects the root resource and API client to already be set.
		If a user is logged out and does `rbt logout`, they'll be prompted to login again.
		If a user is logged out and does `rbt login`, they'll successfully login but we'll print the "You're already logged in" message.

		This change addresses those issues and makes sure that we properly
		authenticate commands when interacting with private-API servers. We add
		a `has_session_cookie()` method to the API client (and all the way down
		to the server class) which returns whether we have a local session
		cookie for the server. This information helps us deal with issues 2 and
		3. We also update the login/logout commands to needs_api=False to
		address issues 2 and 3.

		The web login handler no longer assumes that a root resource is set. We
		need the root to get the server info, in order to see whether web login
		is supported on the server or not. In the case where we're working with a
		private-API server while logged out, we'll just assume that the server
		supports web login. Even if it doesn't, the auth will fail and we'll
		move on to the username/password prompt handler.