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.

Sign up for GitHub

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

feat: support OIDC login #127

Merged
merged 2 commits into from
Jun 13, 2024
Merged

feat: support OIDC login #127

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4089e88
feat: support OIDC login
huwshimi Jun 7, 2024
696a721
fix: allow not passing credentials for OIDC login.
huwshimi Jun 11, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
49 changes: 34 additions & 15 deletions api/client.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ import {
GenericFacade,
} from "./types.js";
import { createAsyncHandler, toError } from "./utils.js";
import AdminV4 from "./custom-facades/AdminV4.js";

export const CLIENT_VERSION = "3.3.2";

Expand All @@ -44,6 +45,7 @@ export interface ConnectOptions {
closeCallback: CloseCallback;
debug?: boolean;
facades?: (ClassType<Facade> | GenericFacade)[];
oidcEnabled?: boolean;
onWSCreated?: (ws: WebSocket) => void;
wsclass?: typeof WebSocket;
}
Expand All @@ -56,11 +58,16 @@ export interface ConnectionInfo {
getFacade?: (name: string) => Facade;
}

export interface Credentials {
username?: string;
password?: string;
macaroons?: MacaroonObject[][];
}
export type ExcludeProps<O> = Partial<Record<keyof O, never>>;

export type Credentials =
| {
username: string;
password: string;
}
| {
macaroons: MacaroonObject[][];
};

// The type of a Macaroon from the Admin facade does not match a real macaroon.
const isMacaroonObject = (
Expand Down Expand Up @@ -171,8 +178,8 @@ function connect(
*/
async function connectAndLogin(
url: string,
credentials: Credentials,
options: ConnectOptions,
credentials?: Credentials,
clientVersion = CLIENT_VERSION
): Promise<{
conn?: Connection;
Expand Down Expand Up @@ -206,8 +213,8 @@ async function connectAndLogin(
};
return await connectAndLogin(
generateURL(url, srv),
credentials,
options,
credentials,
clientVersion
);
}
Expand Down Expand Up @@ -240,7 +247,8 @@ class Client {
_transport: Transport;
_bakery?: Bakery | null;
_facades: (ClassType<Facade> | GenericFacade)[];
_admin: AdminV3;
_admin: AdminV3 | AdminV4;
_oidcEnabled: boolean;

constructor(ws: WebSocket, options: ConnectOptions) {
// Instantiate the transport, used for sending messages to the server.
Expand All @@ -250,9 +258,13 @@ class Client {
Boolean(options.debug)
);

this._oidcEnabled = options.oidcEnabled || false;
this._facades = options.facades || [];
this._bakery = options.bakery;
this._admin = new AdminV3(this._transport, {});
this._admin = new (this._oidcEnabled ? AdminV4 : AdminV3)(
this._transport,
{}
);
}

/**
Expand All @@ -269,7 +281,7 @@ class Client {
promise will not be resolved or rejected if a callback is provided.
*/
async login(
credentials: Credentials,
credentials?: Credentials,
Copy link
Contributor

Choose a reason for hiding this comment

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

Was wondering if putting the optional param in login and connectAndLogin at the end of the params list might be a better practice? This way, when not available, we would just not pass the param at the end instead of passing it as undefined in the middle.

Copy link
Member Author

Choose a reason for hiding this comment

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

In both cases clientVersion is also optional, but with a default value (in TS you can't mark a param as optional and provide a default value as the variable will always be initialised). Incidentally, having an optional param before a non-optional gives a TS error.

Also, if we were to swap the order it would mean we would always have to provide the client version when we wanted to pass credentials, which would mean we wouldn't get the benefit of having a default value.

Copy link
Contributor

Choose a reason for hiding this comment

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

Makes sense. I didn't spot that clientVersion is provided a default value.

clientVersion = CLIENT_VERSION
): Promise<Connection | undefined> {
const args: LoginRequest = {
Expand All @@ -283,10 +295,10 @@ class Client {
const url = this._transport._ws.url;
const origin = url;

if (credentials.username && credentials.password) {
if (credentials && "username" in credentials) {
args.credentials = credentials.password;
args["auth-tag"] = `user-${credentials.username}`;
} else {
} else if (credentials && "macaroons" in credentials) {
const macaroons = this._bakery?.storage.get(origin);
let deserialized;
if (macaroons) {
Expand All @@ -300,7 +312,10 @@ class Client {
let response: LoginResult | null = null;
try {
try {
response = await this._admin.login(args);
response =
this._oidcEnabled && "loginWithSessionCookie" in this._admin
? await this._admin.loginWithSessionCookie()
: await this._admin.login(args);
} catch (error) {
if (
error instanceof Error &&
Expand Down Expand Up @@ -337,8 +352,12 @@ class Client {
const serialized = btoa(JSON.stringify(macaroons));
this._bakery?.storage.set(origin, serialized, () => {});
// Send the login request again including the discharge macaroons.
credentials.macaroons = [macaroons];
return resolve(this.login(credentials, clientVersion));
return resolve(
this.login(
{ ...credentials, macaroons: [macaroons] },
clientVersion
)
);
};
const onFailure = (err: string | MacaroonError) => {
reject(
Expand Down
47 changes: 47 additions & 0 deletions api/custom-facades/AdminV4.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
/**
Juju Admin version 4.
This facade is available on:
Controller-machine-agent
Machine-agent
Unit-agent
Controllers
Models
*/

import type { JujuRequest } from "../../generator/interfaces.js";
import { ConnectionInfo, Transport } from "../client.js";
import AdminV3, { LoginResult } from "../facades/admin/AdminV3.js";

/**
admin is the only object that unlogged-in clients can access. It holds any
methods that are needed to log in.
*/
class AdminV4 extends AdminV3 {
static NAME = "Admin";
static VERSION = 4;

NAME = "Admin";
VERSION = 4;

constructor(transport: Transport, info: ConnectionInfo) {
super(transport, info);
}
/**
LoginWithSessionCookie logs in if the session cookie exists when the
websocket connection is made. All subsequent requests on the
connection will act as the authenticated user.
*/
loginWithSessionCookie(): Promise<LoginResult> {
return new Promise((resolve, reject) => {
const req: JujuRequest = {
type: "Admin",
request: "LoginWithSessionCookie",
version: 4,
};

this._transport.write(req, resolve, reject);
});
}
}

export default AdminV4;
73 changes: 61 additions & 12 deletions api/tests/test-client.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -163,7 +163,7 @@ describe("connect", () => {
it("login redirection error failure via promise", (done) => {
connect("wss://1.2.3.4", options).then((juju: Client) => {
juju
?.login({})
?.login({ macaroons: [] })
.then(() => fail)
.catch((error) => {
validateRedirectionLoginFailure(error);
Expand Down Expand Up @@ -206,7 +206,7 @@ describe("connect", () => {
it("login generic redirection error failure via promise", (done) => {
connect("wss://1.2.3.4", options).then((juju: Client) => {
juju
?.login({})
?.login({ macaroons: [] })
.then(() => fail)
.catch((error) => {
expect(error).toStrictEqual(new Error("bad wolf"));
Expand Down Expand Up @@ -253,7 +253,7 @@ describe("connect", () => {
expect(err).toBe(null);
expect(juju).not.toBe(null);
juju
?.login({})
?.login({ macaroons: [] })
.then(() => fail)
.catch((error) => {
validateRedirectionLoginSuccess(juju, error);
Expand Down Expand Up @@ -298,7 +298,7 @@ describe("connect", () => {
it("login redirection error success via promises", (done) => {
connect("wss://1.2.3.4", options).then((juju: Client) => {
juju
?.login({})
?.login({ macaroons: [] })
.then(() => fail)
.catch((error) => {
validateRedirectionLoginSuccess(juju, error);
Expand Down Expand Up @@ -534,6 +534,31 @@ describe("connect", () => {
ws.open();
});

it("connect and enable OIDC login", (done) => {
connect(
"wss://1.2.3.4",
{
...options,
oidcEnabled: true,
},
(err?: CallbackError, juju?: Client) => {
expect(err).toBe(null);
juju?.login().then(() => {
requestEqual(ws.lastRequest, {
type: "Admin",
request: "LoginWithSessionCookie",
version: 4,
});
done();
});
// Reply to the login request.
ws.reply({ response: {} });
}
);
// Open the WebSocket connection.
ws.open();
});

it("connection transport success", (done) => {
const options = { closeCallback: jest.fn() };
makeConnection(options, (conn, ws) => {
Expand Down Expand Up @@ -644,8 +669,8 @@ describe("connectAndLogin", () => {
};

it("connect failure", (done) => {
const creds = {};
connectAndLogin(url, creds, options).catch((error) => {
const creds = { macaroons: [] };
connectAndLogin(url, options, creds).catch((error) => {
expect(error).toStrictEqual(
new Error("cannot connect WebSocket: bad wolf")
);
Expand All @@ -656,8 +681,8 @@ describe("connectAndLogin", () => {
});

it("login redirection error failure", (done) => {
const creds = { user: "who", password: "tardis" };
connectAndLogin(url, creds, options)
const creds = { username: "who", password: "tardis" };
connectAndLogin(url, options, creds)
.then(() => fail)
.catch((error) => {
expect(error.message).toBe("cannot connect to model after redirection");
Expand All @@ -684,7 +709,7 @@ describe("connectAndLogin", () => {
it("login redirection error success", (done) => {
// If this test is timing out then check that the setTimeout is opening the
// model websocket.
const creds = { user: "who", password: "tardis" };
const creds = { username: "who", password: "tardis" };
let modelWS: MockWebSocket;
const options = {
bakery: makeBakery(true),
Expand Down Expand Up @@ -748,7 +773,7 @@ describe("connectAndLogin", () => {
}
},
};
connectAndLogin(url, creds, options).then(
connectAndLogin(url, options, creds).then(
(result?: { conn?: Connection; logout: Client["logout"] }) => {
expect(result).not.toBe(null);
expect(result?.conn).not.toBe(null);
Expand All @@ -762,8 +787,8 @@ describe("connectAndLogin", () => {
});

it("login success", (done) => {
const creds = { user: "who", password: "tardis" };
connectAndLogin(url, creds, options).then((result: any) => {
const creds = { username: "who", password: "tardis" };
connectAndLogin(url, options, creds).then((result: any) => {
expect(result).not.toBe(null);
expect(result.conn).not.toBe(null);
expect(result.logout).not.toBe(null);
Expand All @@ -781,6 +806,30 @@ describe("connectAndLogin", () => {
// Open the WebSocket connection.
ws.open();
});

it("login success", (done) => {
connectAndLogin(url, { ...options, oidcEnabled: true }).then(
(result: any) => {
result.logout();
// The WebSocket is now closed.
expect(ws.readyState).toBe(3);
requestEqual(ws.lastRequest, {
type: "Admin",
request: "LoginWithSessionCookie",
version: 4,
});
done();
}
);
ws.queueResponses(
new Map([
// Reply to the login request.
[1, { response: { facades: [] } }],
])
);
// Open the WebSocket connection.
ws.open();
});
});

describe("generateModelURL", () => {
Expand Down
Loading