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

Add search 'by-path' url for browser #5535

Merged
merged 1 commit into from
Oct 17, 2020
Merged

Add search 'by-path' url for browser #5535

merged 1 commit into from
Oct 17, 2020

Conversation

8-bit-fox
Copy link

@8-bit-fox 8-bit-fox commented Oct 10, 2020

This PR adds the option to use the URL scheme to search for an entry by path. Hence, tools implementing the keepassxc-browser API can use a credential's path to request a specific credential.

The code currently reuses the strategy implemented by @piegamesde in #4763.

Testing strategy

  • Unit tests.

Type of change

  • ✅ New feature (change that adds functionality)

@8-bit-fox
Copy link
Author

As the path is known, I think one should avoid the search. What do you think @piegamesde ?

Copy link
Contributor

@piegamesde piegamesde left a comment

Choose a reason for hiding this comment

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

Wow, that was quick! I don't see any problems, but also I'm not really familiar with the code base of keepassxc

}
return handleURL(entry->url(), url, submitUrl);
}

QString BrowserService::getPath(Entry* entry) const
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm really not sure, but could this method be added as method of Entry instead? It feels like it would be better to put it there.

@droidmonkey
Copy link
Member

droidmonkey commented Oct 10, 2020

I would rather we implement the proposed reference syntax from #5369. This would apply to the by-uuid previous merge implementation as well. Obviously this url reference syntax would be implemented globally, not just in the browser service.

@8-bit-fox
Copy link
Author

8-bit-fox commented Oct 11, 2020

Yes, you are right.In case that the proposal is accepted, I can adapt my change proposal accordingly, as well as refactor the by-uuid syntax. The question would be, if you already require the full implementation of the syntax? E.g. retrieving history items etc via the browser interface.

@droidmonkey
Copy link
Member

We can merge this, once fixed up, for now and easily replace it with the new syntax.

src/core/Entry.cpp Outdated Show resolved Hide resolved
@8-bit-fox 8-bit-fox marked this pull request as ready for review October 11, 2020 13:27
@8-bit-fox
Copy link
Author

As far as I am concerned, this PR is ready for review.

@8-bit-fox
Copy link
Author

@droidmonkey, @piegamesde Do I need to add something here?

@piegamesde
Copy link
Contributor

The changes are fine for me, but I can't judge how this interacts with #5369.

@8-bit-fox
Copy link
Author

The changes are fine for me, but I can't judge how this interacts with #5369.

Well, once #5369 is implemented, we would need to adjust the URL here. See #5535 (comment)

Of course, IMO, the implementation of #5369 would imply the implementation of a generic search handler such that the bells-and-whistles for by-uuid and by-path are unnecessary. The strategy to introduce these new features must be decided by the maintainers - I would go in incremental steps ;)

@droidmonkey
Copy link
Member

I cleaned up the code a bit, ready to merge once CI completes

@keepassxreboot keepassxreboot deleted a comment from 8-bit-fox Oct 17, 2020
@droidmonkey droidmonkey merged commit eb6f0eb into keepassxreboot:develop Oct 17, 2020
@8-bit-fox 8-bit-fox deleted the feature/browser-search-by-path branch October 19, 2020 19:12
@phoerious phoerious added pr: new feature Pull request that adds a new feature and removed new feature labels Nov 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature: Browser pr: new feature Pull request that adds a new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants