-
Notifications
You must be signed in to change notification settings - Fork 884
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 XEP-0070: Verifying HTTP Requests via XMPP support #557
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I usually do not add new features to stable Smack branches, so please adjust the target branch to master
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First round of review. I think especially the manager design needs to be changed.
...imental/src/main/java/org/jivesoftware/smackx/httpauthorizationrequest/packet/ConfirmIQ.java
Outdated
Show resolved
Hide resolved
...imental/src/main/java/org/jivesoftware/smackx/httpauthorizationrequest/packet/ConfirmIQ.java
Outdated
Show resolved
Hide resolved
|
||
private final String id; | ||
private final String method; | ||
private final String url; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
private final String url; | |
private final URI url; |
I believe this member should be of type URI
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this variable be handled as a string all the ways. Otherwise need to do extract steps during xml phrase() and in toXML() method.
In aTalk app implementation, the value is used as string in prompting user to accept or reject the auth request. Do not see the need for app to access the URL directly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The URI class is not intended to be in fields. So please keep as a string
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The URI class is not intended to be in fields.
Care to elaborate on that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The URI class has a lot of internal fields, the object size itself is 80 bytes and each internal string adds more and more e.g. URI.create("https://github.com/igniterealtime/Smack/pull/557")
took 400 bytes in memory (checked with JOL).
And the parsing itself also needs for CPU.
But there is no any logic that needs the parsed URI. We just need to show the link to a user.
...al/src/main/java/org/jivesoftware/smackx/httpauthorizationrequest/provider/package-info.java
Outdated
Show resolved
Hide resolved
...ntal/src/main/java/org/jivesoftware/smackx/httpauthorizationrequest/packet/package-info.java
Outdated
Show resolved
Hide resolved
...tal/src/main/java/org/jivesoftware/smackx/httpauthorizationrequest/element/package-info.java
Outdated
Show resolved
Hide resolved
...src/main/java/org/jivesoftware/smackx/httpauthorizationrequest/element/ConfirmExtension.java
Outdated
Show resolved
Hide resolved
...n/java/org/jivesoftware/smackx/httpauthorizationrequest/HTTPAuthorizationRequestManager.java
Outdated
Show resolved
Hide resolved
...n/java/org/jivesoftware/smackx/httpauthorizationrequest/HTTPAuthorizationRequestManager.java
Outdated
Show resolved
Hide resolved
...n/java/org/jivesoftware/smackx/httpauthorizationrequest/HTTPAuthorizationRequestManager.java
Outdated
Show resolved
Hide resolved
|
||
private final String id; | ||
private final String method; | ||
private final String url; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The URI class is not intended to be in fields. So please keep as a string
Thank you for your contribution!
Before creating a Pull Request, please make sure to
gradle check
successfully in order to make sure that your code does not break any JUnit tests and is conform to the projects code style.SMACK-XXX
) in the body of your commit message (not the header), so that Jira automatically links the PR to the issue.