PHP checking if one ID is asociated with active ID

Question

Im doing a project in PHP, something like a help desk(im new to world of programing). Every employee($zaposlenik) can answer every question once. If he already answered selected question he should be redirected to mojaTvrtka.php (home page of his workplace) and there should be a echo saying "You allready answered that question". Im not sure why it wont work. The else part of IF works fine.

<?php 
include("baza.php");
include("header.php");

$pid=$_POST['id'];
$zaposlenik=$_SESSION['activeUserId'];
$tekst=$_POST["tekstOdgovora"];
$datum=date('Y-m-d H:i:s');

$connect=connectDB();
$query1="SELECT pitanje_id,odgovor.zaposlenik_id AS odgZap
FROM odgovor
LEFT JOIN zaposlenik
ON odgovor.pitanje_id=zaposlenik.korisnik_id
WHERE odgovor.zaposlenik_id='$activeUserId'";

$result1=queryDB($connect,$query1);

if(mysqli_num_rows($result1) == $zaposlenik))
{
        header("Location:mojaTvrtka.php");
        echo "Vec ste odgovorili na ovo pitanje!";
}
else 
{
    $query="INSERT INTO odgovor
    (pitanje_id,zaposlenik_id,tekst,datum_vrijeme_odgovora) VALUES ('$pid','$zaposlenik','$tekst','$datum')";

    $result=queryDB($connect,$query);

header("Location: detaljiPitanja.php?id=$pid");
}

disconnectDB($connect);
?>

Answer 1

A few things,

The web is stateless, what this means is that from one request to another things are done separately (more on this later)

In your code

if(mysqli_num_rows($result1) == $zaposlenik))
{
    $message = urlencode("Vec ste odgovorili na ovo pitanje!"); //make sure to url encode it, so it's safe for query args
    header("Location:mojaTvrtka.php?flash_message=$message"); //pass message via the url
    exit(); //always exit after header, ABSOLUTELY no output is allowed before header
}

Now because of the stateless nature of the we have to use some kind of persistent storage to pass the message. Data is not passed across requests. This is part of why your echo doesn't work. The other reason is even if you do echo it, the page redirects so no one can see it. The simplest (and ugliest) way is to just pass that message as part of the url.

It's very important to use exit after a header redirect. As doing things after it can cause "strange" things to happen.

Then On the page this is directed to you can

 $flash_message = isset($_GET['flash_message']) ? "<div class=\"flash_message\" >{$_GET['flash_message']}</div>" : '';

 echo $flash_message;

Untested, but it's simple enough.

The other part

You have this:

  if(mysqli_num_rows($result1) == $zaposlenik))

Basically, what you are saying here is the activeUserId's value should match then number of results. So if my id is 120292 then I must have exactly that number or rows returned of this condition is false. Which then triggers the else, which of course has this $query="INSERT INTO odgovor.

In reality you just want to know if they have 1 row. So

 if(mysqli_num_rows($result1) == 1)//returns exactly 1 rows

Or you can test it's truthfulness simply by ding this way

if(mysqli_num_rows($result1)) //returns some number of rows

One last thing

Even though I cant read this:

$query1="SELECT pitanje_id,odgovor.zaposlenik_id AS odgZap
     FROM odgovor
     LEFT JOIN zaposlenik
     ON odgovor.pitanje_id=zaposlenik.korisnik_id
     WHERE odgovor.zaposlenik_id='$activeUserId'";

I can tell if you need to match a user ID and a question ID, that you have only one WHERE condition. So you probably need to add something for the question ID in there. I would assume that there is some form of relationship for this (which I assume is the answer table) that relates it to the question table. Such as a foreign key (the question id). So you just need to add that in, and get the id of the question in question (see what I did there).

//Obviously this is just PSUDO code, so replace this with your actual field and value...
WHERE odgovor.zaposlenik_id='$activeUserId' AND question_id='$questionId'";

SQL Injection

Also this stuff odgovor.zaposlenik_id='$activeUserId' is venerable to SQL Injection, so please use prepared statements.

And example of injection would be this:

   $activeUserId = "' OR 1 LIMIT 1 --";

What this would do is make your query this:

   SELECT pitanje_id,odgovor.zaposlenik_id AS odgZap
   FROM odgovor
   LEFT JOIN zaposlenik
   ON odgovor.pitanje_id=zaposlenik.korisnik_id
   WHERE odgovor.zaposlenik_id='' OR 1 LIMIT 1 --'

The -- is the start of a comment in SQL so nothing after that runs. What this does is select the first row (OR 1 is always true) and return just that row.

In this case it's pretty mundane as it would just act like they answered the question, but the point is I am able to execute any SQL command I want against your DB. Assuming user input finds it's way into $activeUserId, even if it doesn't is it worth the risk?

Prepared statements allow you do your SQL it like this;

    WHERE odgovor.zaposlenik_id=? AND question_id=?

When you prepare it the DB parses the SQL, says it's ok. Then the values are added in and executed when the DB knows not to make them SQL, thereby making these attacks impossible (If done correctly).

I find showing an example helps to illustrate how bad this is. That said I don't plan to write a tutorial on how to properly prepare queries as there are plenty out there on the web.

Cheers!