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

JWT authentication & Clean up the board #23

Merged
merged 29 commits into from
Jul 1, 2024
Merged

JWT authentication & Clean up the board #23

merged 29 commits into from
Jul 1, 2024

Conversation

hweihwang
Copy link
Contributor

@hweihwang hweihwang commented Jun 7, 2024

  • See actual Nextcloud user on the board (ongoing)
  • Expose read only permission through token


$userId = $user->getUID();

$key = "your_secret_key";
Copy link
Member

Choose a reason for hiding this comment

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

Could we make this configurable in the app config right away? And for the websocket_server configurable as environment variable?

$issuedAt = time();
$expirationTime = $issuedAt + 3600;
$payload = [
'userid' => $userId,
Copy link
Member

Choose a reason for hiding this comment

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

We should have the file id and probably permissions in the token payload right away so its use can be limited to that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

User can interact with multiple files at the same time, so we will have different short-lived tokens for each file. What are your thoughts on this approach? Here are two considerations:

  • We can have one short/medium-lived token representing the user (not tied to any specific file) and perform the check/kick whenever retrieving or saving content to a specific file.

  • We can use short-lived tokens for each file with the necessary permissions. These permissions can become outdated when user's permissions changed, but if they are short-lived, it shouldn't be a huge problem. But still need to check whenever retrieving or saving
    @juliushaertl

Copy link
Member

Choose a reason for hiding this comment

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

I'd still prefer file based short-lived tokens. This will make it easier to further extend for cases where there is no user in scope as for public share links of files. Generally I'd also prefer to give the least possible scope to any tokens.

Signed-off-by: Hoang Pham <[email protected]>
Signed-off-by: Hoang Pham <[email protected]>
Signed-off-by: Hoang Pham <[email protected]>
Signed-off-by: Hoang Pham <[email protected]>
Signed-off-by: Hoang Pham <[email protected]>
@juliusknorr
Copy link
Member

Very nice, could you check the Ci failures? Then we can merge 👍

@juliusknorr
Copy link
Member

PHPUnit tests seem caused because we have not tests and no test config yet.

Signed-off-by: Hoang Pham <[email protected]>
Signed-off-by: Hoang Pham <[email protected]>
Signed-off-by: Hoang Pham <[email protected]>
Signed-off-by: Hoang Pham <[email protected]>
Signed-off-by: Hoang Pham <[email protected]>
Signed-off-by: Hoang Pham <[email protected]>
Signed-off-by: Hoang Pham <[email protected]>
Comment on lines 34 to 36
private readonly IUserSession $userSession,
private readonly IRootFolder $rootFolder,
private readonly IConfig $config
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
private readonly IUserSession $userSession,
private readonly IRootFolder $rootFolder,
private readonly IConfig $config
private IUserSession $userSession,
private IRootFolder $rootFolder,
private IConfig $config

We still need to support PHP 8.0 for now

Comment on lines 36 to 40
IRequest $request,
private readonly IUserSession $userSession,
private readonly IConfig $config,
/** @psalm-suppress MissingDependency */
private readonly IRootFolder $rootFolder
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
IRequest $request,
private readonly IUserSession $userSession,
private readonly IConfig $config,
/** @psalm-suppress MissingDependency */
private readonly IRootFolder $rootFolder
IRequest $request,
private IUserSession $userSession,
private IConfig $config,
/** @psalm-suppress MissingDependency */
private IRootFolder $rootFolder

@juliusknorr
Copy link
Member

Thanks @hweihwang

I left two more comments as we need to keep supporting PHP 8.0 for now. Can you also squash the commits either logically or into one final one so we have a clean history?

I noticed when attempting to rebase the PR (I didn't have much time) that there were quite some merge commits that make future rebasing harder. In the future it might be better to only use rebasing to catch up with changes from the base branch.

Signed-off-by: Hoang Pham <[email protected]>
@juliusknorr juliusknorr merged commit 04dc6de into main Jul 1, 2024
20 of 22 checks passed
@jancborchardt jancborchardt deleted the feat/auth branch July 8, 2024 12:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

2 participants