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

Specific user api like login and signup #12

Merged
merged 11 commits into from
Feb 17, 2025

Conversation

AmitsinghTanwar007
Copy link
Collaborator

No description provided.

Comment on lines 26 to 28
username TEXT NOT NULL,
userid TEXT NOT NULL PRIMARY KEY,
password TEXT NOT NULL
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
username TEXT NOT NULL,
userid TEXT NOT NULL PRIMARY KEY,
password TEXT NOT NULL
user_name TEXT NOT NULL,
user_id TEXT NOT NULL PRIMARY KEY,
password TEXT NOT NULL

@@ -1 +1,2 @@
/target
/src/.envrc
Copy link
Owner

Choose a reason for hiding this comment

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

this looks wrong, please verify. It should be on

Suggested change
/src/.envrc
/.envrc

impl Config {
fn new() -> Self {
Config {
host: format!("http://{}:8030", env!("UPI_HOST")),
host: format!("http://{}:8030",env::var("UPI_HOST").unwrap()),
Copy link
Owner

Choose a reason for hiding this comment

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

This is also wrong, this will not work. the old code used env! which fetches the env at compile time and adds them. This does it on runtime. We don't support environment variable at runtime yet.

{
"input": "{ \"path1\": \"/upi/person1\", \"path2\": \"/upi/person2\", \"amount\": 10 }",
"input": "{ \"path1\": \"~/amit/mono\", \"path2\": \"/accounts/Nishant1234/nishant/mono\", \"amount\": 10 }",
Copy link
Owner

Choose a reason for hiding this comment

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

This should be:

Suggested change
"input": "{ \"path1\": \"~/amit/mono\", \"path2\": \"/accounts/Nishant1234/nishant/mono\", \"amount\": 10 }",
"input": "{ \"path1\": \"~/mono\", \"path2\": \"/accounts/Nishant1234/mono\", \"amount\": 10 }",

proto/user.proto Outdated
Comment on lines 10 to 13
string username = 1;
string name =2;
string email = 3;
string password = 4;
Copy link
Owner

Choose a reason for hiding this comment

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

please have consistent formatting

Err(_err) => {
return Ok(UserData{
message: false,
username: "".to_string(),
Copy link
Owner

Choose a reason for hiding this comment

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

this looks wrong, if you want to reset, please make username an Option<T> type instead of empty string.

None => return Err(Box::new(Status::unauthenticated("No JWT token found"))),
};

let secret = env::var("secret").unwrap_or_else(|_| "default_value".to_string());
Copy link
Owner

Choose a reason for hiding this comment

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

same here, and consider using a single defined constant for the environment variable if you are going with .b approach

let mut process_state = ProcessState::new(
UserCtx {
user_id: "root".to_string(),
user_id:user_id.to_string(),
Copy link
Owner

Choose a reason for hiding this comment

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

please consider running cargo +nightly fmt

.unwrap()
.as_secs() as usize,
};
let secret = env::var("secret").unwrap_or_else(|_| "default_value".to_string());
Copy link
Owner

Choose a reason for hiding this comment

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

same here.


#[async_trait]
impl UserStorage for SqliteStorage {
async fn insert(&self,username: &str, password: &str) -> Result<String> {
Copy link
Owner

Choose a reason for hiding this comment

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

insert of replace is a dangerous operation. This can overwrite a genuine user with someone random. Imagine if someone recreates the icici account? That would be bad. Only use insert.

@NishantJoshi00 NishantJoshi00 merged commit b859539 into NishantJoshi00:main Feb 17, 2025
2 of 4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants