Skip to content

Commit

Permalink
Improve private scenes error pages (#40)
Browse files Browse the repository at this point in the history
* Improve 401 and 404 errors for private scenes (fix #39 )

* fix share link to scenes

* try to only set the www-authenticate header on non-interactive sessions
  • Loading branch information
sdumetz committed Jan 3, 2024
1 parent 375e76e commit 84d8af3
Show file tree
Hide file tree
Showing 8 changed files with 76 additions and 21 deletions.
4 changes: 2 additions & 2 deletions source/server/auth/UserManager.ts
Original file line number Diff line number Diff line change
Expand Up @@ -297,12 +297,12 @@ export default class UserManager {
return (await this.db.get(`
SELECT COALESCE(
json_extract(access, '$.' || $uid),
${0 < uid? `json_extract(access, '$.1'),`:""}
${((0 < uid) ? `json_extract(access, '$.1'),`:"")}
json_extract(access, '$.0')
) AS access
FROM scenes
WHERE scene_name = $scene
`, {$scene:scene, $uid:uid.toString(10)}))?.access;
`, {$scene:scene, $uid:uid.toString(10)}))?.access ?? null;
}

/**
Expand Down
6 changes: 3 additions & 3 deletions source/server/integration.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ describe("Web Server Integration", function(){
await request(this.server).get("/scenes/foo/models/foo.glb").expect(200);
});
it("can't create a model", async function(){
await request(this.server).put("/scenes/bar/models/bar.glb").expect(401);
await request(this.server).put("/scenes/foo/models/bar.glb").expect(401);
});
it("can't fetch user list", async function(){
await request(this.server).get("/api/v1/users")
Expand Down Expand Up @@ -191,8 +191,8 @@ describe("Web Server Integration", function(){
.set("Accept", "")
.expect(200);
});
it("can't edit other people's models", async function(){
await this.agent.put("/scenes/foo/models/foo.glb").send("foo\n").expect(401);
it("can't edit other people's models (obfuscated as 404)", async function(){
await this.agent.put("/scenes/foo/models/foo.glb").send("foo\n").expect(404);
});
it.skip("can be granted permissions", async function(){

Expand Down
4 changes: 2 additions & 2 deletions source/server/routes/api/v1/scenes/scene/get.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -36,10 +36,10 @@ describe("GET /api/v1/scenes/:scene", function(){
.expect("Content-Type", "application/json; charset=utf-8");
});

it("is access-protected", async function(){
it("is access-protected (obfuscated as 404)", async function(){
await userManager.grant("foo", "default", "none");
await request(this.server).get("/api/v1/scenes/foo")
.expect(401);
.expect(404);
});
});

Expand Down
4 changes: 2 additions & 2 deletions source/server/routes/scenes/get/file.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -37,13 +37,13 @@ describe("GET /scenes/:scene/:filename(.*)", function(){
.expect("foo\n");
});

it("can't get a private scene's file", async function(){
it("can't get a private scene's file (obfuscated as 404)", async function(){
let scene_id = await vfs.createScene("foo", {"0":"none", [user.uid]: "admin"});
await vfs.writeDoc("{}", scene_id, user.uid);
await vfs.writeFile(dataStream(), {scene: "foo", mime:"model/gltf-binary", name: "models/foo.glb", user_id: user.uid});

await request(this.server).get("/scenes/foo/models/foo.glb")
.expect(401);
.expect(404);
});

it("can get an owned scene's file", async function(){
Expand Down
5 changes: 4 additions & 1 deletion source/server/server.ts
Original file line number Diff line number Diff line change
Expand Up @@ -205,7 +205,10 @@ export default async function createServer(config = defaultConfig) :Promise<expr
}
let code = (error instanceof HTTPError )? error.code : 500;

if(code === 401){
if(code === 401 && !(req.get("Accept")?.startsWith("text/html") && req.get("User-Agent")?.startsWith("Mozilla"))){
//We try to NOT send the header for browser requests because we prefer the HTML login to the browser's popup
//Browser tends to prefer text/html and always send Mozilla/5.0 at the beginning of their user-agent
//If someone has customized their headers, they'll get the ugly popup and live with it.
res.set("WWW-Authenticate", "Basic realm=\"authenticated access\"");
}

Expand Down
45 changes: 41 additions & 4 deletions source/server/templates/error.hbs
Original file line number Diff line number Diff line change
@@ -1,8 +1,45 @@
<corpus-navbar>
<nav-link href="/ui/scenes/" class="nav-link">Collection</nav-link>
<nav-link href="https://ethesaurus.holusion.com" class="nav-link">Documentation</nav-link>

<div class="divider"></div>
<user-button class="nav-link">Connexion</user-button>
</corpus-navbar>
<main class="error-main">
{{#with error}}
<h1>Error :{{code}}</h1>
<h1 class="error-title">Error :{{code}}</h1>

<p>{{message}}</p>
<p class="message">{{message}}</p>

<pre><code>{{stack}}</code></pre>
<pre class="stack"><code>{{stack}}</code></pre>

{{/with}}
{{/with}}
</main>
<modal-dialog></modal-dialog>

<style>
main.error-main{
color:var(--color-light);
margin: auto;
max-width:1200px;
min-height: 60vh;
display:flex;
align-items: center;
flex-direction: column;
justify-content: center;
}
main.error-main .error-title{
color:var(--color-secondary);
}
main.error-main .message{
font-weight: bold;
}
main.error-main .stack{
background:var(--color-medium);
max-width:82ch;
padding: 1ch;
}
</style>

<script src="/js/corpus.js"></script>
19 changes: 14 additions & 5 deletions source/server/utils/locals.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ import {basename, dirname} from "path";
import User, { SafeUser } from "../auth/User";
import UserManager, { AccessType, AccessTypes } from "../auth/UserManager";
import Vfs, { GetFileParams } from "../vfs";
import { BadRequestError, ForbiddenError, HTTPError, InternalError, UnauthorizedError } from "./errors";
import { BadRequestError, ForbiddenError, HTTPError, InternalError, NotFoundError, UnauthorizedError } from "./errors";

export interface AppLocals extends Record<string, any>{
port :number;
Expand Down Expand Up @@ -72,11 +72,20 @@ function _perms(check:number,req :Request, res :Response, next :NextFunction){
if(isAdministrator) return next();

let userManager = getUserManager(req);
(res.locals.access? Promise.resolve(res.locals.access):
userManager.getAccessRights(scene, uid).then(a=>{ res.locals.access = a; return a })
(res.locals.access?
Promise.resolve(res.locals.access) :
userManager.getAccessRights(scene, uid)
).then( access => {
if(check <= AccessTypes.indexOf(access)) next();
else next(new UnauthorizedError(`user does not have ${AccessTypes[check]} rights on ${scene}`));
res.locals.access = access;
const lvl = AccessTypes.indexOf(access);
if(check <= lvl){
next();
} else if(req.method === "GET" || lvl <= AccessTypes.indexOf("none")){
next(new NotFoundError(`Can't find scene ${scene}. It may be private or not exist entirely.`))
} else {
//User has insuficient level but can read the scene
next(new UnauthorizedError(`user does not have ${AccessTypes[check]} rights on ${scene}`));
}
}, next);
}

Expand Down
10 changes: 8 additions & 2 deletions source/ui/state/router.ts
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,7 @@ export function router<T extends Constructor<LitElement>>(baseClass:T) : T & Con

if(!this.inPath(url.pathname) && this.path != "/") return; //Return to bubble up the stack
ev.stopPropagation(); //Handle the route change
ev.preventDefault();

if(!url.pathname.endsWith("/")) url.pathname += "/";
window.history.pushState({},"", url);
Expand Down Expand Up @@ -129,12 +130,17 @@ export function navigate(that :HTMLElement,href ?:string|URL, queries?:Record<st
else url.searchParams.set(key, value.toString());
}
}
console.debug("Navigate to :", url.toString(), "with queries : ", queries);
(that ?? this).dispatchEvent(new CustomEvent("navigate", {
const unhandled = (that ?? this).dispatchEvent(new CustomEvent("navigate", {
detail: {href: url},
bubbles: true,
composed: true
}));

if(unhandled){
window.location.href = url.toString();
}else{
console.debug("Navigate to :", url.toString(), "with queries : ", queries);
}
}

export declare class Link{
Expand Down

0 comments on commit 84d8af3

Please sign in to comment.