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

Correctly conform JWTError to AbortError #162

Merged
merged 8 commits into from
Nov 16, 2024
Merged

Correctly conform JWTError to AbortError #162

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
2cf91a0
Update SIWA test with new token and testing the body so we know when …
0xTim Nov 3, 2024
c0e1f99
Wrap JWTError for conformance
0xTim Nov 5, 2024
ddd1269
Wrap more errors
0xTim Nov 5, 2024
b8ebf37
Better workaround
0xTim Nov 5, 2024
f1ed0bf
Remove unused import
0xTim Nov 5, 2024
006b571
Fix lint error
0xTim Nov 5, 2024
e81248d
Do it 'correctly'
0xTim Nov 13, 2024
a0fcf8d
Fix linting
0xTim Nov 13, 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
1 change: 1 addition & 0 deletions .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,3 +7,4 @@ Package.resolved
DerivedData
.swiftpm
Tests/LinuxMain.swift
.vscode/
4 changes: 4 additions & 0 deletions Sources/JWT/JWTError+Vapor.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,10 @@
import Vapor

extension JWTError: @retroactive AbortError {
public var reason: String {
Copy link
Member Author

Choose a reason for hiding this comment

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

@gwynne can you see any issues with this given JWTError already has var reason: String? defined? Tbh I'm surprised this actually worked but it seems to and I can't get the compiler to complain about ambiguous types no matter what I try

Copy link
Member

Choose a reason for hiding this comment

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

I think it's a bad idea even if it seems to work, but I don't have an alternate suggestion for having AbortError conformance.

Copy link
Contributor

Choose a reason for hiding this comment

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

Depends what is calling reason. If it thinks the instance is an AbortError, this will be called, otherwise if it thinks it's a JWTError, then the other one will be called.

Copy link
Member Author

Choose a reason for hiding this comment

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

As long at the compiler doesn't see both and get confused we should be fine. The alternative is to wrap the error in our own type and conform that to AbortError - not as nice by far (but then we are only conforming types we own to a protocol rather than types we don't own to protocols we don't own)

self.description
}

public var status: HTTPResponseStatus {
.unauthorized
}
Expand Down
4 changes: 3 additions & 1 deletion Tests/JWTTests/JWTTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -262,15 +262,17 @@ struct JWTTests {
var headers = HTTPHeaders()
headers.bearerAuthorization = .init(
token: """
eyJraWQiOiJmaDZCczhDIiwiYWxnIjoiUlMyNTYifQ.eyJpc3MiOiJodHRwczovL2FwcGxlaWQuYXBwbGUuY29tIiwiYXVkIjoiZGV2LnRpbWMuc2l3YS1kZW1vLlRJTGlPUyIsImV4cCI6MTcwODUxNTY3NiwiaWF0IjoxNzA4NDI5Mjc2LCJzdWIiOiIwMDE1NDIuYjA0MTAwYzUxYWNiNDhkM2E1NzA2ODRmMTdkNjM5NGQuMTYwMyIsImNfaGFzaCI6ImFxQjM1RXR1bWFtVUg0VjZBYklmaXciLCJlbWFpbCI6Ijh5c2JjaHZjMm1AcHJpdmF0ZXJlbGF5LmFwcGxlaWQuY29tIiwiZW1haWxfdmVyaWZpZWQiOnRydWUsImlzX3ByaXZhdGVfZW1haWwiOnRydWUsImF1dGhfdGltZSI6MTcwODQyOTI3Niwibm9uY2Vfc3VwcG9ydGVkIjp0cnVlLCJyZWFsX3VzZXJfc3RhdHVzIjoyfQ.E4SmBvvsr-L1f4rbwoXIg23XJEdA6WQxLfT6Z0TaFRTNbufuUtvG41MwJvf62T3HdCsY1VXlhdVYmTNbzqCuax6CUObue2ndx6osInDzfTkzysx17eUeCaG1XCfq9mScuVgW8xh3ZPfIeQdsII-MnP8ZG7q-CAxf6soSza_BKrrw4TArvEXrjbZO7FI1U2K72JtVZ118wcuEWfv8JO-FWFOHgWzJujqxI_7ayVG-mQfZitmYXv5ws-stZMxA0RvIbuYLWAksI6-ehYEgeEQa6NzzcJNWm3oArB0ithQE59fqFDoKCwpLchBMANz3tmNpN194Rc4ppL-niIDWFE-0Ug
eyJraWQiOiJGZnRPTlR4b0VnIiwiYWxnIjoiUlMyNTYifQ.eyJpc3MiOiJodHRwczovL2FwcGxlaWQuYXBwbGUuY29tIiwiYXVkIjoiZGV2LnRpbWMuc2l3YS1kZW1vLlRJTGlPUyIsImV4cCI6MTczMDc2MDAxOCwiaWF0IjoxNzMwNjczNjE4LCJzdWIiOiIwMDE1NDIuYjA0MTAwYzUxYWNiNDhkM2E1NzA2ODRmMTdkNjM5NGQuMTYwMyIsImNfaGFzaCI6IlUxc1d0Z1dfWTZSb3d1a09WRzJmNEEiLCJlbWFpbCI6Ijh5c2JjaHZjMm1AcHJpdmF0ZXJlbGF5LmFwcGxlaWQuY29tIiwiZW1haWxfdmVyaWZpZWQiOnRydWUsImlzX3ByaXZhdGVfZW1haWwiOnRydWUsImF1dGhfdGltZSI6MTczMDY3MzYxOCwibm9uY2Vfc3VwcG9ydGVkIjp0cnVlfQ.fb-e48W_zMGfT0LqciYnBUBy7KxVaV5JC5VV4HFhhpK8yz0AUxeYHmXpkvt1gLPNnjd3c-fzMS0hUR-NiffgYuNs3qSFXSenb4BwYdDIuMXElggUPX3j6HU2TV-JYsTFl4tZpgnFs_0_56pscaJzQONCdrZdKJiD0lmtum7D-doH43aKflV-pAMXSZTCli9HwRNeZikmbY6wBS5Ltg4VI5Z8Usge4eS2HINdHIPSCadYf858pZ8huAaj5Jm4t_5j988khwgqBMc9haTHZgiUpK7SZDePuRsAAQQVCXnRsuibxFX66ugo5BEEKCdK-xg66iAstb_mC_628gMrybC-_w
""")

try await app.test(.GET, "test", headers: headers) { res async in
#expect(res.status == .unauthorized)
#expect(res.body.string.contains("expired"))
ptoffy marked this conversation as resolved.
Show resolved Hide resolved
}

try await app.test(.GET, "test2", headers: headers) { res async in
#expect(res.status == .unauthorized)
#expect(res.body.string.contains("expired"))
}
}
}
Expand Down